From fa731e53e998ff45f3b72e33568b6784abbb1d17 Mon Sep 17 00:00:00 2001 From: Gonzalo Vilaseca Date: Sat, 29 Sep 2018 15:01:30 +0100 Subject: [PATCH] [Config] Fix slow service discovery for large excluded directories --- .../Component/Config/Loader/FileLoader.php | 4 +- .../Config/Resource/GlobResource.php | 53 ++++++++++++------- .../Tests/Fixtures/Exclude/AnExcludedFile.txt | 0 .../Exclude/ExcludeToo/AnotheExcludedFile.txt | 0 .../Tests/Resource/GlobResourceTest.php | 40 ++++++++++++++ .../DependencyInjection/Loader/FileLoader.php | 4 +- .../Tests/Loader/XmlFileLoaderTest.php | 28 +++++++++- .../Tests/Loader/YamlFileLoaderTest.php | 13 ++++- 8 files changed, 115 insertions(+), 27 deletions(-) create mode 100644 src/Symfony/Component/Config/Tests/Fixtures/Exclude/AnExcludedFile.txt create mode 100644 src/Symfony/Component/Config/Tests/Fixtures/Exclude/ExcludeToo/AnotheExcludedFile.txt diff --git a/src/Symfony/Component/Config/Loader/FileLoader.php b/src/Symfony/Component/Config/Loader/FileLoader.php index ae652da3b9..f38842ff39 100644 --- a/src/Symfony/Component/Config/Loader/FileLoader.php +++ b/src/Symfony/Component/Config/Loader/FileLoader.php @@ -93,7 +93,7 @@ abstract class FileLoader extends Loader /** * @internal */ - protected function glob(string $pattern, bool $recursive, &$resource = null, bool $ignoreErrors = false) + protected function glob(string $pattern, bool $recursive, &$resource = null, bool $ignoreErrors = false, bool $forExclusion = false, array $excluded = array()) { if (\strlen($pattern) === $i = strcspn($pattern, '*?{[')) { $prefix = $pattern; @@ -120,7 +120,7 @@ abstract class FileLoader extends Loader return; } - $resource = new GlobResource($prefix, $pattern, $recursive); + $resource = new GlobResource($prefix, $pattern, $recursive, $forExclusion, $excluded); foreach ($resource as $path => $info) { yield $path => $info; diff --git a/src/Symfony/Component/Config/Resource/GlobResource.php b/src/Symfony/Component/Config/Resource/GlobResource.php index a04cbf7be8..55dbf7b5ac 100644 --- a/src/Symfony/Component/Config/Resource/GlobResource.php +++ b/src/Symfony/Component/Config/Resource/GlobResource.php @@ -27,6 +27,8 @@ class GlobResource implements \IteratorAggregate, SelfCheckingResourceInterface, private $pattern; private $recursive; private $hash; + private $forExclusion; + private $excludedPrefixes; /** * @param string $prefix A directory prefix @@ -35,11 +37,13 @@ class GlobResource implements \IteratorAggregate, SelfCheckingResourceInterface, * * @throws \InvalidArgumentException */ - public function __construct(?string $prefix, string $pattern, bool $recursive) + public function __construct(?string $prefix, string $pattern, bool $recursive, bool $forExclusion = false, array $excludedPrefixes = array()) { $this->prefix = realpath($prefix) ?: (file_exists($prefix) ? $prefix : false); $this->pattern = $pattern; $this->recursive = $recursive; + $this->forExclusion = $forExclusion; + $this->excludedPrefixes = $excludedPrefixes; if (false === $this->prefix) { throw new \InvalidArgumentException(sprintf('The path "%s" does not exist.', $prefix)); @@ -95,26 +99,35 @@ class GlobResource implements \IteratorAggregate, SelfCheckingResourceInterface, if (0 !== strpos($this->prefix, 'phar://') && false === strpos($this->pattern, '/**/') && (\defined('GLOB_BRACE') || false === strpos($this->pattern, '{'))) { foreach (glob($this->prefix.$this->pattern, \defined('GLOB_BRACE') ? GLOB_BRACE : 0) as $path) { - if ($this->recursive && is_dir($path)) { - $files = iterator_to_array(new \RecursiveIteratorIterator( - new \RecursiveCallbackFilterIterator( - new \RecursiveDirectoryIterator($path, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS), - function (\SplFileInfo $file) { return '.' !== $file->getBasename()[0]; } - ), - \RecursiveIteratorIterator::LEAVES_ONLY - )); - uasort($files, function (\SplFileInfo $a, \SplFileInfo $b) { - return (string) $a > (string) $b ? 1 : -1; - }); - - foreach ($files as $path => $info) { - if ($info->isFile()) { - yield $path => $info; - } - } - } elseif (is_file($path)) { + if (is_file($path)) { yield $path => new \SplFileInfo($path); } + if (!is_dir($path)) { + continue; + } + if ($this->forExclusion) { + yield $path => new \SplFileInfo($path); + continue; + } + if (!$this->recursive || isset($this->excludedPrefixes[str_replace('\\', '/', $path)])) { + continue; + } + $files = iterator_to_array(new \RecursiveIteratorIterator( + new \RecursiveCallbackFilterIterator( + new \RecursiveDirectoryIterator($path, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS), + function (\SplFileInfo $file, $path) { + return !isset($this->excludedPrefixes[str_replace('\\', '/', $path)]) && '.' !== $file->getBasename()[0]; + } + ), + \RecursiveIteratorIterator::LEAVES_ONLY + )); + uasort($files, 'strnatcmp'); + + foreach ($files as $path => $info) { + if ($info->isFile()) { + yield $path => $info; + } + } } return; @@ -132,7 +145,7 @@ class GlobResource implements \IteratorAggregate, SelfCheckingResourceInterface, $prefixLen = \strlen($this->prefix); foreach ($finder->followLinks()->sortByName()->in($this->prefix) as $path => $info) { - if (preg_match($regex, substr('\\' === \DIRECTORY_SEPARATOR ? str_replace('\\', '/', $path) : $path, $prefixLen)) && $info->isFile()) { + if (preg_match($regex, substr(str_replace('\\', '/', $path), $prefixLen)) && $info->isFile()) { yield $path => $info; } } diff --git a/src/Symfony/Component/Config/Tests/Fixtures/Exclude/AnExcludedFile.txt b/src/Symfony/Component/Config/Tests/Fixtures/Exclude/AnExcludedFile.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/Symfony/Component/Config/Tests/Fixtures/Exclude/ExcludeToo/AnotheExcludedFile.txt b/src/Symfony/Component/Config/Tests/Fixtures/Exclude/ExcludeToo/AnotheExcludedFile.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/Symfony/Component/Config/Tests/Resource/GlobResourceTest.php b/src/Symfony/Component/Config/Tests/Resource/GlobResourceTest.php index c66770c305..ac7ae5bd75 100644 --- a/src/Symfony/Component/Config/Tests/Resource/GlobResourceTest.php +++ b/src/Symfony/Component/Config/Tests/Resource/GlobResourceTest.php @@ -47,6 +47,46 @@ class GlobResourceTest extends TestCase $this->assertSame($dir, $resource->getPrefix()); } + public function testIteratorForExclusionDoesntIterateThroughSubfolders() + { + $dir = \dirname(__DIR__).\DIRECTORY_SEPARATOR.'Fixtures'; + $resource = new GlobResource($dir, \DIRECTORY_SEPARATOR.'Exclude', true, true); + + $paths = iterator_to_array($resource); + + $file = $dir.\DIRECTORY_SEPARATOR.'Exclude'; + $this->assertArrayHasKey($file, $paths); + $this->assertCount(1, $paths); + } + + public function testIteratorSkipsFoldersForGivenExcludedPrefixes() + { + $dir = \dirname(__DIR__).\DIRECTORY_SEPARATOR.'Fixtures'; + $resource = new GlobResource($dir, '/*Exclude*', true, false, array($dir.\DIRECTORY_SEPARATOR.'Exclude' => true)); + + $paths = iterator_to_array($resource); + + $file = $dir.\DIRECTORY_SEPARATOR.'Exclude'.\DIRECTORY_SEPARATOR.'AnExcludedFile.txt'; + $this->assertArrayNotHasKey($file, $paths); + + $file = $dir.\DIRECTORY_SEPARATOR.'Exclude'.\DIRECTORY_SEPARATOR.'ExcludeToo'.\DIRECTORY_SEPARATOR.'AnotheExcludedFile.txt'; + $this->assertArrayNotHasKey($file, $paths); + } + + public function testIteratorSkipsFoldersWithForwardSlashForGivenExcludedPrefixes() + { + $dir = \dirname(__DIR__).\DIRECTORY_SEPARATOR.'Fixtures'; + $resource = new GlobResource($dir, '/*Exclude*', true, false, array($dir.'/Exclude' => true)); + + $paths = iterator_to_array($resource); + + $file = $dir.\DIRECTORY_SEPARATOR.'Exclude/AnExcludedFile.txt'; + $this->assertArrayNotHasKey($file, $paths); + + $file = $dir.\DIRECTORY_SEPARATOR.'Exclude'.\DIRECTORY_SEPARATOR.'ExcludeToo'.\DIRECTORY_SEPARATOR.'AnotheExcludedFile.txt'; + $this->assertArrayNotHasKey($file, $paths); + } + public function testIsFreshNonRecursiveDetectsNewFile() { $dir = \dirname(__DIR__).'/Fixtures'; diff --git a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php index b376a93873..f1186214d5 100644 --- a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php @@ -109,7 +109,7 @@ abstract class FileLoader extends BaseFileLoader $excludePrefix = null; $excludePatterns = $parameterBag->unescapeValue($parameterBag->resolveValue($excludePatterns)); foreach ($excludePatterns as $excludePattern) { - foreach ($this->glob($excludePattern, true, $resource) as $path => $info) { + foreach ($this->glob($excludePattern, true, $resource, false, true) as $path => $info) { if (null === $excludePrefix) { $excludePrefix = $resource->getPrefix(); } @@ -123,7 +123,7 @@ abstract class FileLoader extends BaseFileLoader $classes = array(); $extRegexp = '/\\.php$/'; $prefixLen = null; - foreach ($this->glob($pattern, true, $resource) as $path => $info) { + foreach ($this->glob($pattern, true, $resource, false, false, $excludePaths) as $path => $info) { if (null === $prefixLen) { $prefixLen = \strlen($resource->getPrefix()); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php index 954e7670f5..673348caae 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php @@ -611,7 +611,19 @@ class XmlFileLoaderTest extends TestCase $fixturesDir = \dirname(__DIR__).\DIRECTORY_SEPARATOR.'Fixtures'.\DIRECTORY_SEPARATOR; $this->assertTrue(false !== array_search(new FileResource($fixturesDir.'xml'.\DIRECTORY_SEPARATOR.'services_prototype.xml'), $resources)); - $this->assertTrue(false !== array_search(new GlobResource($fixturesDir.'Prototype', '/*', true), $resources)); + + $prototypeRealPath = \realpath(__DIR__.\DIRECTORY_SEPARATOR.'..'.\DIRECTORY_SEPARATOR.'Fixtures'.\DIRECTORY_SEPARATOR.'Prototype'); + $globResource = new GlobResource( + $fixturesDir.'Prototype', + '/*', + true, + false, + array( + str_replace(\DIRECTORY_SEPARATOR, '/', $prototypeRealPath.\DIRECTORY_SEPARATOR.'OtherDir') => true, + str_replace(\DIRECTORY_SEPARATOR, '/', $prototypeRealPath.\DIRECTORY_SEPARATOR.'BadClasses') => true, + ) + ); + $this->assertTrue(false !== array_search($globResource, $resources)); $resources = array_map('strval', $resources); $this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo', $resources); $this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\Bar', $resources); @@ -631,7 +643,19 @@ class XmlFileLoaderTest extends TestCase $fixturesDir = \dirname(__DIR__).\DIRECTORY_SEPARATOR.'Fixtures'.\DIRECTORY_SEPARATOR; $this->assertTrue(false !== array_search(new FileResource($fixturesDir.'xml'.\DIRECTORY_SEPARATOR.'services_prototype_array.xml'), $resources)); - $this->assertTrue(false !== array_search(new GlobResource($fixturesDir.'Prototype', '/*', true), $resources)); + + $prototypeRealPath = realpath(__DIR__.\DIRECTORY_SEPARATOR.'..'.\DIRECTORY_SEPARATOR.'Fixtures'.\DIRECTORY_SEPARATOR.'Prototype'); + $globResource = new GlobResource( + $fixturesDir.'Prototype', + '/*', + true, + false, + array( + str_replace(\DIRECTORY_SEPARATOR, '/', $prototypeRealPath.\DIRECTORY_SEPARATOR.'BadClasses') => true, + str_replace(\DIRECTORY_SEPARATOR, '/', $prototypeRealPath.\DIRECTORY_SEPARATOR.'OtherDir') => true, + ) + ); + $this->assertTrue(false !== array_search($globResource, $resources)); $resources = array_map('strval', $resources); $this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo', $resources); $this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\Bar', $resources); diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php index 12f57bbdc0..1c6b73545b 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php @@ -365,7 +365,18 @@ class YamlFileLoaderTest extends TestCase $fixturesDir = \dirname(__DIR__).\DIRECTORY_SEPARATOR.'Fixtures'.\DIRECTORY_SEPARATOR; $this->assertTrue(false !== array_search(new FileResource($fixturesDir.'yaml'.\DIRECTORY_SEPARATOR.'services_prototype.yml'), $resources)); - $this->assertTrue(false !== array_search(new GlobResource($fixturesDir.'Prototype', '', true), $resources)); + + $prototypeRealPath = \realpath(__DIR__.\DIRECTORY_SEPARATOR.'..'.\DIRECTORY_SEPARATOR.'Fixtures'.\DIRECTORY_SEPARATOR.'Prototype'); + $globResource = new GlobResource( + $fixturesDir.'Prototype', + '', + true, + false, array( + str_replace(\DIRECTORY_SEPARATOR, '/', $prototypeRealPath.\DIRECTORY_SEPARATOR.'BadClasses') => true, + str_replace(\DIRECTORY_SEPARATOR, '/', $prototypeRealPath.\DIRECTORY_SEPARATOR.'OtherDir') => true, + ) + ); + $this->assertTrue(false !== array_search($globResource, $resources)); $resources = array_map('strval', $resources); $this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo', $resources); $this->assertContains('reflection.Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\Bar', $resources);