[DI] Prevent AutowirePass from triggering irrelevant deprecations #22282
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We could register an error handler, but I'd do this only for deprecated definitions.
restoring the previous error handler properly should fix that issue. |
333fd36 to
0dbb1eb
Compare
| @@ -309,6 +319,10 @@ private function getReflectionClass($id, Definition $definition) | |||
| $reflector = false; | |||
| } | |||
|
|
|||
| if ($deprecated) { | |||
| restore_error_handler(); | |||
There was a problem hiding this comment.
this needs to happen even when the previous code throws (eg the previous error handler)
| @@ -301,6 +301,16 @@ private function getReflectionClass($id, Definition $definition) | |||
| return false; | |||
| } | |||
|
|
|||
| if ($deprecated = $definition->isDeprecated()) { | |||
| $prevErrorHandler = set_error_handler(function ($level, $message, $file, $line, $context) use (&$prevErrorHandler) { | |||
There was a problem hiding this comment.
$context is deprecated in 7.2, let's not use it
| return false; | ||
| } | ||
|
|
||
| return $prevErrorHandler ? $prevErrorHandler($level, $message, $file, $line, $context) : false; |
There was a problem hiding this comment.
onliner:
return E_USER_DEPRECATED === $level || !$prevErrorHandler ? false : $prevErrorHandler($level, $message, $file, $line);
a475e03 to
87d0879
Compare
87d0879 to
afaadbe
Compare
|
Comments addressed, thanks! |
069e2ff to
6f5b183
Compare
xabbuh
reviewed
Apr 5, 2017
| @@ -303,9 +303,28 @@ private function getReflectionClass($id, Definition $definition) | |||
|
|
|||
| $class = $this->container->getParameterBag()->resolveValue($class); | |||
|
|
|||
| if ($deprecated = $definition->isDeprecated()) { | |||
| $prevErrorHandler = set_error_handler(function ($level, $message, $file, $line) use (&$prevErrorHandler) { | |||
| return E_USER_DEPRECATED === $level || !$prevErrorHandler ? false : $prevErrorHandler($level, $message, $file, $line); | |||
There was a problem hiding this comment.
I don't find this very readable. Can we at least add some parentheses?
6f5b183 to
77927f4
Compare
|
Thank you @chalasr. |
fabpot
added a commit
that referenced
this pull request
Apr 5, 2017
…cations (chalasr) This PR was merged into the 2.8 branch. Discussion ---------- [DI] Prevent AutowirePass from triggering irrelevant deprecations | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no (just a failing test case atm) | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | n/a | License | MIT | Doc PR | n/a For populating available types the AutowirePass iterates over `$container->getDefinitions()` trying to instantiate a reflection class for each definition. Problem is that if one of these classes is deprecated, a notice is triggered due to the reflection, even if the service is actually never used. ~~Right now this only reproduces the issue with a failing test case~~, this bug (if we agree it's a bug) breaks the test suite of a bundle I maintain (see https://travis-ci.org/lexik/LexikJWTAuthenticationBundle/jobs/218275650#L262) Solutions I can think about for now: - ~~Skip deprecated definitions from type registering, meaning that if a service is deprecated a day, all autowired services that rely on it will suddenly be broken, also the bug would remain if the definition is not deprecated but relies on a deprecated class, not good I think~~ - Register an error handler ignoring deprecations during the type registering process (`AutowirePass#populateAvailableType()`), ~~works but makes my test suite say `THE ERROR HANDLER HAS CHANGED`. I'll push my try as a 2nd commit.~~ Thoughts? Commits ------- 77927f4 [DI] Prevent AutowirePass from triggering irrelevant deprecations
fabpot
added a commit
that referenced
this pull request
Apr 5, 2017
…g reflection against all existing services (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | yes | BC breaks? | yes - compile time only | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - (patch best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22295/files?w=1).) "By-id" autowiring, as introduced in #22060 is free from all the issues that "by-type" autowiring has: - it has no magic and requires explicit type<>id matching (*vs* using reflection on all services to cherry-pick *the* one that matches some type-hint *at that time*, which is fragile) - it is free from any ambiguities (*vs* the Damocles' sword of breaking config just by enabling some unrelated bundle) - it is easily introspected: just look at DI config files (*vs* inspecting the type-hierarchy of all services + their type-hints) - ~~it is side-effect free, thus plain predictable (*vs* auto-registration of discovered types as services)~~ - it plays nice with deprecated services or classes (see #22282) - *etc.* Another consideration is that any "by-type" autowired configuration is either broken (because of future ambiguities) - or equivalent to a "by-id" configuration (because resolving ambiguities *means* adding explicit type<>id mappings.) For theoreticians, we could say that "by-id" autowiring is the asymptotic limit of "by-type" autowiring :-) For all these reasons - and also because it reduces the complexity of the code base - I propose to change the behavior and only support "by-id" autowiring in 3.3. This will break some configurations relying on "by-type" autowiring. Yet the break will only happen at compile-time, which means this won't *silently* break any apps. For all core Symfony services, they will work out of the box thanks to #22098 *et al.* For the remaining services, fixing ones config should be pretty straightforward: just follow the suggestions provided by the exception messages. If they are fine to you, you'll end up with the exact same config in the end. And maybe you'll spot issues that were hidden previously. Commits ------- cc5e582 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For populating available types the AutowirePass iterates over
$container->getDefinitions()trying to instantiate a reflection class for each definition.Problem is that if one of these classes is deprecated, a notice is triggered due to the reflection, even if the service is actually never used.
Right now this only reproduces the issue with a failing test case, this bug (if we agree it's a bug) breaks the test suite of a bundle I maintain (see https://travis-ci.org/lexik/LexikJWTAuthenticationBundle/jobs/218275650#L262)Solutions I can think about for now:
Skip deprecated definitions from type registering, meaning that if a service is deprecated a day, all autowired services that rely on it will suddenly be broken, also the bug would remain if the definition is not deprecated but relies on a deprecated class, not good I thinkAutowirePass#populateAvailableType()),works but makes my test suite sayTHE ERROR HANDLER HAS CHANGED. I'll push my try as a 2nd commit.Thoughts?