Skip to content

[DI] Improve PSR4-based service discovery #23986

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 1 commit into from
Closed

[DI] Improve PSR4-based service discovery #23986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 46 additions & 13 deletions src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,6 @@ public function __construct(ContainerBuilder $container, FileLocatorInterface $l
*/
public function registerClasses(Definition $prototype, $namespace, $resource, $exclude = null)
{
if ('\\' !== substr($namespace, -1)) {
throw new InvalidArgumentException(sprintf('Namespace prefix must end with a "\\": %s.', $namespace));
}
if (!preg_match('/^(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+\\\\)++$/', $namespace)) {
throw new InvalidArgumentException(sprintf('Namespace is not a valid PSR-4 prefix: %s.', $namespace));
}

$classes = $this->findClasses($namespace, $resource, $exclude);
// prepare for deep cloning
$prototype = serialize($prototype);
Expand Down Expand Up @@ -123,15 +116,13 @@ private function findClasses($namespace, $pattern, $excludePattern)
if (!preg_match($extRegexp, $path, $m) || !$info->isReadable()) {
continue;
}
$class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, -strlen($m[0]))), '\\');

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)) {
if (!$class = $this->getClassFromFile($path)) {
// no class found in file
continue;
}
// check to make sure the expected class exists
if (!$r = $this->container->getReflectionClass($class)) {
throw new InvalidArgumentException(sprintf('Expected to find class "%s" in file "%s" while importing services from resource "%s", but it was not found! Check the namespace prefix used with the resource.', $class, $path, $pattern));
}

$r = $this->container->getReflectionClass($class);
Copy link
Member

Choose a reason for hiding this comment

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

I think this may still fail if the parent class is missing. Can you confirm @nicolas-grekas ?

Copy link
Member

Choose a reason for hiding this comment

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

not anymore
but anyway, this implementation is going to change completly (see my other comments)


if (!$r->isInterface() && !$r->isTrait() && !$r->isAbstract()) {
$classes[] = $class;
Expand All @@ -149,4 +140,46 @@ private function findClasses($namespace, $pattern, $excludePattern)

return $classes;
}

/**
* @param string $path
*
* @return string|null
*
* @see http://jarretbyrne.com/2015/06/197/
*/
private function getClassFromFile($path)
{
$contents = file_get_contents($path);
$class = '';
$parsingNamespace = false;
$parsingClass = false;

foreach (token_get_all($contents) as $token) {
if (is_array($token) && $token[0] === T_NAMESPACE) {
$parsingNamespace = true;
}

if (is_array($token) && $token[0] === T_CLASS) {
Copy link
Member

Choose a reason for hiding this comment

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

have you testing your code on a file containing anonymous classes ?

$parsingClass = true;
}

if ($parsingNamespace) {
if (is_array($token) && in_array($token[0], array(T_STRING, T_NS_SEPARATOR), true)) {
$class .= $token[1];
}

if ($token === ';') {
Copy link
Member

Choose a reason for hiding this comment

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

what about code using the bracketed notation for namespaces ?

$parsingNamespace = false;
}
}

if ($parsingClass && is_array($token) && $token[0] === T_STRING) {
$class .= '\\'.$token[1];
$parsingClass = false;
}
}

return class_exists($class) ? $class : null;
}
}
5 changes: 5 additions & 0 deletions ...onent/DependencyInjection/Tests/Fixtures/Prototype/OtherDir/Component1/Dir1/NotAClass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

function foo() {
return 'bar';
}
7 changes: 7 additions & 0 deletions ...ponent/DependencyInjection/Tests/Fixtures/Prototype/OtherDir/Component1/Dir1/Service1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Component1\Dir1;

class Service1
{
}
8 changes: 8 additions & 0 deletions ...ponent/DependencyInjection/Tests/Fixtures/Prototype/OtherDir/Component1/Dir2/Service2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Component1\Dir2;

class Service2
{

}
8 changes: 8 additions & 0 deletions ...ponent/DependencyInjection/Tests/Fixtures/Prototype/OtherDir/Component1/Dir3/Service3.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Component1\Dir3;

class Service3
{

}
7 changes: 7 additions & 0 deletions ...ponent/DependencyInjection/Tests/Fixtures/Prototype/OtherDir/Component2/Dir1/Service4.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Component2\Dir1;

class Service4
{
}
8 changes: 8 additions & 0 deletions ...ponent/DependencyInjection/Tests/Fixtures/Prototype/OtherDir/Component2/Dir2/Service5.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Component2\Dir2;

class Service5
{

}
8 changes: 8 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_prototype_glob.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
services:
dir1:
resource: ../Prototype/OtherDir/*/Dir1
tags: [foo]

dir2:
resource: ../Prototype/OtherDir/*/Dir2
tags: [bar]
13 changes: 0 additions & 13 deletions src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,19 +113,6 @@ public function testRegisterClassesWithExclude()
$this->assertFalse($container->has(DeeperBaz::class));
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
* @expectedExceptionMessageRegExp /Expected to find class "Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\Prototype\\Bar" in file ".+" while importing services from resource "Prototype\/Sub\/\*", but it was not found\! Check the namespace prefix used with the resource/
*/
public function testRegisterClassesWithBadPrefix()
{
$container = new ContainerBuilder();
$loader = new TestFileLoader($container, new FileLocator(self::$fixturesPath.'/Fixtures'));

// the Sub is missing from namespace prefix
$loader->registerClasses(new Definition(), 'Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\\', 'Prototype/Sub/*');
}

/**
* @dataProvider getIncompatibleExcludeTests
*/
Expand Down
28 changes: 28 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,34 @@ public function testPrototype()
$this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\Bar', $resources);
}

public function testPrototypeWithGlob()
{
$container = new ContainerBuilder();
$loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
$loader->load('services_prototype_glob.yml');

$ids = array_keys($container->getDefinitions());
sort($ids);

$this->assertSame(array(
Prototype\OtherDir\Component1\Dir1\Service1::class,
Prototype\OtherDir\Component1\Dir2\Service2::class,
Prototype\OtherDir\Component2\Dir1\Service4::class,
Prototype\OtherDir\Component2\Dir2\Service5::class,
'service_container',
), $ids);

$this->assertTrue($container->getDefinition(Prototype\OtherDir\Component1\Dir1\Service1::class)->hasTag('foo'));
$this->assertTrue($container->getDefinition(Prototype\OtherDir\Component2\Dir1\Service4::class)->hasTag('foo'));
$this->assertFalse($container->getDefinition(Prototype\OtherDir\Component1\Dir1\Service1::class)->hasTag('bar'));
$this->assertFalse($container->getDefinition(Prototype\OtherDir\Component2\Dir1\Service4::class)->hasTag('bar'));

$this->assertTrue($container->getDefinition(Prototype\OtherDir\Component1\Dir2\Service2::class)->hasTag('bar'));
$this->assertTrue($container->getDefinition(Prototype\OtherDir\Component2\Dir2\Service5::class)->hasTag('bar'));
$this->assertFalse($container->getDefinition(Prototype\OtherDir\Component1\Dir2\Service2::class)->hasTag('foo'));
$this->assertFalse($container->getDefinition(Prototype\OtherDir\Component2\Dir2\Service5::class)->hasTag('foo'));
}

public function testDefaults()
{
$container = new ContainerBuilder();
Expand Down