Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Rework config hierarchy: defaults > instanceof > service config#22356
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
a301f86 to6fb83a1Compare| */ | ||
| publicfunctionsetChanges(array$changes) | ||
| { | ||
| $this->changes =array(); |
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.
$this->changes = $changes?
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.
good catch, fixed
6fb83a1 toab86457Compare| } | ||
| } | ||
| if ($didProcess) { | ||
| $container->register('abstract.'.__CLASS__,'')->setAbstract(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.
what is the goal of this ?
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.
that's the base abstract service all "instanceof" chains inherit from. SeeL56 below.
| 'abstract' =>'abstract', | ||
| 'deprecated' =>'deprecated', | ||
| 'factory' =>'factory', | ||
| 'arguments' =>'arguments', |
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.
Why are you removing these ?
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.
Because they make no sense as "instanceof" conditionals.
(borrowed from#22294, ping@weaverryan for more info)
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.
Mostly, I thought these were an "abuse" of instanceof - these things are better handled with a normal parent-child. But, I suppose there's nothing wrong with having them.
However, I thinkarguments should not be included. A definition could have multiple instanceof. And with parent-child, the child opts into its 1 specific parent. But with instanceof, these can be applied without the child knowing. I feel like there will be merging edge-cases with arguments.
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.
Let's discuss more the use offactory inside defaults and/or instanceof sections in#22189.
| /** | ||
| * Sets the tracked changes for the Definition object. | ||
| * | ||
| * @return $this |
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.
Maybe@internal?
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.
not need imho
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.
I would mark bothgetChanges() andsetChanges() as@internal.
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.
ChilDefinition::getChanges can't be made internal ("bc break")
| } | ||
| $value->setInheritTags(false); | ||
| if (!$this->container->has($parent =$value->getParent())) { |
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.
This surprised me at first - it looks like we're silencing invalid parent ids. But, you've done this simply so that the later ResolveDefinitionTemplatesPass can throw the proper exception. Maybe add a one-line comment mentioning a later pass checks for parent validity? I want to leave some breadcrumbs for the future :)
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.
actually, I made it throw, so that tag inheritance is made incompatible with dynamically created parents and save a few surprises (since tags would not be inherited for them, despite the field)
| { | ||
| $didProcess =false; | ||
| foreach ($container->getDefinitions()as$id =>$definition) { | ||
| if ($definitioninstanceof ChildDefinition) { |
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.
We could add a one-line comment here:
Don't apply instanceof to children definition (but it will still be applied to their parent)
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.
comment added
| if (isset($instanceofParent->getChanges()['shared'])) { | ||
| $shared =$instanceofParent->isShared(); | ||
| } |
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.
Why does shared need to be treated special? It looks likeResolveDefinitionTemplatesPass now treats it like any other field, so wouldn't that take care of this without any special code?
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.
shared isnot handled as the other fields (that'd be a BC break): it is never inherited from the parent, as before (see lines after "purposely ignored attributes": there is no "setShared" call there)
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.
You're right - and the integration test proves this. I didn't see it at first :)
b373e53 to7c47114Compareweaverryan commentedApr 11, 2017
👍 I've tested this, it's clear and I like it! |
…child, parent definitions
7c47114 to6d6116bComparefabpot commentedApr 11, 2017
Thank you@nicolas-grekas. |
…service config (weaverryan, nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Rework config hierarchy: defaults > instanceof > service config| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Replaces#22294.The main change is that ChildDefinition does not have "instanceof" applied anymore. All the complexity of the pass came from the merging logic required to deal with them. But in fact, we'd better not have any such logic and just not apply "instanceof" to ChildDefinition (but have them inherit from their parents with the usual logic).Commits-------6d6116b Adding an integration test for the hirarchy of defaults, instanceof, child, parent definitionsab86457 [DI] Rework config hierarchy: defaults > instanceof > service configcbaee55 [DI] Track changes at the "Definition" level
nicolas-grekas commentedApr 11, 2017
and thank you@weaverryan :) |
Uh oh!
There was an error while loading.Please reload this page.
Replaces#22294.
The main change is that ChildDefinition does not have "instanceof" applied anymore. All the complexity of the pass came from the merging logic required to deal with them. But in fact, we'd better not have any such logic and just not apply "instanceof" to ChildDefinition (but have them inherit from their parents with the usual logic).