Merge pull request #963 from robfrawley/pr-937

[Data Loader] Pass root paths to file system locator constructor (instead of loader)
liip · Aug 31, 2017 · 8c22844 · 8c22844
2 parents 67b058c + abc60cc
commit 8c22844
Show file tree

Hide file tree

Showing 10 changed files with 137 additions and 70 deletions.
diff --git a/Binary/Loader/FileSystemLoader.php b/Binary/Loader/FileSystemLoader.php
@@ -36,31 +36,54 @@ class FileSystemLoader implements LoaderInterface
     protected $locator;
 
     /**
+     * This method will continue to support two prior, deprecated signitures for the duration of the 1.x
+     * release. The currently documented signiture will be the only valid usage once 2.0 is release. You
+     * can reference PR-963 {@see https://github.com/liip/LiipImagineBundle/pull/963} for more information.
+     *
      * @param MimeTypeGuesserInterface  $mimeGuesser
      * @param ExtensionGuesserInterface $extensionGuesser
-     * @param string[]                  $dataRoots
+     * @param LocatorInterface          
      */
-    public function __construct(
-        MimeTypeGuesserInterface $mimeGuesser,
-        ExtensionGuesserInterface $extensionGuesser,
-        $dataRoots
-        /* LocatorInterface $locator */
-    ) {
+    public function __construct(MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $locator)
+    {
         $this->mimeTypeGuesser = $mimeGuesser;
         $this->extensionGuesser = $extensionGuesser;
 
-        if (count($dataRoots) === 0) {
-            throw new InvalidArgumentException('One or more data root paths must be specified.');
-        }
+        if ($locator instanceof LocatorInterface) { // post-1.9.0 behavior
+            $this->locator = $locator;
+        } elseif (is_array($locator) || is_string($locator)) { // pre-1.9.0 behaviour
+            if (count((array) $locator) === 0) {
+                throw new InvalidArgumentException('One or more data root paths must be specified.');
+            }
 
-        if (func_num_args() >= 4 && false === ($this->locator = func_get_arg(3)) instanceof LocatorInterface) {
-            throw new \InvalidArgumentException(sprintf('Method %s() expects a LocatorInterface for the forth argument.', __METHOD__));
-        } elseif (func_num_args() < 4) {
-            @trigger_error(sprintf('Method %s() will have a forth `LocatorInterface $locator` argument in version 2.0. Not defining it is deprecated since version 1.7.2', __METHOD__), E_USER_DEPRECATED);
-            $this->locator = new FileSystemLocator();
-        }
+            if (func_num_args() >= 4) {
+                if (func_get_arg(3) instanceof LocatorInterface) {
+                    @trigger_error(sprintf(
+                        'Passing a LocatorInterface as fourth parameter to %s() is deprecated. It needs to be the '.
+                        'third parameter. The previous third parameter (data roots) is removed and the data roots must '.
+                        'now be passed as a constructor argument to the LocatorInterface passed to this method.', __METHOD__
+                    ), E_USER_DEPRECATED);
 
-        $this->locator->setOptions(array('roots' => (array) $dataRoots));
+                    $this->locator = func_get_arg(3);
+                    $this->locator->setOptions(array('roots' => (array) $locator));
+                } else {
+                    throw new \InvalidArgumentException(sprintf(
+                        'Unknown call to %s(). Please check the method signature.', __METHOD__
+                    ));
+                }
+            } else {
+                @trigger_error(sprintf(
+                    'Method %s() will expect the third parameter to be a LocatorInterface in version 2.0. Defining '.
+                    'data roots instead is deprecated since version 1.9.0', __METHOD__
+                ), E_USER_DEPRECATED);
+
+                $this->locator = new FileSystemLocator((array) $locator);
+            }
+        } else { // invalid behavior
+            throw new \InvalidArgumentException(sprintf(
+                'Method %s() expects a LocatorInterface for the third argument.', __METHOD__
+            ));
+        }
     }
 
     /**

diff --git a/Binary/Locator/FileSystemLocator.php b/Binary/Locator/FileSystemLocator.php
@@ -24,6 +24,16 @@ class FileSystemLocator implements LocatorInterface
     private $roots = array();
 
     /**
+     * @param string[] $roots
+     */
+    public function __construct(array $roots = array())
+    {
+        $this->roots = array_map(array($this, 'sanitizeRootPath'), $roots);
+    }
+
+    /**
+     * @deprecated Since version 0.9.0, use __construct(array $roots) instead
+     *
      * @param array[] $options
      */
     public function setOptions(array $options = array())
@@ -37,6 +47,11 @@ public function setOptions(array $options = array())
             throw new InvalidArgumentException(sprintf('Invalid options provided to %s()', __METHOD__), null, $e);
         }
 
+        @trigger_error(
+            sprintf('%s() is deprecated. Pass the data roots to the constructor instead.', __METHOD__),
+            E_USER_DEPRECATED
+        );
+
         $this->roots = array_map(array($this, 'sanitizeRootPath'), (array) $options['roots']);
     }
 

diff --git a/DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php b/DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php
@@ -12,10 +12,11 @@
 namespace Liip\ImagineBundle\DependencyInjection\Factory\Loader;
 
 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\ContainerInterface;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\DefinitionDecorator;
 use Symfony\Component\DependencyInjection\Reference;
 
 class FileSystemLoaderFactory extends AbstractLoaderFactory
@@ -26,8 +27,7 @@ class FileSystemLoaderFactory extends AbstractLoaderFactory
     public function create(ContainerBuilder $container, $loaderName, array $config)
     {
         $definition = $this->getChildLoaderDefinition();
-        $definition->replaceArgument(2, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));
-        $definition->replaceArgument(3, $this->createLocatorReference($config['locator']));
+        $definition->replaceArgument(2, new Reference($this->setupLocator($loaderName, $config, $container)));
 
         return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
     }
@@ -165,18 +165,34 @@ private function getBundlePathsUsingNamedObj(array $classes)
     }
 
     /**
-     * @param string $reference
+     * @param string           $loaderName
+     * @param array            $config
+     * @param ContainerBuilder $container
      *
-     * @return Reference
+     * @return string
      */
-    private function createLocatorReference($reference)
+    private function setupLocator($loaderName, array $config, ContainerBuilder $container)
     {
-        $name = sprintf('liip_imagine.binary.locator.%s', $reference);
+        $locator = $this->getLocatorDefinition($config);
+        $locator->replaceArgument(0, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));
 
-        if (SymfonyFramework::hasDefinitionSharing()) {
-            return new Reference($name);
-        }
+        $locatorId = sprintf('liip_imagine.binary.locator.%s.%s', $config['locator'], $loaderName);
+        $container->setDefinition($locatorId, $locator);
+
+        return $locatorId;
+    }
+
+    /**
+     * @param array $config
+     *
+     * @return Definition
+     */
+    private function getLocatorDefinition(array $config)
+    {
+        $key = sprintf('liip_imagine.binary.locator.%s', $config['locator']);
+        $def = class_exists('\Symfony\Component\DependencyInjection\ChildDefinition') ?
+            new ChildDefinition($key) : new DefinitionDecorator($key);
 
-        return new Reference($name, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, false);
+        return $def;
     }
 }
diff --git a/Resources/config/imagine.xml b/Resources/config/imagine.xml
@@ -264,10 +264,12 @@
         <!-- Data loader locators -->
 
         <service id="liip_imagine.binary.locator.filesystem" class="%liip_imagine.binary.locator.filesystem.class%" public="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_imagine.binary.locator.filesystem_insecure.class%" public="false">
+            <argument><!-- will be injected by FileSystemLoaderFactory --></argument>
             <tag name="liip_imagine.binary.locator" shared="false" />
         </service>
 

diff --git a/Tests/Binary/Loader/FileSystemLoaderTest.php b/Tests/Binary/Loader/FileSystemLoaderTest.php
@@ -128,7 +128,7 @@ public function testMultipleRootLoadCases($root, $path)
 
     /**
      * @expectedException \InvalidArgumentException
-     * @expectedExceptionMessageRegExp {Method .+ expects a LocatorInterface for the forth argument}
+     * @expectedExceptionMessageRegExp {Unknown call to [^(]+__construct\(\)\. Please check the method signature\.}
      */
     public function testThrowsIfConstructionArgumentsInvalid()
     {
@@ -146,7 +146,7 @@ public function testThrowsIfConstructionArgumentsInvalid()
      */
     public function testThrowsIfZeroCountRootPathArray()
     {
-        $this->getFileSystemLoader(array());
+        new FileSystemLoader(MimeTypeGuesser::getInstance(), ExtensionGuesser::getInstance(), array());
     }
 
     /**
@@ -203,14 +203,6 @@ public function testThrowsIfFileDoesNotExist()
         $this->getFileSystemLoader()->find('fileNotExist');
     }
 
-    /**
-     * @return FileSystemLocator
-     */
-    private function getFileSystemLocator()
-    {
-        return new FileSystemLocator();
-    }
-
     /**
      * @return string[]
      */
@@ -230,11 +222,20 @@ private function getFileSystemLoader($root = null, LocatorInterface $locator = n
         return new FileSystemLoader(
             MimeTypeGuesser::getInstance(),
             ExtensionGuesser::getInstance(),
-            null !== $root ? $root : $this->getDefaultDataRoots(),
-            null !== $locator ? $locator : $this->getFileSystemLocator()
+            null !== $locator ? $locator : $this->getFileSystemLocator(null !== $root ? $root : $this->getDefaultDataRoots())
         );
     }
 
+    /**
+     * @param string|string[] $roots
+     *
+     * @return FileSystemLocator
+     */
+    private function getFileSystemLocator($roots)
+    {
+        return new FileSystemLocator((array) $roots);
+    }
+
     /**
      * @param FileBinary|mixed $return
      * @param string|null      $message

diff --git a/Tests/Binary/Locator/AbstractFileSystemLocatorTest.php b/Tests/Binary/Locator/AbstractFileSystemLocatorTest.php
@@ -17,11 +17,11 @@
 abstract class AbstractFileSystemLocatorTest extends \PHPUnit_Framework_TestCase
 {
     /**
-     * @param string[]|string $paths
+     * @param string[]|string $roots
      *
      * @return LocatorInterface
      */
-    abstract protected function getFileSystemLocator($paths);
+    abstract protected function getFileSystemLocator($roots);
 
     public function testImplementsLocatorInterface()
     {

diff --git a/Tests/Binary/Locator/FileSystemInsecureLocatorTest.php b/Tests/Binary/Locator/FileSystemInsecureLocatorTest.php
@@ -19,17 +19,6 @@
  */
 class FileSystemInsecureLocatorTest extends AbstractFileSystemLocatorTest
 {
-    /**
-     * @return LocatorInterface
-     */
-    protected function getFileSystemLocator($paths)
-    {
-        $locator = new FileSystemInsecureLocator();
-        $locator->setOptions(array('roots' => (array) $paths));
-
-        return $locator;
-    }
-
     public function testLoadsOnSymbolicLinks()
     {
         $loader = $this->getFileSystemLocator($root = realpath(__DIR__.'/../../Fixtures/FileSystemLocator/root-02'));
@@ -90,4 +79,14 @@ public static function provideMultipleRootLoadCases()
             return array(array($prepend[mt_rand(0, count($prepend) - 1)], $params[0]), $params[1]);
         }, static::provideLoadCases());
     }
+
+    /**
+     * @param string|string[] $roots
+     *
+     * @return LocatorInterface
+     */
+    protected function getFileSystemLocator($roots)
+    {
+        return new FileSystemInsecureLocator((array) $roots);
+    }
 }
diff --git a/Tests/Binary/Locator/FileSystemLocatorTest.php b/Tests/Binary/Locator/FileSystemLocatorTest.php
@@ -19,17 +19,6 @@
  */
 class FileSystemLocatorTest extends AbstractFileSystemLocatorTest
 {
-    /**
-     * @return LocatorInterface
-     */
-    protected function getFileSystemLocator($paths)
-    {
-        $locator = new FileSystemLocator();
-        $locator->setOptions(array('roots' => (array) $paths));
-
-        return $locator;
-    }
-
     /**
      * @expectedException \Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException
      * @expectedExceptionMessage Source image invalid
@@ -85,4 +74,14 @@ public static function provideMultipleRootLoadCases()
             return array(array($prepend[mt_rand(0, count($prepend) - 1)], $params[0]), $params[1]);
         }, static::provideLoadCases());
     }
+
+    /**
+     * @param string|string[] $roots
+     *
+     * @return LocatorInterface
+     */
+    protected function getFileSystemLocator($roots)
+    {
+        return new FileSystemLocator((array) $roots);
+    }
 }
diff --git a/Tests/DependencyInjection/Factory/Loader/FileSystemLoaderFactoryTest.php b/Tests/DependencyInjection/Factory/Loader/FileSystemLoaderFactoryTest.php
@@ -63,7 +63,8 @@ public function testCreateLoaderDefinitionOnCreate()
         $this->assertInstanceOfChildDefinition($loaderDefinition);
         $this->assertEquals('liip_imagine.binary.loader.prototype.filesystem', $loaderDefinition->getParent());
 
-        $this->assertEquals(array('theDataRoot'), $loaderDefinition->getArgument(2));
+        $locatorReference = $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2);
+        $this->assertEquals(array('theDataRoot'), $container->getDefinition((string) $locatorReference)->getArgument(0));
     }
 
     public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingMetadata()
@@ -98,7 +99,8 @@ public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingMetadat
             'LiipBarBundle' => $barBundleRootPath.'/Resources/public',
         );
 
-        $this->assertEquals($expected, $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2));
+        $locatorReference = $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2);
+        $this->assertEquals($expected, $container->getDefinition((string) $locatorReference)->getArgument(0));
     }
 
     public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingMetadataAndBlacklisting()
@@ -134,7 +136,8 @@ public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingMetadat
             'LiipBarBundle' => $barBundleRootPath.'/Resources/public',
         );
 
-        $this->assertEquals($expected, $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2));
+        $locatorReference = $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2);
+        $this->assertEquals($expected, $container->getDefinition((string) $locatorReference)->getArgument(0));
     }
 
     public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingMetadataAndWhitelisting()
@@ -170,7 +173,8 @@ public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingMetadat
             'LiipFooBundle' => $fooBundleRootPath.'/Resources/public',
         );
 
-        $this->assertEquals($expected, $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2));
+        $locatorReference = $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2);
+        $this->assertEquals($expected, $container->getDefinition((string) $locatorReference)->getArgument(0));
     }
 
     public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingNamedObj()
@@ -201,7 +205,8 @@ public function testCreateLoaderDefinitionOnCreateWithBundlesEnabledUsingNamedOb
             'LiipBarBundle' => $barBundleRootPath.'/Resources/public',
         );
 
-        $this->assertEquals($expected, $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2));
+        $locatorReference = $container->getDefinition('liip_imagine.binary.loader.the_loader_name')->getArgument(2);
+        $this->assertEquals($expected, $container->getDefinition((string) $locatorReference)->getArgument(0));
     }
 
     /**

diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,12 @@
 # Upgrade
 
+## 1.9.0
+
+ - __[Data Loader]__ The arguments for the `FileSystemLoader` class constructor have changed. Passing an array of roots
+ as the third parameter and an (optional) `LocatorInterace` as the fourth parameter is deprecated. A `LocatorInterface`
+ should now be passed as third parameter, and the array of data roots to the `LocatorInterface::__construct()` method
+ directly. All prior signatures will continue to work until `2.0` is release.
+
 ## 1.8.0
 
  - __[Routing]__ The `Resources/config/routing.xml` file has been deprecated and will be removed in `2.0`. Use the new