Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixes issues #27828 and #28326#29897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
08163ae to5c037b9Compare5c037b9 to3378889Compareprzemyslaw-bogusz commentedJan 16, 2019
I've amended and rebased the PR to clear of all the short array syntax highlights. |
GuilhemN left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for taking care of this!
I think your approach is too simular to the one we just gave up on. I believe we should really focus on the root issue and detect when services are overriden in loaders as@nicolas-grekas suggested (something like just before callingsetDefinition in a loader, check if the id is already in use and if so mark its bindings as used before overriding it). This should reduce the impact of the change and make it less impredictible.setDefinition is called in so many places that modifying it has huge impacts (and more likely side effects as we've experienced).
| if (!empty($bindings)) { | ||
| foreach ($bindingsas$argument =>$binding) { | ||
| list(,$identifier) =$binding->getValues(); | ||
| $this->bindings[$id][$argument][$identifier] =true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't this marking all bindings as used? If I'm correct, no exception will ever be thrown.
Edit: ok, I misunderstood your code :)
But the issue we have is about overriden services (when a service is overriden the set of services bindings are tested on is smaller than expected, hence an exception might be thrown that wouldn't have been if the former service hadn't been replaced, it only happens when using defaults/shared bindings).
About what you solve here, I don't think we should support manual overriding of bindings since I almost consider the management of shared bindings inBoundArgument as internal.
przemyslaw-bogusz commentedJan 17, 2019
Before we start discussing this in detail, I would like to ask a general question, which I should have asked in the first place, before making the PR. This PR includes two fixes, that apply to bindings, and thus both modifyResolveBindingsPass, and that is why I have decided to merge them. However, I can divide it into two separete PRs, which will clearly indicate which fragments of the code belong to the given fix. If so, please advise me, is it better to make two PRs at the same time, or just one, and the second one after the first one is resolved? |
GuilhemN commentedJan 18, 2019
Since the two changes are about quite different issues, I would advise you to split your PR, one may be merged earlier this way but most of all it also it easier to review. (PRs affecting thousands of lines should also be split in fact but here the size of your PR is fine :)) |
przemyslaw-bogusz commentedJan 19, 2019
…ults (przemyslaw-bogusz)This PR was merged into the 4.3-dev branch.Discussion----------[DI] Fix bad error message for unused bind under _defaults| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27828| License | MIT**Sidenote**: I originally included the fix in#29897, but I decided to close the previous PR and divide it into two separate PRs for clarity.**Description:**With this fix, the message regarding an unused bind will have a clear information about the type of the bind (defined under __defaults_, __instanceof_ or _per service_), as well as the name of the file, in which it was configurated. It's for, both, YAML and XML configurations.For the core team, please note, that the fix assumes a possibility of definings binds under __instanceof_, which was introduced in#27806. But since fixes are merged into other branches, I thought that it might be necessary to inlude this possibility. If this case requires making separate fixes for different branches, I will gladly do it.Commits-------35bf420 [DI] Fix bad error message for unused bind under _defaults
#27828
I extended a new error message proposed by@weaverryan by adding information about the file in which the bind is located. This will require changing some tests inResolveBindingsPassTest, which have an expectedExceptionMessageRegexp assertion. I will do it once the new error message is accepted. If I am not mistaken, the bug may also affect configurations using XML instead of YAML, but I left it for a possible separate PR.
#28326
The included test is a small modification of a test proposed by@GuilhemN. The general idea for this fix is to track all the bindings in theContainerBuilder, to see, if a specific argument in a specific service has more than one bound value. If so, this means, that a default bind from one file was overwritten by a default from another file, and the overwritten may be treated as used.