Replace Logger::deprecated with PRS-3 Logger::warning

[rvanlaak]

... in order to be able to inject Monolog logger or other PRS-3 Loggers

see #511

[ycombinator]

Thanks for the PR, @rvanlaak.

Your change works but I'd like to refactor the functionality of generating the method deprecation message into a single method somewhere. Would you mind creating an OpenCloud\Common\Log\Util class with a static method that generates and returns the method deprecation message? Then you can call that static method from all the places you are calling $service->getLogger()->warning(), passing its result to the warning() method like so:

use OpenCloud\Common\Log\Util;

$service->getLogger()->warning(Util::generateMethodDeprecationMessage(__METHOD__, 'new method or class'));

Also, as noted in the CONTRIBUTING.md file, please run vendor/bin/php-cs-fixer fix --diff --level psr2 . before committing code. This will ensure that the build will pass in Travis CI as well.

Thanks, again, for the submission!

[rvanlaak]

What about changing Logger::deprecated into a static function, and let it return the message? I agree with you that this PR otherwise introduces multiple places to modify the deprecation message.

Another question about the Logger: does the container that is returned by OpenStack->objectStoreService inherit the Logger instance from the OpenStack object?

[ycombinator]

What about changing Logger::deprecated into a static function, and let it return the message? I agree with you that this PR otherwise introduces multiple places to modify the deprecation message.

Yes, that could work too! One more thing -- please add a unit test for this method in the tests/OpenCloud/Tests/Common/Log/LoggerTest.php file.

Another question about the Logger: does the container that is returned by OpenStack->objectStoreService inherit the Logger instance from the OpenStack object?

No, I believe it has to be set explicitly via $container->setLogger() but @jamiehannaford could confirm this.

[rvanlaak]

Are there any scenarios that it shouldn't be favorable to also pass the parent's logger when creating a Service or Container? Otherwise I propose to let the ServiceBuilder::factory set the logger in the new class if not null.

[jamiehannaford]

I can't imagine why you would not want the parent logger to be injected by default. If a user does not want this, they can reset with setLogger(null). So I'm in favour of adding to ServiceBuilder as mentioned by @rvanlaak

[jamiehannaford]

@rvanlaak Could you rebase against working and re-push your changes? We had to revert a PR on that branch, so it's kept the commit history in this PR

[rvanlaak]

Actually never did a rebase before. Tried it a couple of times, but I seem to be missing the point.

Do I need to checkout the current upstream/working branch, and rebase the changes of deprecated on top of that?

[jamiehannaford]

I'd do something like this:

git checkout deprecated
git fetch upstream/working
git rebase upstream/working
git push -f <my_remote> deprecated

[rvanlaak]

Rebasing against upstream/working somehow does still include all unwanted commits. Maybe you can pick the commits?

[ycombinator]

@rvanlaak Hmm. Weird. I'll cherry-pick the commits over to working. Thanks!

[ycombinator]

@rvanlaak I've cherry-picked commits a85a1a4 and c8e2de1 from your branch into the working branch. Closing this PR now. Thank you for this contribution!

[rvanlaak]

👍

[rvanlaak]

@jamiehannaford I've submitted PR #518 to reuse the Logger instance