Fixes issues #27828 and #28326 #29897
Conversation
08163ae to
5c037b9
Compare
5c037b9 to
3378889
Compare
|
I've amended and rebased the PR to clear of all the short array syntax highlights. |
There was a problem hiding this comment.
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 calling setDefinition 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 ($bindings as $argument => $binding) { | ||
| list(, $identifier) = $binding->getValues(); | ||
| $this->bindings[$id][$argument][$identifier] = true; |
There was a problem hiding this comment.
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 in BoundArgument as internal.
|
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 modify ResolveBindingsPass, 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? |
|
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 :)) |
…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 in ResolveBindingsPassTest, 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 the ContainerBuilder, 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.