8000 [DI] Rework config hierarchy: defaults > instanceof > service config by nicolas-grekas · Pull Request #22356 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 11, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Adding an integration test for the hirarchy of defaults, instanceof, …
…child, parent definitions
  • Loading branch information
weaverryan authored and nicolas-grekas committed Apr 11, 2017
commit 6d6116b920f70c08cc5936a90128f2e9c163af1c
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public function process(ContainerBuilder $container)
$didProcess = false;
foreach ($container->getDefinitions() as $id => $definition) {
if ($definition instanceof ChildDefinition) {
Copy link
Member

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added

// don't apply "instanceof" to children: it will be applied to their parent
continue;
}
if ($definition !== $processedDefinition = $this->processDefinition($container, $id, $definition)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;

/**
* Applies tags inheritance to definitions.
Expand All @@ -31,7 +32,7 @@ protected function processValue($value, $isRoot = false)
$value->setInheritTags(false);

if (!$this->container->has($parent = $value->getParent())) {
Copy link
Member

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

Copy link
Member Author

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)

return parent::processValue($value, $isRoot);
throw new RuntimeException(sprintf('Parent definition "%s" does not exist.', $parent));
}

$parentDef = $this->container->findDefinition($parent);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
namespace Symfony\Component\DependencyInjection\Tests\Compiler;

use PHPUnit\Framework\TestCase;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\Alias;
use Symfony\Component\DependencyInjection\Loader\YamlFileLoader;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\ContainerBuilder;

Expand Down Expand Up @@ -114,4 +116,79 @@ public function testProcessInlinesWhenThereAreMultipleReferencesButFromTheSameDe
$this->assertFalse($container->hasDefinition('b'));
$this->assertFalse($container->hasDefinition('c'), 'Service C was not inlined.');
}

public function testInstanceofDefaultsAndParentDefinitionResolution()
{
$container = new ContainerBuilder();
$container->setResourceTracking(false);

// loading YAML with an expressive test-case in that file
$loader = new YamlFileLoader($container, new FileLocator(__DIR__.'/../Fixtures/yaml'));
$loader->load('services_defaults_instanceof_parent.yml');
$container->compile();

// instanceof overrides defaults
$simpleService = $container->getDefinition('service_simple');
$this->assertFalse($simpleService->isAutowired());
$this->assertFalse($simpleService->isShared());

// all tags are kept
$this->assertEquals(
array(
'foo_tag' => array(array('priority' => 100), array()),
'bar_tag' => array(array()),
),
$simpleService->getTags()
);

// calls are all kept, but service-level calls are last
$this->assertEquals(
array(
// from instanceof
array('setSunshine', array('bright')),
// from service
array('enableSummer', array(true)),
array('setSunshine', array('warm')),
),
$simpleService->getMethodCalls()
);

// service override instanceof
$overrideService = $container->getDefinition('service_override_instanceof');
$this->assertTrue($overrideService->isAutowired());

// children definitions get no instanceof
$childDef = $container->getDefinition('child_service');
$this->assertEmpty($childDef->getTags());

$childDef2 = $container->getDefinition('child_service_with_parent_instanceof');
// taken from instanceof applied to parent
$this->assertFalse($childDef2->isAutowired());
// override the instanceof
$this->assertTrue($childDef2->isShared());
// tags inherit like normal
$this->assertEquals(
array(
'foo_tag' => array(array('priority' => 100), array()),
'bar_tag' => array(array()),
),
$simpleService->getTags()
);
}
}

class IntegrationTestStub extends IntegrationTestStubParent
{
}

class IntegrationTestStubParent
{
public function enableSummer($enable)
{
// methods used in calls - added here to prevent errors for not existing
}

public function setSunshine($type)
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
services:
_defaults:
autowire: true

_instanceof:
Symfony\Component\DependencyInjection\Tests\Compiler\IntegrationTestStubParent:
# should override _defaults
autowire: false
shared: false
tags:
- { name: foo_tag }
calls:
- [setSunshine, [bright]]

# a second instanceof that will be applied
Symfony\Component\DependencyInjection\Tests\Compiler\IntegrationTestStub:
tags:
- { name: bar_tag }

service_simple:
class: Symfony\Component\DependencyInjection\Tests\Compiler\IntegrationTestStub
tags:
- { name: foo_tag, priority: 100 }
# calls from instanceof are kept, but this comes later
calls:
- [enableSummer, [true]]
- [setSunshine, [warm]]

service_override_instanceof:
class: Symfony\Component\DependencyInjection\Tests\Compiler\IntegrationTestStub
# override instanceof
autowire: true

parent_service:
abstract: true
lazy: true

# instanceof will not be applied to this
child_service:
class: Symfony\Component\DependencyInjection\Tests\Compiler\IntegrationTestStub
parent: parent_service

parent_service_with_class:
abstract: true
class: Symfony\Component\DependencyInjection\Tests\Compiler\IntegrationTestStub

child_service_with_parent_instanceof:
parent: parent_service_with_class
shared: true
0