8000 [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols by nicolas-grekas · Pull Request #46279 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[DependencyInjection] Optimize autowiring logic by telling it about excluded symbols #46279

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 1 commit into from
May 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
[DependencyInjection] Optimize autowiring logic by telling it about e…
…xcluded symbols
  • Loading branch information
nicolas-grekas committed May 8, 2022
commit 739789f384cca3b06a3b5f35793e116fff337473
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')
->set(SessionInterface::class)->abstract()->tag('container.excluded')
;
};
< 8000 th scope="col">Diff line change
Original file line number Diff line number
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());

6D40 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
Loading
0