Skip to content

Commit

Permalink
[DependencyInjection] Optimize autowiring logic by telling it about e…
Browse files Browse the repository at this point in the history
…xcluded symbols
  • Loading branch information
nicolas-grekas committed May 8, 2022
1 parent 8a26600 commit 739789f
Show file tree
Hide file tree
Showing 16 changed files with 123 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class UnusedTagsPass implements CompilerPassInterface
'console.command',
'container.env_var_loader',
'container.env_var_processor',
'container.excluded',
'container.hot_path',
'container.no_preload',
'container.preload',
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/services.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Bundle\FrameworkBundle\CacheWarmer\ConfigBuilderCacheWarmer;
use Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache;
use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\Config\Resource\SelfCheckingResourceChecker;
use Symfony\Component\Config\ResourceCheckerConfigCacheFactory;
use Symfony\Component\Console\ConsoleEvents;
Expand All @@ -26,7 +27,10 @@
use Symfony\Component\EventDispatcher\EventDispatcherInterface as EventDispatcherInterfaceComponentAlias;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\Session\SessionInterface;
use Symfony\Component\HttpFoundation\UrlHelper;
use Symfony\Component\HttpKernel\CacheClearer\ChainCacheClearer;
use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate;
Expand Down Expand Up @@ -218,5 +222,11 @@ class_exists(WorkflowEvents::class) ? WorkflowEvents::ALIASES : []
->set('config_builder.warmer', ConfigBuilderCacheWarmer::class)
->args([service(KernelInterface::class), service('logger')->nullOnInvalid()])
->tag('kernel.cache_warmer')

// register as abstract and excluded, aka not-autowirable types
->set(LoaderInterface::class)->abstract()->tag('container.excluded')
->set(Request::class)->abstract()->tag('container.excluded')
->set(Response::class)->abstract()->tag('container.excluded')

This comment has been minimized.

Copy link
@brusch

brusch Nov 7, 2022

Contributor

@nicolas-grekas This seems to break existing installations with the following error message:
The definition ".service_locator.Fz2ZPLc" has a reference to an abstract definition "Symfony\Component\HttpFoundation\Response". Abstract definitions cannot be the target of references.

See also:
https://github.com/pimcore/pimcore/actions/runs/3408995937/jobs/5670226500#step:7:702

Removing the line, fixes the issue for me.

This comment has been minimized.

Copy link
@stof

stof Nov 7, 2022

Member

please open an issue instead of commenting on the commit. Otherwise, this discussion will be lost

This comment has been minimized.

Copy link
@brusch

brusch Nov 7, 2022

Contributor

@stof sure, done: #48138

->set(SessionInterface::class)->abstract()->tag('container.excluded')
;
};
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ protected function processValue(mixed $value, bool $isRoot = false)
if (\is_array($value)) {
foreach ($value as $k => $v) {
if ($isRoot) {
if ($v->hasTag('container.excluded')) {
continue;
}
$this->currentId = $k;
}
if ($v !== $processedValue = $this->processValue($v, $isRoot)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,19 @@ private function createTypeNotFoundMessageCallback(TypedReference $reference, st

private function createTypeNotFoundMessage(TypedReference $reference, string $label, string $currentId): string
{
if (!$r = $this->container->getReflectionClass($type = $reference->getType(), false)) {
$type = $reference->getType();

$i = null;
$namespace = $type;
do {
$namespace = substr($namespace, 0, $i);

if ($this->container->hasDefinition($namespace) && $tag = $this->container->getDefinition($namespace)->getTag('container.excluded')) {
return sprintf('Cannot autowire service "%s": %s needs an instance of "%s" but this type has been excluded %s.', $currentId, $label, $type, $tag[0]['source'] ?? 'from autowiring');
}
} while (false !== $i = strrpos($namespace, '\\'));

if (!$r = $this->container->getReflectionClass($type, false)) {
// either $type does not exist or a parent class does not exist
try {
$resource = new ClassExistenceResource($type, false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,11 @@ public function merge(self $container)
throw new BadMethodCallException('Cannot merge on a compiled container.');
}

$this->addDefinitions($container->getDefinitions());
foreach ($container->getDefinitions() as $id => $definition) {
if (!$definition->hasTag('container.excluded') || !$this->has($id)) {
$this->setDefinition($id, $definition);
}
}
$this->addAliases($container->getAliases());
$this->getParameterBag()->add($container->getParameterBag()->all());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ class PrototypeConfigurator extends AbstractServiceConfigurator
private string $resource;
private ?array $excludes = null;
private bool $allowParent;
private ?string $path;

public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader, Definition $defaults, string $namespace, string $resource, bool $allowParent)
public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader, Definition $defaults, string $namespace, string $resource, bool $allowParent, string $path = null)
{
$definition = new Definition();
if (!$defaults->isPublic() || !$defaults->isPrivate()) {
Expand All @@ -57,6 +58,7 @@ public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader,
$this->loader = $loader;
$this->resource = $resource;
$this->allowParent = $allowParent;
$this->path = $path;

parent::__construct($parent, $definition, $namespace, $defaults->getTags());
}
Expand All @@ -66,7 +68,7 @@ public function __destruct()
parent::__destruct();

if (isset($this->loader)) {
$this->loader->registerClasses($this->definition, $this->id, $this->resource, $this->excludes);
$this->loader->registerClasses($this->definition, $this->id, $this->resource, $this->excludes, $this->path);
}
unset($this->loader);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ final public function alias(string $id, string $referencedId): AliasConfigurator
*/
final public function load(string $namespace, string $resource): PrototypeConfigurator
{
return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true);
return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true, $this->path);
}

/**
Expand Down
26 changes: 20 additions & 6 deletions src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ public function import(mixed $resource, string $type = null, bool|string $ignore
* @param string $namespace The namespace prefix of classes in the scanned directory
* @param string $resource The directory to look for classes, glob-patterns allowed
* @param string|string[]|null $exclude A globbed path of files to exclude or an array of globbed paths of files to exclude
* @param string|null $source The path to the file that defines the auto-discovery rule
*/
public function registerClasses(Definition $prototype, string $namespace, string $resource, string|array $exclude = null)
public function registerClasses(Definition $prototype, string $namespace, string $resource, string|array $exclude = null/*, string $source = null*/)
{
if (!str_ends_with($namespace, '\\')) {
throw new InvalidArgumentException(sprintf('Namespace prefix must end with a "\\": "%s".', $namespace));
Expand All @@ -100,9 +101,10 @@ public function registerClasses(Definition $prototype, string $namespace, string
throw new InvalidArgumentException(sprintf('Namespace is not a valid PSR-4 prefix: "%s".', $namespace));
}

$source = \func_num_args() > 4 ? func_get_arg(4) : null;
$autoconfigureAttributes = new RegisterAutoconfigureAttributesPass();
$autoconfigureAttributes = $autoconfigureAttributes->accept($prototype) ? $autoconfigureAttributes : null;
$classes = $this->findClasses($namespace, $resource, (array) $exclude, $autoconfigureAttributes);
$classes = $this->findClasses($namespace, $resource, (array) $exclude, $autoconfigureAttributes, $source);
// prepare for deep cloning
$serializedPrototype = serialize($prototype);

Expand Down Expand Up @@ -169,7 +171,7 @@ protected function setDefinition(string $id, Definition $definition)
}
}

private function findClasses(string $namespace, string $pattern, array $excludePatterns, ?RegisterAutoconfigureAttributesPass $autoconfigureAttributes): array
private function findClasses(string $namespace, string $pattern, array $excludePatterns, ?RegisterAutoconfigureAttributesPass $autoconfigureAttributes, ?string $source): array
{
$parameterBag = $this->container->getParameterBag();

Expand All @@ -189,7 +191,6 @@ private function findClasses(string $namespace, string $pattern, array $excludeP

$pattern = $parameterBag->unescapeValue($parameterBag->resolveValue($pattern));
$classes = [];
$extRegexp = '/\\.php$/';
$prefixLen = null;
foreach ($this->glob($pattern, true, $resource, false, false, $excludePaths) as $path => $info) {
if (null === $prefixLen) {
Expand All @@ -204,10 +205,10 @@ private function findClasses(string $namespace, string $pattern, array $excludeP
continue;
}

if (!preg_match($extRegexp, $path, $m) || !$info->isReadable()) {
if (!str_ends_with($path, '.php') || !$info->isReadable()) {
continue;
}
$class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, -\strlen($m[0]))), '\\');
$class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, -4)), '\\');

if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $class)) {
continue;
Expand Down Expand Up @@ -242,6 +243,19 @@ private function findClasses(string $namespace, string $pattern, array $excludeP
}
}

if (null !== $prefixLen) {
$attributes = null !== $source ? ['source' => sprintf('in "%s/%s"', basename(\dirname($source)), basename($source))] : [];

foreach ($excludePaths as $path => $_) {
$class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, str_ends_with($path, '.php') ? -4 : null)), '\\');
if (!$this->container->has($class)) {
$this->container->register($class)
->setAbstract(true)
->addTag('container.excluded', $attributes);
}
}
}

return $classes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ private function parseDefinitions(\DOMDocument $xml, string $file, Definition $d
}
$excludes = [$service->getAttribute('exclude')];
}
$this->registerClasses($definition, (string) $service->getAttribute('namespace'), (string) $service->getAttribute('resource'), $excludes);
$this->registerClasses($definition, (string) $service->getAttribute('namespace'), (string) $service->getAttribute('resource'), $excludes, $file);
} else {
$this->setDefinition((string) $service->getAttribute('id'), $definition);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ private function parseDefinition(string $id, array|string|null $service, string
}
$exclude = $service['exclude'] ?? null;
$namespace = $service['namespace'] ?? $id;
$this->registerClasses($definition, $namespace, $service['resource'], $exclude);
$this->registerClasses($definition, $namespace, $service['resource'], $exclude, $file);
} else {
$this->setDefinition($id, $definition);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@

require_once __DIR__.'/../Fixtures/includes/autowiring_classes.php';

/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class AutowirePassTest extends TestCase
{
public function testProcess()
Expand Down Expand Up @@ -1186,4 +1183,38 @@ public function testAsDecoratorAttribute()
$this->assertSame(AsDecoratorBaz::class.'.inner', (string) $container->getDefinition(AsDecoratorBaz::class)->getArgument(0));
$this->assertSame(2, $container->getDefinition(AsDecoratorBaz::class)->getArgument(0)->getInvalidBehavior());
}

public function testTypeSymbolExcluded()
{
$container = new ContainerBuilder();

$container->register(Foo::class)->setAbstract(true)->addTag('container.excluded', ['source' => 'for tests']);
$aDefinition = $container->register('a', NotGuessableArgument::class);
$aDefinition->setAutowired(true);

$pass = new AutowirePass();
try {
$pass->process($container);
$this->fail('AutowirePass should have thrown an exception');
} catch (AutowiringFailedException $e) {
$this->assertSame('Cannot autowire service "a": argument "$k" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotGuessableArgument::__construct()" needs an instance of "Symfony\Component\DependencyInjection\Tests\Compiler\Foo" but this type has been excluded for tests.', (string) $e->getMessage());
}
}

public function testTypeNamespaceExcluded()
{
$container = new ContainerBuilder();

$container->register(__NAMESPACE__)->setAbstract(true)->addTag('container.excluded');
$aDefinition = $container->register('a', NotGuessableArgument::class);
$aDefinition->setAutowired(true);

$pass = new AutowirePass();
try {
$pass->process($container);
$this->fail('AutowirePass should have thrown an exception');
} catch (AutowiringFailedException $e) {
$this->assertSame('Cannot autowire service "a": argument "$k" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotGuessableArgument::__construct()" needs an instance of "Symfony\Component\DependencyInjection\Tests\Compiler\Foo" but this type has been excluded from autowiring.', (string) $e->getMessage());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,21 @@ public function testMerge()
$this->assertSame(['AInterface' => $childDefA, 'BInterface' => $childDefB], $container->getAutoconfiguredInstanceof());
}

public function testMergeWithExcludedServices()
{
$container = new ContainerBuilder();
$container->setAlias('bar', 'foo');
$container->register('foo', 'Bar\FooClass');
$config = new ContainerBuilder();
$config->register('bar', 'Bar')->addTag('container.excluded');
$config->register('foo', 'Bar')->addTag('container.excluded');
$config->register('baz', 'Bar')->addTag('container.excluded');
$container->merge($config);
$this->assertEquals(['service_container', 'foo', 'baz'], array_keys($container->getDefinitions()));
$this->assertFalse($container->getDefinition('foo')->hasTag('container.excluded'));
$this->assertTrue($container->getDefinition('baz')->hasTag('container.excluded'));
}

public function testMergeThrowsExceptionForDuplicateAutomaticInstanceofDefinitions()
{
$this->expectException(InvalidArgumentException::class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\BadClasses\MissingParent;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\FooInterface;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\AnotherSub\DeeperBaz;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\AnotherSub;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Baz;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\Bar;
use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\BarInterface;
Expand Down Expand Up @@ -116,10 +116,15 @@ public function testRegisterClassesWithExclude()
'Prototype/{%other_dir%/AnotherSub,Foo.php}'
);

$this->assertTrue($container->has(Bar::class));
$this->assertTrue($container->has(Baz::class));
$this->assertFalse($container->has(Foo::class));
$this->assertFalse($container->has(DeeperBaz::class));
$this->assertFalse($container->getDefinition(Bar::class)->isAbstract());
$this->assertFalse($container->getDefinition(Baz::class)->isAbstract());
$this->assertTrue($container->getDefinition(Foo::class)->isAbstract());
$this->assertTrue($container->getDefinition(AnotherSub::class)->isAbstract());

$this->assertFalse($container->getDefinition(Bar::class)->hasTag('container.excluded'));
$this->assertFalse($container->getDefinition(Baz::class)->hasTag('container.excluded'));
$this->assertTrue($container->getDefinition(Foo::class)->hasTag('container.excluded'));
$this->assertTrue($container->getDefinition(AnotherSub::class)->hasTag('container.excluded'));

$this->assertEquals([BarInterface::class], array_keys($container->getAliases()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ public function testPrototype()
$loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml'));
$loader->load('services_prototype.xml');

$ids = array_keys($container->getDefinitions());
$ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
sort($ids);
$this->assertSame([Prototype\Foo::class, Prototype\Sub\Bar::class, 'service_container'], $ids);

Expand Down Expand Up @@ -750,7 +750,7 @@ public function testPrototypeExcludeWithArray()
$loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml'));
$loader->load('services_prototype_array.xml');

$ids = array_keys($container->getDefinitions());
$ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
sort($ids);
$this->assertSame([Prototype\Foo::class, Prototype\Sub\Bar::class, 'service_container'], $ids);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ public function testPrototype()
$loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
$loader->load('services_prototype.yml');

$ids = array_keys($container->getDefinitions());
$ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
sort($ids);
$this->assertSame([Prototype\Foo::class, Prototype\Sub\Bar::class, 'service_container'], $ids);

Expand Down Expand Up @@ -528,7 +528,7 @@ public function testPrototypeWithNamespace()
$loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
$loader->load('services_prototype_namespace.yml');

$ids = array_keys($container->getDefinitions());
$ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
sort($ids);

$this->assertSame([
Expand Down

0 comments on commit 739789f

Please sign in to comment.