Skip to content

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

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8698228
Reworking the instanceof feature to act more like a "default" than li…
weaverryan Apr 5, 2017
db96c89
Moving changes tracking from ChildDefinition to Definition and fixing…
weaverryan Apr 5, 2017
24a6a83
Adding a test for the config overriding order
weaverryan Apr 6, 2017
6202909
Fixing a bug where instanceof was not copied into the new Definition …
weaverryan Apr 6, 2017
28d7d8d
Adding a test for instanceof behavior with parent-child services
weaverryan Apr 6, 2017
de0f718
Adding more tests
weaverryan Apr 6, 2017
8fd3738
Updating calls logic to merge/override with instanceof
weaverryan Apr 6, 2017
5e39c5d
Using Definition instead of ChildDefinition for instanceofConditionals
weaverryan Apr 6, 2017
132201e
Fixing outdated type-hint
weaverryan Apr 6, 2017
d7c4836
Fixing method name after rebase
weaverryan Apr 6, 2017
a44d92b
Making instanceof method call matching case insensitive
weaverryan Apr 6, 2017
a2aea3b
Fixing an edge case with instanceof property overriding when value is…
weaverryan Apr 6, 2017
5f9ca7c
A few tweaks thanks to nicolas
weaverryan Apr 6, 2017
2021675
Fixing missing use
weaverryan Apr 6, 2017
a95de6c
Thanks fabbot!
weaverryan Apr 6, 2017
34ad21a
Updating more code for ChildDefinition -> Definition change
weaverryan Apr 6, 2017
371fec2
Fixing failing tests: some of the compiler passes caused false "chang…
weaverryan Apr 6, 2017
82fa3c0
Updating DI tags logic to merge, but not replace tags
weaverryan Apr 6, 2017
1f2ce11
thanks fabbot!
weaverryan Apr 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 0 additions & 121 deletions src/Symfony/Component/DependencyInjection/ChildDefinition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ class ChildDefinition extends Definition
{
private $parent;
private $inheritTags = false;
private $changes = array();

/**
* @param string $parent The id of Definition instance to decorate
Expand All @@ -45,16 +44,6 @@ public function getParent()
return $this->parent;
}

/**
* Returns all changes tracked for the Definition object.
*
* @return array An array of changes for this Definition
*/
public function getChanges()
{
return $this->changes;
}

/**
* Sets whether tags should be inherited from the parent or not.
*
Expand All @@ -79,116 +68,6 @@ public function getInheritTags()
return $this->inheritTags;
}

/**
* {@inheritdoc}
*/
public function setClass($class)
{
$this->changes['class'] = true;

return parent::setClass($class);
}

/**
* {@inheritdoc}
*/
public function setFactory($callable)
{
$this->changes['factory'] = true;

return parent::setFactory($callable);
}

/**
* {@inheritdoc}
*/
public function setConfigurator($callable)
{
$this->changes['configurator'] = true;

return parent::setConfigurator($callable);
}

/**
* {@inheritdoc}
*/
public function setFile($file)
{
$this->changes['file'] = true;

return parent::setFile($file);
}

/**
* {@inheritdoc}
*/
public function setShared($boolean)
{
$this->changes['shared'] = true;

return parent::setShared($boolean);
}

/**
* {@inheritdoc}
*/
public function setPublic($boolean)
{
$this->changes['public'] = true;

return parent::setPublic($boolean);
}

/**
* {@inheritdoc}
*/
public function setLazy($boolean)
{
$this->changes['lazy'] = true;

return parent::setLazy($boolean);
}

/**
* {@inheritdoc}
*/
public function setAbstract($boolean)
{
$this->changes['abstract'] = true;

return parent::setAbstract($boolean);
}

/**
* {@inheritdoc}
*/
public function setDecoratedService($id, $renamedId = null, $priority = 0)
{
$this->changes['decorated_service'] = true;

return parent::setDecoratedService($id, $renamedId, $priority);
}

/**
* {@inheritdoc}
*/
public function setDeprecated($boolean = true, $template = null)
{
$this->changes['deprecated'] = true;

return parent::setDeprecated($boolean, $template);
}

/**
* {@inheritdoc}
*/
public function setAutowired($autowired)
{
$this->changes['autowired'] = true;

return parent::setAutowired($autowired);
}

/**
* Gets an argument to pass to the service constructor/factory method.
*
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ public function __construct()
$this->beforeOptimizationPasses = array(
100 => array(
$resolveClassPass = new ResolveClassPass(),
new ResolveDefinitionInheritancePass(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nicolas-grekas nicolas-grekas Apr 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
);

$this->optimizationPasses = array(array(
new ExtensionCompilerPass(),
new ResolveDefinitionTemplatesPass(),
new ResolveDefinitionInheritancePass(),
new ServiceLocatorTagPass(),
new DecoratorServicePass(),
new ResolveParameterPlaceHoldersPass(),
Expand Down
76 changes: 38 additions & 38 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionInheritancePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\Definition;

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

$class = $value instanceof ChildDefinition ? $this->resolveDefinition($value) : $value->getClass();
$class = $value->getClass();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (!$class || false !== strpos($class, '%') || !$instanceof = $value->getInstanceofConditionals()) {
return parent::processValue($value, $isRoot);
Expand All @@ -39,55 +38,56 @@ protected function processValue($value, $isRoot = false)
continue;
}
if ($interface === $class || is_subclass_of($class, $interface)) {
$this->mergeDefinition($value, $definition);
$this->mergeInstanceofDefinition($value, $definition);
}
}

return parent::processValue($value, $isRoot);
}

/**
* Populates the class and tags from parent definitions.
*/
private function resolveDefinition(ChildDefinition $definition)
private function mergeInstanceofDefinition(Definition $def, Definition $instanceofDefinition)
{
if (!$this->container->has($parent = $definition->getParent())) {
return;
$configured = $def->getChanges();
$changes = $instanceofDefinition->getChanges();
if (!isset($configured['shared']) && isset($changes['shared'])) {
$def->setShared($instanceofDefinition->isShared());
}

$parentDef = $this->container->findDefinition($parent);
$class = $parentDef instanceof ChildDefinition ? $this->resolveDefinition($parentDef) : $parentDef->getClass();
$class = $definition->getClass() ?: $class;

// append parent tags when inheriting is enabled
if ($definition->getInheritTags()) {
$definition->setInheritTags(false);

foreach ($parentDef->getTags() as $k => $v) {
foreach ($v as $v) {
$definition->addTag($k, $v);
}
}
if (!isset($configured['configurator']) && isset($changes['configurator'])) {
$def->setConfigurator($instanceofDefinition->getConfigurator());
}

return $class;
}

private function mergeDefinition(Definition $def, ChildDefinition $definition)
{
$changes = $definition->getChanges();
if (isset($changes['shared'])) {
$def->setShared($definition->isShared());
if (!isset($configured['public']) && isset($changes['public'])) {
$def->setPublic($instanceofDefinition->isPublic());
}
if (isset($changes['abstract'])) {
$def->setAbstract($definition->isAbstract());
if (!isset($configured['lazy']) && isset($changes['lazy'])) {
$def->setLazy($instanceofDefinition->isLazy());
}
if (!isset($configured['autowired']) && isset($changes['autowired'])) {
$def->setAutowired($instanceofDefinition->isAutowired());
}
// merge properties
$properties = $def->getProperties();
foreach ($instanceofDefinition->getProperties() as $k => $v) {
// don't override properties set explicitly on the service
if (!array_key_exists($k, $properties)) {
$def->setProperty($k, $v);
}
}
// merge method calls
if ($instanceofCalls = $instanceofDefinition->getMethodCalls()) {
$currentCallMethods = array_map(function ($call) {
return strtolower($call[0]);
}, $def->getMethodCalls());

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

$def->setMethodCalls(array_merge($uniqueInstanceofCalls, $def->getMethodCalls()));
}

ResolveDefinitionTemplatesPass::mergeDefinition($def, $definition);

// prepend instanceof tags
$tailTags = $def->getTags();
if ($headTags = $definition->getTags()) {
if ($headTags = $instanceofDefinition->getTags()) {
$def->setTags($headTags);
foreach ($tailTags as $k => $v) {
foreach ($v as $v) {
Expand Down
68 changes: 44 additions & 24 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionTemplatesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,33 +95,27 @@ private function doResolveDefinition(ChildDefinition $definition)
if ($parentDef->isDeprecated()) {
$def->setDeprecated(true, $parentDef->getDeprecationMessage('%service_id%'));
}
$def->setFactory($parentDef->getFactory());
$def->setConfigurator($parentDef->getConfigurator());
$def->setFile($parentDef->getFile());
$def->setPublic($parentDef->isPublic());
$def->setLazy($parentDef->isLazy());
$def->setAutowired($parentDef->isAutowired());

self::mergeDefinition($def, $definition);

// merge autowiring types
foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
$def->addAutowiringType($autowiringType);
$parentChanges = $parentDef->getChanges();
if (isset($parentChanges['factory'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@GuilhemN GuilhemN Apr 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$def->setFactory($parentDef->getFactory());
}
if (isset($parentChanges['configurator'])) {
$def->setConfigurator($parentDef->getConfigurator());
}
if (isset($parentChanges['file'])) {
$def->setFile($parentDef->getFile());
}
if (isset($parentChanges['public'])) {
$def->setPublic($parentDef->isPublic());
}
if (isset($parentChanges['lazy'])) {
$def->setLazy($parentDef->isLazy());
}
if (isset($parentChanges['autowired'])) {
$def->setAutowired($parentDef->isAutowired());
}

// these attributes are always taken from the child
$def->setAbstract($definition->isAbstract());
$def->setShared($definition->isShared());
$def->setTags($definition->getTags());

return $def;
}

/**
* @internal
*/
public static function mergeDefinition(Definition $def, ChildDefinition $definition)
{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// overwrite with values specified in the decorator
$changes = $definition->getChanges();
if (isset($changes['class'])) {
Expand Down Expand Up @@ -182,5 +176,31 @@ public static function mergeDefinition(Definition $def, ChildDefinition $definit
if ($calls = $definition->getMethodCalls()) {
$def->setMethodCalls(array_merge($def->getMethodCalls(), $calls));
}

// merge autowiring types
foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
$def->addAutowiringType($autowiringType);
}

// these attributes are always taken from the child
if (isset($changes['abstract'])) {
$def->setAbstract($definition->isAbstract());
}
if (isset($changes['shared'])) {
$def->setShared($definition->isShared());
}
$def->setTags($definition->getTags());
$def->setInstanceofConditionals($definition->getInstanceofConditionals());

// append parent tags when inheriting is enabled
if ($definition->getInheritTags()) {
foreach ($parentDef->getTags() as $k => $v) {
foreach ($v as $v) {
$def->addTag($k, $v);
}
}
}

return $def;
}
}
3 changes: 3 additions & 0 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveParameterPlaceHoldersPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@ protected function processValue($value, $isRoot = false)
return $this->bag->resolveValue($value);
}
if ($value instanceof Definition) {
// don't record these as new changes to the Definition
$value->setTrackChanges(false);
$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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return parent::processValue($value, $isRoot);
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,13 @@ public function process(ContainerBuilder $container)
continue;
}

// don't record these as new changes to the Definition
$definition->setTrackChanges(false);
$definition->setArguments($this->processArguments($definition->getArguments()));
$definition->setMethodCalls($this->processArguments($definition->getMethodCalls()));
$definition->setProperties($this->processArguments($definition->getProperties()));
$definition->setFactory($this->processFactory($definition->getFactory()));
$definition->setTrackChanges(true);
}

foreach ($container->getAliases() as $id => $alias) {
Expand Down
Loading