[DI] Rework config hierarchy: defaults > instanceof > service config #22356

nicolas-grekas · 2017-04-10T10:12:21Z

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

chalasr · 2017-04-10T16:10:50Z

src/Symfony/Component/DependencyInjection/Definition.php

+     */
+    public function setChanges(array $changes)
+    {
+        $this->changes = array();


$this->changes = $changes?

good catch, fixed

stof · 2017-04-10T16:29:20Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php

+            }
+        }
+        if ($didProcess) {
+            $container->register('abstract.'.__CLASS__, '')->setAbstract(true);


what is the goal of this ?

that's the base abstract service all "instanceof" chains inherit from. See L56 below.

stof · 2017-04-10T16:39:38Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

-        'abstract' => 'abstract',
-        'deprecated' => 'deprecated',
-        'factory' => 'factory',
-        'arguments' => 'arguments',


Why are you removing these ?

Because they make no sense as "instanceof" conditionals.
(borrowed from #22294, ping @weaverryan for more info)

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 think arguments 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.

Let's discuss more the use of factory inside defaults and/or instanceof sections in #22189.

weaverryan · 2017-04-11T10:49:28Z

src/Symfony/Component/DependencyInjection/Definition.php

+    /**
+     * Sets the tracked changes for the Definition object.
+     *
+     * @return $this


Maybe @internal?

not need imho

I would mark both getChanges() and setChanges() as @internal.

ChilDefinition::getChanges can't be made internal ("bc break")

weaverryan · 2017-04-11T10:58:27Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveTagsInheritancePass.php

+        }
+        $value->setInheritTags(false);
+
+        if (!$this->container->has($parent = $value->getParent())) {


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 :)

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)

weaverryan · 2017-04-11T11:00:58Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php

+    {
+        $didProcess = false;
+        foreach ($container->getDefinitions() as $id => $definition) {
+            if ($definition instanceof ChildDefinition) {


We could add a one-line comment here:

Don't apply instanceof to children definition (but it will still be applied to their parent)

comment added

weaverryan · 2017-04-11T11:07:49Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php

+
+                if (isset($instanceofParent->getChanges()['shared'])) {
+                    $shared = $instanceofParent->isShared();
+                }


Why does shared need to be treated special? It looks like ResolveDefinitionTemplatesPass now treats it like any other field, so wouldn't that take care of this without any special code?

shared is not 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)

You're right - and the integration test proves this. I didn't see it at first :)

weaverryan · 2017-04-11T14:20:58Z

👍

I've tested this, it's clear and I like it!

…child, parent definitions

fabpot · 2017-04-11T16:32:39Z

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 definitions ab86457 [DI] Rework config hierarchy: defaults > instanceof > service config cbaee55 [DI] Track changes at the "Definition" level

nicolas-grekas · 2017-04-11T16:41:56Z

and thank you @weaverryan :)

nicolas-grekas added this to the 3.3 milestone Apr 10, 2017

carsonbot added Status: Needs Review DependencyInjection Bug labels Apr 10, 2017

nicolas-grekas force-pushed the di-instanceof-order branch from a301f86 to 6fb83a1 Compare April 10, 2017 11:22

chalasr reviewed Apr 10, 2017

View reviewed changes

weaverryan and others added 2 commits April 10, 2017 18:14

[DI] Track changes at the "Definition" level

cbaee55

[DI] Rework config hierarchy: defaults > instanceof > service config

ab86457

nicolas-grekas force-pushed the di-instanceof-order branch from 6fb83a1 to ab86457 Compare April 10, 2017 16:14

stof reviewed Apr 10, 2017

View reviewed changes

weaverryan reviewed Apr 11, 2017

View reviewed changes

nicolas-grekas force-pushed the di-instanceof-order branch from b373e53 to 7c47114 Compare April 11, 2017 14:17

weaverryan mentioned this pull request Apr 11, 2017

[DI] Rework config hierarchy: defaults > instanceof > service config #22294

Closed

Adding an integration test for the hirarchy of defaults, instanceof, …

6d6116b

…child, parent definitions

nicolas-grekas force-pushed the di-instanceof-order branch from 7c47114 to 6d6116b Compare April 11, 2017 14:40

fabpot merged commit 6d6116b into symfony:master Apr 11, 2017

nicolas-grekas deleted the di-instanceof-order branch April 11, 2017 16:41

nicolas-grekas mentioned this pull request Apr 20, 2017

[DI] master: Can't use 'factory' in '_instanceof' in service Yaml #22489

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Rework config hierarchy: defaults > instanceof > service config #22356

[DI] Rework config hierarchy: defaults > instanceof > service config #22356

[DI] Rework config hierarchy: defaults > instanceof > service config #22356

[DI] Rework config hierarchy: defaults > instanceof > service config #22356

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment