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

weaverryan · 2017-04-05T12:57:45Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no (because instanceof is new)
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	instanceof symfony/symfony-docs#7538

This reworks how defaults, instanceof and service config override each other. For example:

services:
    _defaults:
        autowire: true

    _instanceof:
        Symfony\Component\Security\Core\Authorization\Voter\VoterInterface:
            public: false
            autowire: false
            tags: [security.voter]

    AppBundle\Security\PostVoter:
        public: true
        tags:
            - { name: security.voter, priority: 100 }

	Before	After	Changed?
public	false (taken from `_instanceof`	true (service overrides `_instanceof`)	changed
autowired	false	false	no change
tags	2 `security.voter` tags	2 `security.voter` tags	no change
Logic	`_defaults` > service config > `_instanceof`	`_defaults` > `_instanceof` > service config

tl;dr; _instanceof now acts like _defaults, but for specific instances. As a user, this is how I expected it to work originally.

NOTE Much of the config merging behavior in this PR - i.e. tags, calls - could be accomplished without such a big diff. But any scalar config - i.e. public, autowire, configurator, etc - requires all of the changes in this PR.

Important Notes:

_instanceof has its own override/merge logic. Mostly, it's easy - scalar values override. Some are more opinionated:
- tags from _instanceof and service are merged - all tags are kept. It's up to the compiler pass to decide of 2 tags should be merged into 1 or if both are needed
- calls from _instanceof and service are merged, unless the method name of the call match, then only the one from the service wins (i.e. ability to override a call). _instanceof calls are called first
I removed support for arguments, factory, deprecated and abstract from _instanceof, and I think we could remove more. These feel like an abuse of _instanceof when a parent class is more proper (and the behavior of how parent-child config is merged is more predictable in general)
This also works with parent-child definitions. The parent-child relationship is resolved first and then instanceof is applied. This means the real hierarchy is:

_defaults > _instanceof > parent definition > child definition

And yep, there's a test for this in IntegrationTest :)

[Bug Fix] instanceofConditionals were being lost when a service had a parent. This was fixed in sha: 6202909. We simply take the conditionals from the child... which in practice will likely match the parent conditionals, unless they were created in different YAML files (in which case, using the _instanceof from the file where the child was configured is what we want)

Cheers!

weaverryan · 2017-04-05T12:58:45Z

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

@@ -42,13 +42,13 @@ public function __construct()
        $this->beforeOptimizationPasses = array(
            100 => array(
                $resolveClassPass = new ResolveClassPass(),
-                new ResolveDefinitionInheritancePass(),


I moved this down to be after ResolveDefinitionTemplatesPass on purpose: this allows the parent-child definitions to be resolved first. Then, the ResolveDefinitionInheritancePass doesn't need to worry about this.

Thinking about that, this should be kept here: all following compiler passes need to always win. Moving the pass after all DI extension passes is what makes change-tracking a nightmare.
unless I missed something, the full target order proposed in this PR should be:
defaults > instanceof > parents > service > passes

(vs defaults > parents > service > instanceof > passes as implemented today)

weaverryan · 2017-04-05T13:00:23Z

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

-     * @internal
-     */
-    public static function mergeDefinition(Definition $def, ChildDefinition $definition)
-    {


This class was simply changed to look like it did before #21530 (adjusted for changes since then)

weaverryan · 2017-04-05T13:04:29Z

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

@@ -27,7 +27,7 @@ protected function processValue($value, $isRoot = false)
            return parent::processValue($value, $isRoot);
        }

-        $class = $value instanceof ChildDefinition ? $this->resolveDefinition($value) : $value->getClass();
+        $class = $value->getClass();


This entire class/file could/should be renamed (ResolveInstanceofPass) - I didn't do it to keep the diff more manageable

nicolas-grekas · 2017-04-05T14:06:08Z

src/Symfony/Component/DependencyInjection/Definition.php

+     *
+     * @return array An array of configured parts for this Definition
+     */
+    public function getConfiguredParts()


should this replace getChanges on ChildDefinition? (or have getChanges migrate here instead?)

nicolas-grekas · 2017-04-05T14:10:37Z

don't add a tag if one by that name was already added

this is already doable with a minor change in the existing code base, isn't it?
if yes, then what's the purpose of this PR? I mean: either the description of the PR should be updated to expain why the other changes are required, or the patch be simplified drastically.
Or do I miss something?

stof · 2017-04-05T15:15:08Z

@weaverryan We still must have instanceof winning over defaults. Otherwise, it removes the possibility to say "I want all services in this file to be private, except for commands because they are required to be public".

Service-level config should win over instanceof, but not service defaults.

stof · 2017-04-05T15:16:09Z

the order needs to be _defaults > _instanceof > service, whcih each step winning over the previous one.

nicolas-grekas

This PR implements another merging logic than the current one. If we agree that this is the most intuitive behavior, then I'm fine with it :)

For the reader, the current behavior (pre this PR) is defaults -> service -> instanceof,
my reasoning doing so was from less explicit to most explicit:

less explicit: I don't know what I'm wiring => defaults
a bit more explicit: I know which class I'm wiring => service
most explicit: I know the type hierarchy of my class => instanceof

This matches the ordering of the configuration loading (since the code discovers things in this order also - this PR can't change that).

The logic in this PR is from less specific to most specific:

less specific: all services in that file => defaults
a bit more specific: some services in that file => instanceof
most specific: this service in that file => service

nicolas-grekas · 2017-04-06T12:38:05Z

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

+        $properties = $def->getProperties();
+        foreach ($instanceofDefinition->getProperties() as $k => $v) {
+            // don't override properties set explicitly on the service
+            if (!isset($properties[$k])) {


should use array_key_exists, or the + operator (would replace the foreach)

nicolas-grekas · 2017-04-06T12:39:56Z

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

+        }
+        // append method calls
+        if ($calls = $instanceofDefinition->getMethodCalls()) {
+            $def->setMethodCalls(array_merge($def->getMethodCalls(), $calls));


same-name existing calls should not be added IMHO

nicolas-grekas · 2017-04-06T12:43:47Z

src/Symfony/Component/DependencyInjection/Definition.php

+        return $this;
+    }
+
+    private function trackChange($type)


could be called "touch" (first read did tell me what it did - ie could have been the setter also, with boolean arg)

nicolas-grekas · 2017-04-06T12:46:20Z

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

            $definition->setAutowired($autowire);
+            if ($autowireDefaultsUsed) {


"if" not needed

nicolas-grekas · 2017-04-06T12:46:57Z

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

@@ -494,7 +502,13 @@ private function parseDefinition($id, $service, $file, array $defaults)
                throw new InvalidArgumentException(sprintf('Invalid autowire attribute: "by_type", "by_id", true or false expected, "%s" given in "%s".', is_string($autowire) ? $autowire : gettype($autowire), $file));
            }

+            if ($autowireDefaultsUsed) {


"if" not needed, call setTrackChanges(!$autowireDefaultsUsed)

nicolas-grekas · 2017-04-06T12:55:50Z

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

+        }
+        // merge method calls
+        if ($instanceofCalls = $instanceofDefinition->getMethodCalls()) {
+            $currentCallMethods = array_map(function($call) {


array_column to the rescue

Ah, but now I also need to strtolower for the case insensitive matching later :)

nicolas-grekas · 2017-04-06T12:56:14Z

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

+
+            $uniqueInstanceofCalls = array_filter($instanceofCalls, function($instanceofCall) use ($currentCallMethods) {
+                // don't add an instanceof call if it was overridden on the service
+                return !in_array($instanceofCall[0], $currentCallMethods);


should be case insensitive

+1! Got it + test update

…ke a ChildDefinition tl;dr Before, instanceof values would replace those explicitly set on services. Now, it acts more like a default: only setting a value on a service if that value wasn't explicitly set on that service (i.e. don't override). For things like tags and calls, they're merged intelligently and on a case-by-case basis (merging tags works different than merging calls)

… bug The bug is where _defaults was overriding _instanceof config. To handle that, we ignore change tracking when setting from _defaults

…after resolving children

This uncovered a bug, where in ResolveDefinitionTemplatePass, we need to be more careful when setting attributes so that we don't trigger change tracking when there really were not any changes.

… null

weaverryan · 2017-04-06T15:23:51Z

This is ready to go once again. I updated the description quite a bit to be as clear as possible.

Other than general code review, the question simply comes down to this: how do we want the override logic to work with _defaults, _instanceof and the service def? @nicolas-grekas explains the 2 here: #22294 (review)

Cheers!

…e" positives to the Definition object

weaverryan · 2017-04-06T16:26:05Z

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

            $value->setClass($this->bag->resolveValue($value->getClass()));
            $value->setFile($this->bag->resolveValue($value->getFile()));
            $value->setProperties($this->bag->resolveValue($value->getProperties()));
            $value->setMethodCalls($this->bag->resolveValue($value->getMethodCalls()));
+            $value->setTrackChanges(true);


This is a side-effect of the change tracking... changes. This wasn't actually needed to get the new behavior working correctly, it was just needed to get some tests to pass (i.e. the tests were compiling the container and then comparing Definition objects together... one of the Definition objects now had a few extra "changes")

weaverryan · 2017-04-06T16:29:58Z

One more update:

@nicolas-grekas and I agree that we shouldn't merge/override tags like this, but should update the compiler passes to intelligently handle multiple tags (because sometimes 1 tag should replace another, but other times 2 tags should really stay as 2 tags - i.e. controller.service_argument). I've made that change and updated the PR description.
@nicolas-grekas discussed with me that the new "changes" tracking may be difficult to maintain. Basically, it's not possible for the service definition config to override _instanceof without this. If we want the behavior described in this PR, it's necessary. If we hate the changes too much, then we need to stay with the old logic.

The logic is that compiler passes are the only ones that truly have enough information to know if one tag should replace another, or if both are needed.

GuilhemN · 2017-04-07T17:32:38Z

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

+            }
+            $definition->setAutowired($autowire)
+                ->setTrackChanges(true)
+            ;


What about updating this to:

if (isset($service['autowire'])) { $definition->setAutowired($service['autowire']); } elseif (isset($defaults['autowire'])) { $definition ->setTrackChanges(false) ->setAutowired($defaults['autowire']) ->setTrackChanges(true); }

GuilhemN · 2017-04-07T17:39:30Z

src/Symfony/Component/DependencyInjection/Definition.php

+     *
+     * @return $this
+     */
+    public function setTrackChanges($trackChanges)


What about marking this as @internal ? I don't think we should bother about untracked changes due to a choice of the user.
Maybe we could even use a bound callable where necessary.

GuilhemN · 2017-04-07T17:47:51Z

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

-        foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
-            $def->addAutowiringType($autowiringType);
+        $parentChanges = $parentDef->getChanges();
+        if (isset($parentChanges['factory'])) {


I see one fragility here.
What if the parent uses defaults?

services: _defaults: autowire: true parent: ~

services: Foo\Bar: parent: parent

Then Foo\Bar won't be autowired, right?

I think the defaults should always be tracked but we should allow to differentiate changes from defaults (maybe just internally, I see no need to do this in userland):

if (isset($defaults['autowire'])) { $definition->setTrackChanges('defaults') ->setAutowired($defaults['autowire']) ->setTrackChanges(true); }

and then use something like the following in the passes needing it:

$definition->getChanges($withoutDefaults = true);

We could also fix #22345 using this by tracking the detected class name differently (or maybe even by not tracking it).

weaverryan · 2017-04-11T14:25:25Z

Closing in favor of #22356

…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

carsonbot added Status: Needs Review DependencyInjection labels Apr 5, 2017

weaverryan commented Apr 5, 2017

View reviewed changes

weaverryan force-pushed the instanceof_as_defaults branch from 5279a96 to 33d341c Compare April 5, 2017 13:01

weaverryan commented Apr 5, 2017

View reviewed changes

nicolas-grekas reviewed Apr 5, 2017

View reviewed changes

weaverryan changed the title ~~[DI] Reworking instanceof to function as "default" values~~ [DI] Rework config hierarchy: defaults > instanceof > service config Apr 6, 2017

nicolas-grekas reviewed Apr 6, 2017

View reviewed changes

weaverryan added 8 commits April 6, 2017 10:54

Moving changes tracking from ChildDefinition to Definition and fixing…

db96c89

… bug The bug is where _defaults was overriding _instanceof config. To handle that, we ignore change tracking when setting from _defaults

Adding a test for the config overriding order

24a6a83

Fixing a bug where instanceof was not copied into the new Definition …

6202909

…after resolving children

Adding a test for instanceof behavior with parent-child services

28d7d8d

This uncovered a bug, where in ResolveDefinitionTemplatePass, we need to be more careful when setting attributes so that we don't trigger change tracking when there really were not any changes.

Adding more tests

de0f718

Updating calls logic to merge/override with instanceof

8fd3738

Using Definition instead of ChildDefinition for instanceofConditionals

5e39c5d

weaverryan force-pushed the instanceof_as_defaults branch from 8c76a7f to 5e39c5d Compare April 6, 2017 14:58

weaverryan added 7 commits April 6, 2017 11:02

Fixing outdated type-hint

132201e

Fixing method name after rebase

d7c4836

Making instanceof method call matching case insensitive

a44d92b

Fixing an edge case with instanceof property overriding when value is…

a2aea3b

… null

A few tweaks thanks to nicolas

5f9ca7c

Fixing missing use

2021675

Thanks fabbot!

a95de6c

Updating more code for ChildDefinition -> Definition change

34ad21a

weaverryan mentioned this pull request Apr 6, 2017

[DI] Introducing autoconfigure: automatic _instanceof configuration #22234

Closed

Fixing failing tests: some of the compiler passes caused false "chang…

371fec2

…e" positives to the Definition object

weaverryan commented Apr 6, 2017

View reviewed changes

weaverryan added 2 commits April 6, 2017 12:55

Updating DI tags logic to merge, but not replace tags

82fa3c0

The logic is that compiler passes are the only ones that truly have enough information to know if one tag should replace another, or if both are needed.

thanks fabbot!

1f2ce11

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

GuilhemN reviewed Apr 7, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Apr 10, 2017

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

Merged

weaverryan closed this Apr 11, 2017

weaverryan deleted the instanceof_as_defaults branch April 11, 2017 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

weaverryan commented Apr 5, 2017 •

edited

Loading

weaverryan Apr 5, 2017

nicolas-grekas Apr 7, 2017 •

edited

Loading

nicolas-grekas Apr 7, 2017

weaverryan Apr 5, 2017

weaverryan Apr 5, 2017

nicolas-grekas Apr 5, 2017

nicolas-grekas commented Apr 5, 2017 •

edited

Loading

stof commented Apr 5, 2017

stof commented Apr 5, 2017

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas Apr 6, 2017

nicolas-grekas Apr 6, 2017

nicolas-grekas Apr 6, 2017

nicolas-grekas Apr 6, 2017

nicolas-grekas Apr 6, 2017

nicolas-grekas Apr 6, 2017

weaverryan Apr 6, 2017

nicolas-grekas Apr 6, 2017

weaverryan Apr 6, 2017

weaverryan commented Apr 6, 2017

weaverryan Apr 6, 2017

weaverryan commented Apr 6, 2017 •

edited

Loading

GuilhemN Apr 7, 2017

GuilhemN Apr 7, 2017

GuilhemN Apr 7, 2017

GuilhemN Apr 10, 2017 •

edited

Loading

weaverryan commented Apr 11, 2017

		$definition->setAutowired($autowire);
		if ($autowireDefaultsUsed) {

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

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

Conversation

weaverryan commented Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Apr 7, 2017 • edited Loading

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

nicolas-grekas commented Apr 5, 2017 • edited Loading

stof commented Apr 5, 2017

stof commented Apr 5, 2017

nicolas-grekas left a comment • edited Loading

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

weaverryan commented Apr 6, 2017

Choose a reason for hiding this comment

weaverryan commented Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

weaverryan commented Apr 11, 2017

weaverryan commented Apr 5, 2017 •

edited

Loading

nicolas-grekas Apr 7, 2017 •

edited

Loading

nicolas-grekas commented Apr 5, 2017 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

weaverryan commented Apr 6, 2017 •

edited

Loading

GuilhemN Apr 10, 2017 •

edited

Loading