[Data Loader] [Data Locator] [Dependency Injection] Pass root paths to FileSystemLocator during construction

Binary/Loader/FileSystemLoader.php

@@ -37,18 +37,15 @@ class FileSystemLoader implements LoaderInterface
      * @param MimeTypeGuesserInterface  $mimeGuesser
      * @param ExtensionGuesserInterface $extensionGuesser
      * @param LocatorInterface          $locator
-     * @param string[]                  $rootPaths
      */
     public function __construct(
         MimeTypeGuesserInterface $mimeGuesser,
         ExtensionGuesserInterface $extensionGuesser,
-        LocatorInterface $locator,
-        array $rootPaths = []
+        LocatorInterface $locator
     ) {
         $this->mimeTypeGuesser = $mimeGuesser;
         $this->extensionGuesser = $extensionGuesser;
         $this->locator = $locator;
-        $this->locator->setOptions(['roots' => $rootPaths]);
     }
 
     /**

Binary/Locator/FileSystemInsecureLocator.php

@@ -17,16 +17,17 @@ class FileSystemInsecureLocator extends FileSystemLocator
      * @param string $root
      * @param string $path
      *
-     * @return string|false
+     * @return string|null
      */
-    protected function generateAbsolutePath($root, $path)
+    protected function generateAbsolutePath(string $root, string $path): ?string
     {
-        if (false !== mb_strpos($path, '..'.DIRECTORY_SEPARATOR) ||
-            false !== mb_strpos($path, DIRECTORY_SEPARATOR.'..') ||
-            false === file_exists($absolute = $root.DIRECTORY_SEPARATOR.$path)) {
-            return false;
+        if (false === mb_strpos($path, '..'.DIRECTORY_SEPARATOR) &&
+            false === mb_strpos($path, DIRECTORY_SEPARATOR.'..') &&
+            false !== file_exists($absolute = $root.DIRECTORY_SEPARATOR.$path)
+        ) {
+            return $absolute;
         }
 
-        return $absolute;
+        return null;
     }
 }

Binary/Locator/FileSystemLocator.php

@@ -13,8 +13,6 @@
 
 use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException;
 use Liip\ImagineBundle\Exception\InvalidArgumentException;
-use Symfony\Component\OptionsResolver\Exception\ExceptionInterface;
-use Symfony\Component\OptionsResolver\OptionsResolver;
 
 class FileSystemLocator implements LocatorInterface
 {
@@ -24,20 +22,13 @@ class FileSystemLocator implements LocatorInterface
     private $roots = [];
 
     /**
-     * @param array[] $options
+     * @param string[] $roots
      */
-    public function setOptions(array $options = [])
+    public function __construct(array $roots = [])
     {
-        $resolver = new OptionsResolver();
-        $resolver->setDefaults(['roots' => []]);
-
-        try {
-            $options = $resolver->resolve($options);
-        } catch (ExceptionInterface $e) {
-            throw new InvalidArgumentException(sprintf('Invalid options provided to %s()', __METHOD__), null, $e);
-        }
-
-        $this->roots = array_map([$this, 'sanitizeRootPath'], (array) $options['roots']);
+        $this->roots = array_map(function (string $root): string {
+            return $this->sanitizeRootPath($root);
+        }, $roots);
     }
 
     /**
@@ -47,13 +38,13 @@ public function setOptions(array $options = [])
      *
      * @return string
      */
-    public function locate($path)
+    public function locate(string $path): string
     {
-        if (false !== $absolute = $this->locateUsingRootPlaceholder($path)) {
+        if (null !== $absolute = $this->locateUsingRootPlaceholder($path)) {
             return $this->sanitizeAbsolutePath($absolute);
         }
 
-        if (false !== $absolute = $this->locateUsingRootPathsSearch($path)) {
+        if (null !== $absolute = $this->locateUsingRootPathsSearch($path)) {
             return $this->sanitizeAbsolutePath($absolute);
         }
 
@@ -65,62 +56,66 @@ public function locate($path)
      * @param string $root
      * @param string $path
      *
-     * @return string|false
+     * @return string|null
      */
-    protected function generateAbsolutePath($root, $path)
+    protected function generateAbsolutePath(string $root, string $path): ?string
     {
-        return realpath($root.DIRECTORY_SEPARATOR.$path);
+        if (false !== $absolute = realpath($root.DIRECTORY_SEPARATOR.$path)) {
+            return $absolute;
+        }
+
+        return null;
     }
 
     /**
      * @param string $path
      *
-     * @return bool|string
+     * @return string|null
      */
-    private function locateUsingRootPathsSearch($path)
+    private function locateUsingRootPathsSearch(string $path): ?string
     {
         foreach ($this->roots as $root) {
-            if (false !== $absolute = $this->generateAbsolutePath($root, $path)) {
+            if (null !== $absolute = $this->generateAbsolutePath($root, $path)) {
                 return $absolute;
             }
         }
 
-        return false;
+        return null;
     }
 
     /**
      * @param string $path
      *
-     * @return bool|string
+     * @return string|null
      */
-    private function locateUsingRootPlaceholder($path)
+    private function locateUsingRootPlaceholder(string $path): ?string
     {
-        if (0 !== mb_strpos($path, '@') || 1 !== preg_match('{@(?<name>[^:]+):(?<path>.+)}', $path, $matches)) {
-            return false;
+        if (0 !== mb_strpos($path, '@') || 1 !== preg_match('{^@(?<name>[^:]+):(?<path>.+)$}', $path, $match)) {
+            return null;
         }
 
-        if (isset($this->roots[$matches['name']])) {
-            return $this->generateAbsolutePath($this->roots[$matches['name']], $matches['path']);
+        if (isset($this->roots[$match['name']])) {
+            return $this->generateAbsolutePath($this->roots[$match['name']], $match['path']);
         }
 
-        throw new NotLoadableException(sprintf('Invalid root placeholder "%s" for path "%s"',
-            $matches['name'], $matches['path']));
+        throw new NotLoadableException(sprintf('Invalid root placeholder "@%s" for path "%s"',
+            $match['name'], $match['path']));
     }
 
     /**
-     * @param string $root
+     * @param string $path
      *
      * @throws InvalidArgumentException
      *
      * @return string
      */
-    private function sanitizeRootPath($root)
+    private function sanitizeRootPath(string $path): string
     {
-        if (!empty($root) && false !== $realRoot = realpath($root)) {
-            return $realRoot;
+        if (!empty($path) && false !== $real = realpath($path)) {
+            return $real;
         }
 
-        throw new InvalidArgumentException(sprintf('Root image path not resolvable "%s"', $root));
+        throw new InvalidArgumentException(sprintf('Root image path not resolvable "%s"', $path));
     }
 
     /**
@@ -130,7 +125,7 @@ private function sanitizeRootPath($root)
      *
      * @return string
      */
-    private function sanitizeAbsolutePath($path)
+    private function sanitizeAbsolutePath(string $path): string
     {
         foreach ($this->roots as $root) {
             if (0 === mb_strpos($path, $root)) {

Binary/Locator/LocatorInterface.php

@@ -13,15 +13,10 @@
 
 interface LocatorInterface
 {
-    /**
-     * @param array $options[]
-     */
-    public function setOptions(array $options = []);
-
     /**
      * @param string $path
      *
      * @return string
      */
-    public function locate($path);
+    public function locate(string $path): string;
 }

DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php

@@ -14,8 +14,8 @@
 use Liip\ImagineBundle\Exception\InvalidArgumentException;
 use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
 use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
+use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
-use Symfony\Component\DependencyInjection\Reference;
 
 class FileSystemLoaderFactory extends AbstractLoaderFactory
 {
@@ -24,9 +24,11 @@ class FileSystemLoaderFactory extends AbstractLoaderFactory
      */
     public function create(ContainerBuilder $container, $loaderName, array $config)
     {
+        $locatorDefinition = new ChildDefinition(sprintf('liip_imagine.binary.locator.%s', $config['locator']));
+        $locatorDefinition->replaceArgument(0, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));
+
         $definition = $this->getChildLoaderDefinition();
-        $definition->replaceArgument(2, new Reference(sprintf('liip_imagine.binary.locator.%s', $config['locator'])));
-        $definition->replaceArgument(3, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));
+        $definition->replaceArgument(2, $locatorDefinition);
 
         return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
     }

Resources/config/imagine.xml

@@ -176,7 +176,6 @@
             <argument type="service" id="liip_imagine.mime_type_guesser" />
             <argument type="service" id="liip_imagine.extension_guesser" />
             <argument><!-- will be injected by FileSystemLoaderFactory --></argument>
-            <argument><!-- will be injected by FileSystemLoaderFactory --></argument>
         </service>
 
         <service id="liip_imagine.binary.loader.prototype.stream" class="Liip\ImagineBundle\Binary\Loader\StreamLoader">
@@ -192,10 +191,12 @@
         <!-- Data loader locators -->
 
         <service id="liip_imagine.binary.locator.filesystem" class="Liip\ImagineBundle\Binary\Locator\FileSystemLocator" public="false" shared="false">
+            <argument><!-- will be injected by FilesystemLoaderFactory --></argument>
             <tag name="liip_imagine.binary.locator" shared="false" />
         </service>
 
         <service id="liip_imagine.binary.locator.filesystem_insecure" class="Liip\ImagineBundle\Binary\Locator\FileSystemInsecureLocator" public="false" shared="false">
+            <argument><!-- will be injected by FilesystemLoaderFactory --></argument>
             <tag name="liip_imagine.binary.locator" shared="false" />
         </service>
 

Tests/Binary/Loader/FileSystemLoaderTest.php

@@ -84,7 +84,7 @@ public function testLoad($root, $path)
     }
 
     /**
-     * @return array[]
+     * @return string[][]
      */
     public static function provideMultipleRootLoadCases()
     {
@@ -102,12 +102,12 @@ public static function provideMultipleRootLoadCases()
     /**
      * @dataProvider provideMultipleRootLoadCases
      *
-     * @param string $root
-     * @param string $path
+     * @param string[] $roots
+     * @param string   $path
      */
-    public function testMultipleRootLoadCases($root, $path)
+    public function testMultipleRootLoadCases($roots, $path)
     {
-        $this->assertValidLoaderFindReturn($this->getFileSystemLoader($root)->find($path));
+        $this->assertValidLoaderFindReturn($this->getFileSystemLoader($roots)->find($path));
     }
 
     public function testAllowsEmptyRootPath()
@@ -169,11 +169,13 @@ public function testThrowsIfFileDoesNotExist()
     }
 
     /**
+     * @param string[] $roots
+     *
      * @return FileSystemLocator
      */
-    private function getFileSystemLocator()
+    private function getFileSystemLocator(array $roots)
     {
-        return new FileSystemLocator();
+        return new FileSystemLocator($roots);
     }
 
     /**
@@ -185,18 +187,17 @@ private function getDefaultDataRoots()
     }
 
     /**
-     * @param string|array|null     $root
+     * @param array                 $roots
      * @param LocatorInterface|null $locator
      *
      * @return FileSystemLoader
      */
-    private function getFileSystemLoader($root = null, LocatorInterface $locator = null)
+    private function getFileSystemLoader(array $roots = [], LocatorInterface $locator = null)
     {
         return new FileSystemLoader(
             MimeTypeGuesser::getInstance(),
             ExtensionGuesser::getInstance(),
-            null !== $locator ? $locator : $this->getFileSystemLocator(),
-            null !== $root ? $root : $this->getDefaultDataRoots()
+            null !== $locator ? $locator : $this->getFileSystemLocator(count($roots) ? $roots : $this->getDefaultDataRoots())
         );
     }
 

Tests/Binary/Locator/AbstractFileSystemLocatorTest.php

@@ -46,18 +46,10 @@ public function testThrowsIfFileDoesNotExist()
         $this->getFileSystemLocator(__DIR__)->locate('fileNotExist');
     }
 
-    public function testThrowsIfInvalidOptionProvided()
-    {
-        $this->expectException(\Liip\ImagineBundle\Exception\InvalidArgumentException::class);
-        $this->expectExceptionMessage('Invalid options provided to');
-
-        $this->getFileSystemLocator(__DIR__)->setOptions(['foo' => 'bar']);
-    }
-
     public function testThrowsIfRootPlaceholderInvalid()
     {
         $this->expectException(\Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException::class);
-        $this->expectExceptionMessage('Invalid root placeholder "invalid-placeholder" for path');
+        $this->expectExceptionMessage('Invalid root placeholder "@invalid-placeholder" for path');
 
         $this->getFileSystemLocator(__DIR__)->locate('@invalid-placeholder:file.ext');
     }

Tests/Binary/Locator/FileSystemInsecureLocatorTest.php

@@ -86,9 +86,6 @@ public static function provideMultipleRootLoadCases()
      */
     protected function getFileSystemLocator($paths)
     {
-        $locator = new FileSystemInsecureLocator();
-        $locator->setOptions(['roots' => (array) $paths]);
-
-        return $locator;
+        return new FileSystemInsecureLocator((array) $paths);
     }
 }

Tests/Binary/Locator/FileSystemLocatorTest.php

@@ -81,9 +81,6 @@ public static function provideMultipleRootLoadCases()
      */
     protected function getFileSystemLocator($paths)
     {
-        $locator = new FileSystemLocator();
-        $locator->setOptions(['roots' => (array) $paths]);
-
-        return $locator;
+        return new FileSystemLocator((array) $paths);
     }
 }