bug #22621 [Config] Fix resource tracking with new GlobResource (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Config] Fix resource tracking with new GlobResource

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Right now, resource tracking is done via tracking of directories mtimes, which means a container is rebuilt each time a file is either removed or added, but not when an existing file is modified.
This looks nice on the surface.
BUT.
Most code editors do create a temporary file when you open your code, thus change the parent dir mtime, thus trigger a container rebuild.
When working with PSR-4 loaders, this means each time you just open a file, most of you will trigger a container rebuild.
This is bad :(

Here is a new GlobResource to fix this issue.

Commits
-------

9190e108c1 [Config] Fix resource tracking with new GlobResource

src/Symfony/Component/Config/Exception/FileLocatorFileNotFoundException.php

@@ -18,4 +18,17 @@ namespace Symfony\Component\Config\Exception;
  */
 class FileLocatorFileNotFoundException extends \InvalidArgumentException
 {
+    private $paths;
+
+    public function __construct($message = '', $code = 0, $previous = null, array $paths = array())
+    {
+        parent::__construct($message, $code, $previous);
+
+        $this->paths = $paths;
+    }
+
+    public function getPaths()
+    {
+        return $this->paths;
+    }
 }

src/Symfony/Component/Config/FileLocator.php

@@ -43,7 +43,7 @@ class FileLocator implements FileLocatorInterface
 
         if ($this->isAbsolutePath($name)) {
             if (!file_exists($name)) {
-                throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist.', $name));
+                throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist.', $name), 0, null, array($name));
             }
 
             return $name;
@@ -56,7 +56,7 @@ class FileLocator implements FileLocatorInterface
         }
 
         $paths = array_unique($paths);
-        $filepaths = array();
+        $filepaths = $notfound = array();
 
         foreach ($paths as $path) {
             if (@file_exists($file = $path.DIRECTORY_SEPARATOR.$name)) {
@@ -64,11 +64,13 @@ class FileLocator implements FileLocatorInterface
                     return $file;
                 }
                 $filepaths[] = $file;
+            } else {
+                $notfound[] = $file;
             }
         }
 
         if (!$filepaths) {
-            throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist (in: %s).', $name, implode(', ', $paths)));
+            throw new FileLocatorFileNotFoundException(sprintf('The file "%s" does not exist (in: %s).', $name, implode(', ', $paths)), 0, null, $notfound);
         }
 
         return $filepaths;

src/Symfony/Component/Config/Loader/FileLoader.php

@@ -15,8 +15,8 @@ use Symfony\Component\Config\FileLocatorInterface;
 use Symfony\Component\Config\Exception\FileLoaderLoadException;
 use Symfony\Component\Config\Exception\FileLoaderImportCircularReferenceException;
 use Symfony\Component\Config\Exception\FileLocatorFileNotFoundException;
-use Symfony\Component\Finder\Finder;
-use Symfony\Component\Finder\Glob;
+use Symfony\Component\Config\Resource\FileExistenceResource;
+use Symfony\Component\Config\Resource\GlobResource;
 
 /**
  * FileLoader is the abstract class used by all built-in loaders that are file based.
@@ -85,9 +85,13 @@ abstract class FileLoader extends Loader
     {
         $ret = array();
         $ct = 0;
-        foreach ($this->glob($resource, false, $_, $ignoreErrors) as $resource => $info) {
-            ++$ct;
+        if (!is_string($resource) || false === strpbrk($resource, '*?{[')) {
             $ret[] = $this->doImport($resource, $type, $ignoreErrors, $sourceResource);
+        } else {
+            foreach ($this->glob($resource, false, $_, $ignoreErrors) as $path => $info) {
+                ++$ct;
+                $ret[] = $this->doImport($path, $type, $ignoreErrors, $sourceResource);
+            }
         }
 
         return $ct > 1 ? $ret : (isset($ret[0]) ? $ret[0] : null);
@@ -96,24 +100,17 @@ abstract class FileLoader extends Loader
     /**
      * @internal
      */
-    protected function glob($resource, $recursive, &$prefix = null, $ignoreErrors = false)
+    protected function glob($pattern, $recursive, &$resource = null, $ignoreErrors = false)
     {
-        if (strlen($resource) === $i = strcspn($resource, '*?{[')) {
-            if (!$recursive) {
-                $prefix = null;
-
-                yield $resource => new \SplFileInfo($resource);
-
-                return;
-            }
-            $prefix = $resource;
-            $resource = '';
+        if (strlen($pattern) === $i = strcspn($pattern, '*?{[')) {
+            $prefix = $pattern;
+            $pattern = '';
         } elseif (0 === $i) {
             $prefix = '.';
-            $resource = '/'.$resource;
+            $pattern = '/'.$pattern;
         } else {
-            $prefix = dirname(substr($resource, 0, 1 + $i));
-            $resource = substr($resource, strlen($prefix));
+            $prefix = dirname(substr($pattern, 0, 1 + $i));
+            $pattern = substr($pattern, strlen($prefix));
         }
 
         try {
@@ -123,52 +120,17 @@ abstract class FileLoader extends Loader
                 throw $e;
             }
 
-            return;
-        }
-        $prefix = realpath($prefix) ?: $prefix;
-
-        if (false === strpos($resource, '/**/') && (defined('GLOB_BRACE') || false === strpos($resource, '{'))) {
-            foreach (glob($prefix.$resource, defined('GLOB_BRACE') ? GLOB_BRACE : 0) as $path) {
-                if ($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)) {
-                    yield $path => new \SplFileInfo($path);
-                }
+            $resource = array();
+            foreach ($e->getPaths() as $path) {
+                $resource[] = new FileExistenceResource($path);
             }
 
             return;
         }
+        $resource = new GlobResource($prefix, $pattern, $recursive);
 
-        if (!class_exists(Finder::class)) {
-            throw new \LogicException(sprintf('Extended glob pattern "%s" cannot be used as the Finder component is not installed.', $resource));
-        }
-
-        $finder = new Finder();
-        $regex = Glob::toRegex($resource);
-        if ($recursive) {
-            $regex = substr_replace($regex, '(/|$)', -2, 1);
-        }
-
-        $prefixLen = strlen($prefix);
-        foreach ($finder->followLinks()->sortByName()->in($prefix) as $path => $info) {
-            if (preg_match($regex, substr($path, $prefixLen)) && $info->isFile()) {
-                yield $path => $info;
-            }
+        foreach ($resource as $path => $info) {
+            yield $path => $info;
         }
     }
 

src/Symfony/Component/Config/Resource/GlobResource.php

@@ -0,0 +1,153 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Resource;
+
+use Symfony\Component\Finder\Finder;
+use Symfony\Component\Finder\Glob;
+
+/**
+ * GlobResource represents a set of resources stored on the filesystem.
+ *
+ * Only existence/removal is tracked (not mtimes.)
+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class GlobResource implements \IteratorAggregate, SelfCheckingResourceInterface, \Serializable
+{
+    private $prefix;
+    private $pattern;
+    private $recursive;
+    private $hash;
+
+    /**
+     * Constructor.
+     *
+     * @param string $prefix    A directory prefix
+     * @param string $pattern   A glob pattern
+     * @param bool   $recursive Whether directories should be scanned recursively or not
+     *
+     * @throws \InvalidArgumentException
+     */
+    public function __construct($prefix, $pattern, $recursive)
+    {
+        $this->prefix = realpath($prefix) ?: (file_exists($prefix) ? $prefix : false);
+        $this->pattern = $pattern;
+        $this->recursive = $recursive;
+
+        if (false === $this->prefix) {
+            throw new \InvalidArgumentException(sprintf('The path "%s" does not exist.', $prefix));
+        }
+    }
+
+    public function getPrefix()
+    {
+        return $this->prefix;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function __toString()
+    {
+        return 'glob.'.$this->prefix.$this->pattern.(int) $this->recursive;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isFresh($timestamp)
+    {
+        $hash = $this->computeHash();
+
+        if (null === $this->hash) {
+            $this->hash = $hash;
+        }
+
+        return $this->hash === $hash;
+    }
+
+    public function serialize()
+    {
+        if (null === $this->hash) {
+            $this->hash = $this->computeHash();
+        }
+
+        return serialize(array($this->prefix, $this->pattern, $this->recursive, $this->hash));
+    }
+
+    public function unserialize($serialized)
+    {
+        list($this->prefix, $this->pattern, $this->recursive, $this->hash) = unserialize($serialized);
+    }
+
+    public function getIterator()
+    {
+        if (!file_exists($this->prefix) || (!$this->recursive && '' === $this->pattern)) {
+            return;
+        }
+
+        if (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)) {
+                    yield $path => new \SplFileInfo($path);
+                }
+            }
+
+            return;
+        }
+
+        if (!class_exists(Finder::class)) {
+            throw new \LogicException(sprintf('Extended glob pattern "%s" cannot be used as the Finder component is not installed.', $this->pattern));
+        }
+
+        $finder = new Finder();
+        $regex = Glob::toRegex($this->pattern);
+        if ($this->recursive) {
+            $regex = substr_replace($regex, '(/|$)', -2, 1);
+        }
+
+        $prefixLen = strlen($this->prefix);
+        foreach ($finder->followLinks()->sortByName()->in($this->prefix) as $path => $info) {
+            if (preg_match($regex, substr($path, $prefixLen)) && $info->isFile()) {
+                yield $path => $info;
+            }
+        }
+    }
+
+    private function computeHash()
+    {
+        $hash = hash_init('md5');
+
+        foreach ($this->getIterator() as $path => $info) {
+            hash_update($hash, $path."\n");
+        }
+
+        return hash_final($hash);
+    }
+}

src/Symfony/Component/Config/Tests/Resource/GlobResourceTest.php

@@ -0,0 +1,105 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Tests\Resource;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Config\Resource\GlobResource;
+
+class GlobResourceTest extends TestCase
+{
+    protected function tearDown()
+    {
+        $dir = dirname(__DIR__).'/Fixtures';
+        @rmdir($dir.'/TmpGlob');
+        @unlink($dir.'/TmpGlob');
+        @unlink($dir.'/Resource/TmpGlob');
+        touch($dir.'/Resource/.hiddenFile');
+    }
+
+    public function testIterator()
+    {
+        $dir = dirname(__DIR__).DIRECTORY_SEPARATOR.'Fixtures';
+        $resource = new GlobResource($dir, '/Resource', true);
+
+        $paths = iterator_to_array($resource);
+
+        $file = $dir.'/Resource'.DIRECTORY_SEPARATOR.'ConditionalClass.php';
+        $this->assertEquals(array($file => new \SplFileInfo($file)), $paths);
+        $this->assertInstanceOf('SplFileInfo', current($paths));
+        $this->assertSame($dir, $resource->getPrefix());
+    }
+
+    public function testIsFreshNonRecursiveDetectsNewFile()
+    {
+        $dir = dirname(__DIR__).'/Fixtures';
+        $resource = new GlobResource($dir, '/*', false);
+
+        $this->assertTrue($resource->isFresh(0));
+
+        mkdir($dir.'/TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+
+        rmdir($dir.'/TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+
+        touch($dir.'/TmpGlob');
+        $this->assertFalse($resource->isFresh(0));
+
+        unlink($dir.'/TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+    }
+
+    public function testIsFreshNonRecursiveDetectsRemovedFile()
+    {
+        $dir = dirname(__DIR__).'/Fixtures';
+        $resource = new GlobResource($dir, '/*', false);
+
+        touch($dir.'/TmpGlob');
+        touch($dir.'/.TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+
+        unlink($dir.'/.TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+
+        unlink($dir.'/TmpGlob');
+        $this->assertFalse($resource->isFresh(0));
+    }
+
+    public function testIsFreshRecursiveDetectsRemovedFile()
+    {
+        $dir = dirname(__DIR__).'/Fixtures';
+        $resource = new GlobResource($dir, '/*', true);
+
+        touch($dir.'/Resource/TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+
+        unlink($dir.'/Resource/TmpGlob');
+        $this->assertFalse($resource->isFresh(0));
+
+        touch($dir.'/Resource/TmpGlob');
+        $this->assertTrue($resource->isFresh(0));
+
+        unlink($dir.'/Resource/.hiddenFile');
+        $this->assertTrue($resource->isFresh(0));
+    }
+
+    public function testIsFreshRecursiveDetectsNewFile()
+    {
+        $dir = dirname(__DIR__).'/Fixtures';
+        $resource = new GlobResource($dir, '/*', true);
+
+        $this->assertTrue($resource->isFresh(0));
+
+        touch($dir.'/Resource/TmpGlob');
+        $this->assertFalse($resource->isFresh(0));
+    }
+}

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -33,6 +33,7 @@ use Symfony\Component\Config\Resource\ComposerResource;
 use Symfony\Component\Config\Resource\DirectoryResource;
 use Symfony\Component\Config\Resource\FileExistenceResource;
 use Symfony\Component\Config\Resource\FileResource;
+use Symfony\Component\Config\Resource\GlobResource;
 use Symfony\Component\Config\Resource\ReflectionClassResource;
 use Symfony\Component\Config\Resource\ResourceInterface;
 use Symfony\Component\DependencyInjection\LazyProxy\Instantiator\InstantiatorInterface;
@@ -249,6 +250,10 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
             return $this;
         }
 
+        if ($resource instanceof GlobResource && $this->inVendors($resource->getPrefix())) {
+            return $this;
+        }
+
         $this->resources[(string) $resource] = $resource;
 
         return $this;

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php

@@ -17,6 +17,7 @@ use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 use Symfony\Component\Config\Loader\FileLoader as BaseFileLoader;
 use Symfony\Component\Config\FileLocatorInterface;
+use Symfony\Component\Config\Resource\GlobResource;
 
 /**
  * FileLoader is the abstract class used by all built-in loaders that are file based.
@@ -83,16 +84,16 @@ abstract class FileLoader extends BaseFileLoader
         }
     }
 
-    private function findClasses($namespace, $resource)
+    private function findClasses($namespace, $pattern)
     {
         $parameterBag = $this->container->getParameterBag();
-        $resource = $parameterBag->unescapeValue($parameterBag->resolveValue($resource));
+        $pattern = $parameterBag->unescapeValue($parameterBag->resolveValue($pattern));
         $classes = array();
         $extRegexp = defined('HHVM_VERSION') ? '/\\.(?:php|hh)$/' : '/\\.php$/';
         $prefixLen = null;
-        foreach ($this->glob($resource, true, $prefix) as $path => $info) {
+        foreach ($this->glob($pattern, true, $resource) as $path => $info) {
             if (null === $prefixLen) {
-                $prefixLen = strlen($prefix);
+                $prefixLen = strlen($resource->getPrefix());
             }
 
             if (!preg_match($extRegexp, $path, $m) || !$info->isReadable()) {
@@ -105,16 +106,20 @@ abstract class FileLoader extends BaseFileLoader
             }
             // 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, $resource));
+                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));
             }
             if (!$r->isInterface() && !$r->isTrait()) {
                 $classes[] = $class;
             }
         }
 
-        if (null !== $prefix) {
-            // track directories only for new & removed files
-            $this->container->fileExists($prefix, '/^$/');
+        // track only for new & removed files
+        if ($resource instanceof GlobResource) {
+            $this->container->addResource($resource);
+        } else {
+            foreach ($resource as $path) {
+                $this->container->fileExists($path, false);
+            }
         }
 
         return $classes;

src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php

@@ -20,8 +20,8 @@ use Symfony\Component\DependencyInjection\Loader\YamlFileLoader;
 use Symfony\Component\DependencyInjection\Loader\IniFileLoader;
 use Symfony\Component\Config\Loader\LoaderResolver;
 use Symfony\Component\Config\FileLocator;
-use Symfony\Component\Config\Resource\DirectoryResource;
 use Symfony\Component\Config\Resource\FileResource;
+use Symfony\Component\Config\Resource\GlobResource;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\CaseSensitiveClass;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\NamedArgumentsDummy;
@@ -608,7 +608,7 @@ 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 DirectoryResource($fixturesDir.'Prototype', '/^$/'), $resources));
+        $this->assertTrue(false !== array_search(new GlobResource($fixturesDir.'Prototype', '/*', true), $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);

src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php

@@ -21,8 +21,8 @@ use Symfony\Component\DependencyInjection\Loader\IniFileLoader;
 use Symfony\Component\DependencyInjection\Loader\PhpFileLoader;
 use Symfony\Component\Config\Loader\LoaderResolver;
 use Symfony\Component\Config\FileLocator;
-use Symfony\Component\Config\Resource\DirectoryResource;
 use Symfony\Component\Config\Resource\FileResource;
+use Symfony\Component\Config\Resource\GlobResource;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\CaseSensitiveClass;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\NamedArgumentsDummy;
@@ -382,7 +382,7 @@ 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 DirectoryResource($fixturesDir.'Prototype', '/^$/'), $resources));
+        $this->assertTrue(false !== array_search(new GlobResource($fixturesDir.'Prototype', '', true), $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);

src/Symfony/Component/DependencyInjection/composer.json

@@ -32,7 +32,7 @@
         "symfony/proxy-manager-bridge": "Generate service proxies to lazy load them"
     },
     "conflict": {
-        "symfony/config": "<3.3",
+        "symfony/config": "<=3.3-beta1",
         "symfony/finder": "<3.3",
         "symfony/yaml": "<3.3"
     },