search

zendframework / zend-di

Di component from Zend Framework
BSD 3-Clause "New" or "Revised" License
46 stars 29 forks source link

Inject logger instance via constructor #34

Closed tux-rampage closed 6 years ago

tux-rampage commented 6 years ago

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.

tux-rampage commented 6 years ago

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:

  1. 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?
  2. 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?
Ocramius commented 6 years ago

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 commented 6 years ago

I assume a setter without an Aware implementation would be ok

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

tux-rampage commented 6 years ago

Thanks a lot for the clearification. :+1:

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.

tux-rampage commented 6 years ago

@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.

weierophinney commented 6 years ago

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

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

tux-rampage commented 6 years ago

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 commented 6 years ago

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. :smile:

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!

tux-rampage commented 6 years ago

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