Inject logger instance via constructor #34

tux-rampage · 2018-01-22T10:01:43Z

PR #32 introduces logging capabilities by the use of an "Aware" interface (LoggerAwareInterface of PSR-3). This is discouraged in ZF.

This pull request changes it to constructor injection and removes the introduced usage of Aware.

Since this fixes a violation that only exists in the develop branch, this PR is targeted against develop instead of master.

Are you fixing a bug?
- Detail how the bug is invoked currently.
- Detail the original, incorrect behavior.
- Detail the new, expected behavior.
- Add a regression test that demonstrates the bug, and proves the fix.
- Add a CHANGELOG.md entry for the fix.
Is this related to quality assurance?
As stated by @Ocramius in PR Add Logging To Code Generators (WIP) #32, the usage of aware-interfaces are discouraged throughout the Framework

tux-rampage · 2018-01-22T10:02:22Z

.gitignore

@@ -5,3 +5,4 @@
 /vendor/
 /zf-mkdoc-theme.tgz
 /zf-mkdoc-theme/
+/.idea


Please don't mind this change, it will be reverted when I complete the PR

Ocramius · 2018-01-22T10:13:57Z

.gitignore

@@ -5,3 +5,4 @@
 /vendor/
 /zf-mkdoc-theme.tgz
 /zf-mkdoc-theme/
+/.idea


Ocramius · 2018-01-22T10:14:35Z

test/CodeGenerator/InjectorGeneratorTest.php

        $logger = $this->prophesize(LoggerInterface::class);

+        $generator = new InjectorGenerator($config, $resolver, null, $logger->reveal());


Did you also change the constructor, or was it already accepting an optional logger?

This was the test, only. Just commited the code changes ;-)

tux-rampage · 2018-01-22T10:24:52Z

CHANGELOG.md

+- [#33](https://github.com/zendframework/zend-di/pull/32) add ability to pass
+  `Psr\Log\LoggerInterface` to `Zend\Di\CodeGenerator\InjectorGenerator`'s constructor
+
+
 - [#32](https://github.com/zendframework/zend-di/pull/32) adds the implementation of


@Ocramius I'm a bit unsure about leaving this in the changelog. Should it be removed as for readability (Same goes for the Removed and Fixed section). Since the changes from PR #32 were never released.

Note is fine as long as you aggregate it with the changes in this patch. What matters is that the reader of the release notes understands what happened.

I guess it would be more confusing then, since it may lead people to the assumption that 3.1 introduces the Aware implementation which is actually not the case. I moved the reference to PR #32 to the Removed and Fixed entries to state clearly that this is not the case.

tux-rampage · 2018-01-22T10:25:59Z

CHANGELOG.md

+- [#33](https://github.com/zendframework/zend-di/pull/32) add ability to pass
+  `Psr\Log\LoggerInterface` to `Zend\Di\CodeGenerator\InjectorGenerator`'s constructor
+
+


Noted: Double space willl be removed -> single space.

tux-rampage · 2018-01-22T10:39:19Z

Just for the records:

I chose the Aware implementation of PSR-3 since it is aliving PSR standard and it states a purely optional dependency that does not affect the classes core behaviour. But aware implementations are discouraged. Since this was already discussed - it will be removed without further arguing. (@Ocramius a ref would be awesome, if you got it at hand).

@Ocramius just two things to prevent this in the future and reference potential PR's to this:

Can you please specify your disagreement with the following line any further in the following terms https://github.com/zendframework/zend-di/pull/32/files#diff-6f24fd8c91ea4350af062354fa9081d2R19

Is third party use of traits discouraged*
Because it implements aware?
Anything else?

I assume a setter without an Aware implementation would be ok to make life easier for consumers that want to decorate an already existing instance?

Removed the added note since it might lead to confusion to the reader and moved the ref to PR zendframework#23 to the Removed and Fixed section which states more clearly what happened

Ocramius · 2018-01-22T10:58:38Z

Is third party

Minor issue, given the interface stability

use of traits discouraged

Always discouraged. Only ever use traits in tests, as any downstream users may also use them and therefore we increase the BC surface by A LOT.

Because it implements aware?

This is the bigger issue - there is no need to expose that, as it is an internal implementation detail. Also, dependencies should not be mutable, and the *aware* interfaces render internal components mutable.

Ocramius · 2018-01-22T10:59:56Z

I assume a setter without an Aware implementation would be ok

The existence of the setter is actually the bigger issue here.

tux-rampage · 2018-01-22T11:12:34Z

Thanks a lot for the clearification. 👍

use of traits discouraged

Always discouraged.

The internal use only, could be stated with an "internal" annotation, but I got your point. Even though it was marked internal someone will use it and blame the framework when the trait changes. I'll open a PR removing remaining traits in this component (but this will be low prio). By now no more introduction of new traits.

The existence of the setter is actually the bigger issue here.

Even though it is not an actual dependency, more a utility aggregation (the generator's functionality itself does not actually depend on a logger)? I find this a bit too dogmatic, but I'm fine with cutting this.

edit: thank's github for mentioning internal ...
edit2: This comment is informational and topic for discussion elsewhere. With this PR the trait is removed as requested.

michalbundyra · 2018-01-22T14:08:42Z

src/CodeGenerator/InjectorGenerator.php

    /**
     * Constructs the compiler instance
     *
     * @param ConfigInterface $config The configuration to compile from
     * @param DependencyResolverInterface $resolver The resolver to utilize
     * @param string $namespace Namespace to use for generated class; defaults
     *     to Zend\Di\Generated.
+     * @param LoggerInterface $logger An optional logger instance to log failures


Missing null type in php doc.

Correct, on $namespace as well ...

michalbundyra · 2018-01-24T10:26:51Z

CHANGELOG.md

- Nothing.
+- [#33](https://github.com/zendframework/zend-di/pull/32) removes the implementation of
+  `Psr\Log\LoggerAwareInterface` from `Zend\Di\CodeGenerator\InjectorGenerator` that
+  was introduced with [#32](https://github.com/zendframework/zend-di/pull/32)


I think we should remove it, because #32 is not going to be released, because this PR replaces it. The same in Fixed section.
Notes in Added section are enough. But this is my option and maybe I am wrong?

I agree, in fact I was a bit unsure about it. I'd like to remove these entries for the same reason.
@Ocramius any objections?

I agree with @webimpress. Instead, note that #32 and #34 provide the ability to log generator processing, and give a short example of how that can be enabled.

tux-rampage · 2018-01-24T11:47:10Z

@Ocramius since you brought this up: Can you please approve?
I'd like to merge this by tomorrow, and update the changelog according to the suggestions from @webimpress, if there are no further objections.

Ocramius

LGTM 👍

weierophinney · 2018-01-24T17:22:35Z

edit: thank's github for mentioning internal ...

Surround annotation markings with code markers to avoid that: @internal. 😄

weierophinney

Looks good; I've noted a few documentation/changelog changes.

Also: I think marking any traits developed for internal purposes with an @internal class-level annotation is reasonable. That can be done later, though.

weierophinney · 2018-01-24T17:24:12Z

CHANGELOG.md

- Nothing.
+- [#33](https://github.com/zendframework/zend-di/pull/32) removes the implementation of
+  `Psr\Log\LoggerAwareInterface` from `Zend\Di\CodeGenerator\InjectorGenerator` that
+  was introduced with [#32](https://github.com/zendframework/zend-di/pull/32)


I agree with @webimpress. Instead, note that #32 and #34 provide the ability to log generator processing, and give a short example of how that can be enabled.

weierophinney · 2018-01-24T17:24:37Z

CHANGELOG.md


 ### Fixed

- Nothing.
+- [#33](https://github.com/zendframework/zend-di/pull/32) fixes discouraged use of
+  `Psr\Log\LoggerAware*` that was introduced with [#32](https://github.com/zendframework/zend-di/pull/32)


No need to add this here, as it was never previously released.

weierophinney · 2018-01-24T17:25:32Z

docs/book/codegen.md

-The `InjectorGenerator` implements the PSR-3 [`LoggerAwareInterface`](http://www.php-fig.org/psr/psr-3/#4-psrlogloggerawareinterface).
-So you can pass any PSR-3 logger to its `setLogger()` method.
+The `InjectorGenerator` allows to pass a [PSR-3 logger](http://www.php-fig.org/psr/psr-3/) as optional 
+fourth construction parameter.


Rephrase:

The InjectorGenerator allows passing a PSR-3 logger instance via an optional, fourth constuctor parameter.

weierophinney · 2018-01-24T17:26:30Z

src/CodeGenerator/InjectorGenerator.php

     */
    public function __construct(
        ConfigInterface $config,
        DependencyResolverInterface $resolver,
-        ?string $namespace = null
+        ?string $namespace = null,


Just an FYI: you don't need to use a nullable type here when providing a default null value. (Something @webimpress demonstrated to me recently.)

tux-rampage · 2018-01-24T19:14:59Z

Instead, note that #32 and #34 provide the ability to log generator processing

Now I'm confused again. Since #34 is replacing #32, I'd just remove the refs to #32.

and give a short example of how that can be enabled.

In the changelog? I'd just place a constructor example, would that be sufficient?

Maybe it's too late and I should finish for today, and you can enlighten me tomorrow in the slack channel.

weierophinney · 2018-01-24T19:18:28Z

Now I'm confused again. Since #34 is replacing #32, I'd just remove the refs to #32.

Just note whatever PRs led to the logger capabilities; I may be a bit fuzzy on which ones to use. 😄

and give a short example of how that can be enabled.

In the changelog? I'd just place a constructor example, would that be sufficient?

Perfect!

Typehints of parameters, that have a default value of `null` do not have to be marked nullable explicitly, i.e. `?string` can be just `string`.

tux-rampage · 2018-01-25T05:57:55Z

@weierophinney I incorporated the resquested changes. I also changed the title of this PR to reflect what was actually changed. Can you approve, please?

weierophinney

I've noted a slight rephrase of the changelog entry, but you can do that on merge.

Looks great! 👍

weierophinney · 2018-01-25T15:51:07Z

CHANGELOG.md

- [#32](https://github.com/zendframework/zend-di/pull/32) adds the implementation of
-  `Psr\Log\LoggerAwareInterface` to `Zend\Di\CodeGenerator\InjectorGenerator`
+- [#34](https://github.com/zendframework/zend-di/pull/34) adds ability to pass
+  `Psr\Log\LoggerInterface` to `Zend\Di\CodeGenerator\InjectorGenerator`'s constructor 


Rephrase slightly:

... adds the ability to pass a Psr\Log\LoggerInterface instance to the constructor of Zend\Di\CodeGenerator\InjectorGenerator

Inject logger instance via constructor

See: #34 (comment)

Close #34

Change test to constructor injection

86b40ea

tux-rampage commented Jan 22, 2018

View reviewed changes

tux-rampage requested a review from Ocramius January 22, 2018 10:02

tux-rampage self-assigned this Jan 22, 2018

tux-rampage added the bug label Jan 22, 2018

tux-rampage added this to the 3.1.0 milestone Jan 22, 2018

tux-rampage added 3 commits January 22, 2018 11:04

Remove the use of logger aware trait

d01d106

Change logger consumption to constructor injection

bc72b60

Revert local changes to .gitignore

5f9bb1f

Ocramius suggested changes Jan 22, 2018

View reviewed changes

tux-rampage added 2 commits January 22, 2018 11:19

Update changelog

c488646

Update documentation

df8b616

tux-rampage commented Jan 22, 2018

View reviewed changes

tux-rampage changed the title ~~Change generator to constructor injection (WIP)~~ Change generator to constructor injection Jan 22, 2018

Rephrase the changelog

6a5ff77

Removed the added note since it might lead to confusion to the reader and moved the ref to PR zendframework#23 to the Removed and Fixed section which states more clearly what happened

tux-rampage added the review label Jan 22, 2018

michalbundyra reviewed Jan 22, 2018

View reviewed changes

Correct constructor phpdoc

f629444

michalbundyra reviewed Jan 24, 2018

View reviewed changes

Ocramius approved these changes Jan 24, 2018

View reviewed changes

tux-rampage added approved and removed review labels Jan 24, 2018

weierophinney suggested changes Jan 24, 2018

View reviewed changes

tux-rampage changed the title ~~Change generator to constructor injection~~ Inject Logger Instance via constructor Jan 25, 2018

tux-rampage changed the title ~~Inject Logger Instance via constructor~~ Inject logger instance via constructor Jan 25, 2018

tux-rampage added 3 commits January 25, 2018 06:45

Rephrase changelog and documentation

5304a77

Correct typehint style

814501f

Typehints of parameters, that have a default value of `null` do not have to be marked nullable explicitly, i.e. `?string` can be just `string`.

Add missing dot after "Nothing"

77f8759

Revert accidental changes to .gitignore

2f65872

weierophinney approved these changes Jan 25, 2018

View reviewed changes

tux-rampage merged commit 2f65872 into zendframework:develop Jan 26, 2018

tux-rampage added a commit that referenced this pull request Jan 26, 2018

Merge pull request #34 from tux-rampage/logging-fixes

be66f72

Inject logger instance via constructor

tux-rampage added a commit that referenced this pull request Jan 26, 2018

Update changelog

f28afc9

See: #34 (comment)

tux-rampage added a commit that referenced this pull request Jan 26, 2018

Merge branch 'feature/34' into develop

b6a9cf7

Close #34

tux-rampage deleted the logging-fixes branch November 27, 2018 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inject logger instance via constructor #34

Inject logger instance via constructor #34

tux-rampage commented Jan 22, 2018 •

edited

Loading

tux-rampage Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

Ocramius Jan 22, 2018

tux-rampage Jan 22, 2018

tux-rampage Jan 22, 2018

Ocramius Jan 22, 2018

tux-rampage Jan 22, 2018

tux-rampage Jan 22, 2018

tux-rampage commented Jan 22, 2018

Ocramius commented Jan 22, 2018

Ocramius commented Jan 22, 2018

tux-rampage commented Jan 22, 2018 •

edited

Loading

michalbundyra Jan 22, 2018

tux-rampage Jan 22, 2018

michalbundyra Jan 24, 2018

tux-rampage Jan 24, 2018

weierophinney Jan 24, 2018

tux-rampage commented Jan 24, 2018

Ocramius left a comment

weierophinney commented Jan 24, 2018

weierophinney left a comment

weierophinney Jan 24, 2018

weierophinney Jan 24, 2018

weierophinney Jan 24, 2018

weierophinney Jan 24, 2018

tux-rampage commented Jan 24, 2018

weierophinney commented Jan 24, 2018 •

edited

Loading

tux-rampage commented Jan 25, 2018

weierophinney left a comment

weierophinney Jan 25, 2018

		$logger = $this->prophesize(LoggerInterface::class);

		$generator = new InjectorGenerator($config, $resolver, null, $logger->reveal());

		- [#33](https://github.com/zendframework/zend-di/pull/32) add ability to pass
		`Psr\Log\LoggerInterface` to `Zend\Di\CodeGenerator\InjectorGenerator`'s constructor

Inject logger instance via constructor #34

Inject logger instance via constructor #34

Conversation

tux-rampage commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tux-rampage commented Jan 22, 2018

Ocramius commented Jan 22, 2018

Ocramius commented Jan 22, 2018

tux-rampage commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tux-rampage commented Jan 24, 2018

Ocramius left a comment

Choose a reason for hiding this comment

weierophinney commented Jan 24, 2018

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tux-rampage commented Jan 24, 2018

weierophinney commented Jan 24, 2018 • edited Loading

tux-rampage commented Jan 25, 2018

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tux-rampage commented Jan 22, 2018 •

edited

Loading

tux-rampage commented Jan 22, 2018 •

edited

Loading

weierophinney commented Jan 24, 2018 •

edited

Loading