[Data Loader] Pass root paths to file system locator constructor (instead of loader)

Binary/Loader/FileSystemLoader.php

@@ -38,30 +38,56 @@ class FileSystemLoader implements LoaderInterface
     /**
      * @param MimeTypeGuesserInterface  $mimeGuesser
      * @param ExtensionGuesserInterface $extensionGuesser
-     * @param string[]                  $dataRoots
+     * @param string[]                  $locatorOrDataRoots
      * @param LocatorInterface          $locator
      */
     public function __construct(
         MimeTypeGuesserInterface $mimeGuesser,
         ExtensionGuesserInterface $extensionGuesser,
-        $dataRoots
+        $locatorOrDataRoots
         /* LocatorInterface $locator */
     ) {
         $this->mimeTypeGuesser = $mimeGuesser;
         $this->extensionGuesser = $extensionGuesser;
 
-        if (count($dataRoots) === 0) {
-            throw new InvalidArgumentException('One or more data root paths must be specified.');
-        }
+        if (is_array($locatorOrDataRoots) || is_string($locatorOrDataRoots)) { // pre-1.9.0 behaviour
+            if (count((array) $locatorOrDataRoots) === 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) $locatorOrDataRoots));
+                } 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 dataroots instead is deprecated since version 1.9.0',
+                        __METHOD__
+                    ),
+                    E_USER_DEPRECATED
+                );
+
+                $this->locator = new FileSystemLocator((array) $locatorOrDataRoots);
+            }
+        } elseif ($locatorOrDataRoots instanceof LocatorInterface) {
+            $this->locator = $locatorOrDataRoots;
+        } else {
+            throw new \InvalidArgumentException(sprintf('Method %s() expects a LocatorInterface for the third argument.', __METHOD__));
+        }
     }
 
     /**

Binary/Locator/FileSystemLocator.php

@@ -23,6 +23,11 @@ class FileSystemLocator implements LocatorInterface
      */
     private $roots = array();
 
+    public function __construct(array $dataRoots = array())
+    {
+        $this->roots = array_map(array($this, 'sanitizeRootPath'), (array) $dataRoots);
+    }
+
     /**
      * @param array[] $options
      */
@@ -37,6 +42,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 dataroots to the constuctor instead.', __METHOD__),
+            E_USER_DEPRECATED
+        );
+
         $this->roots = array_map(array($this, 'sanitizeRootPath'), (array) $options['roots']);
     }
 

DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php

@@ -14,8 +14,10 @@
 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\DefinitionDecorator;
 use Symfony\Component\DependencyInjection\Reference;
 
 class FileSystemLoaderFactory extends AbstractLoaderFactory
@@ -25,9 +27,18 @@ class FileSystemLoaderFactory extends AbstractLoaderFactory
      */
     public function create(ContainerBuilder $container, $loaderName, array $config)
     {
+        $parentLocatorServiceId = sprintf('liip_imagine.binary.locator.%s', $config['locator']);
+
+        $locatorDefinition = class_exists('\Symfony\Component\DependencyInjection\ChildDefinition') ?
+            new ChildDefinition($parentLocatorServiceId) : new DefinitionDecorator($parentLocatorServiceId);
+
+        $locatorServiceId = sprintf('liip_imagine.binary.locator.%s.%s', $config['locator'], $loaderName);
+        $container->setDefinition($locatorServiceId, $locatorDefinition);
+
+        $locatorDefinition->replaceArgument(0, $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container));
+
         $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($locatorServiceId));
 
         return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
     }
@@ -163,20 +174,4 @@ private function getBundlePathsUsingNamedObj(array $classes)
 
         return $paths;
     }
-
-    /**
-     * @param string $reference
-     *
-     * @return Reference
-     */
-    private function createLocatorReference($reference)
-    {
-        $name = sprintf('liip_imagine.binary.locator.%s', $reference);
-
-        if (SymfonyFramework::hasDefinitionSharing()) {
-            return new Reference($name);
-        }
-
-        return new Reference($name, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, false);
-    }
 }

Resources/config/imagine.xml

@@ -252,10 +252,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>
 

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());
     }
 
     /**
@@ -206,9 +206,9 @@ public function testThrowsIfFileDoesNotExist()
     /**
      * @return FileSystemLocator
      */
-    private function getFileSystemLocator()
+    private function getFileSystemLocator($dataRoots)
     {
-        return new FileSystemLocator();
+        return new FileSystemLocator((array) $dataRoots);
     }
 
     /**
@@ -230,8 +230,7 @@ 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())
         );
     }
 

Tests/Binary/Locator/FileSystemInsecureLocatorTest.php

@@ -24,10 +24,7 @@ class FileSystemInsecureLocatorTest extends AbstractFileSystemLocatorTest
      */
     protected function getFileSystemLocator($paths)
     {
-        $locator = new FileSystemInsecureLocator();
-        $locator->setOptions(array('roots' => (array) $paths));
-
-        return $locator;
+        return new FileSystemInsecureLocator((array) $paths);
     }
 
     public function testLoadsOnSymbolicLinks()

Tests/Binary/Locator/FileSystemLocatorTest.php

@@ -24,10 +24,7 @@ class FileSystemLocatorTest extends AbstractFileSystemLocatorTest
      */
     protected function getFileSystemLocator($paths)
     {
-        $locator = new FileSystemLocator();
-        $locator->setOptions(array('roots' => (array) $paths));
-
-        return $locator;
+        return new FileSystemLocator((array) $paths);
     }
 
     /**

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));
     }
 
     /**

UPGRADE.md

@@ -1,5 +1,17 @@
 # Upgrade
 
+## 1.8.1
+
+ - __[Data Loader]__ The arguments for the `FileSystemLoader` class constructor have changed. Instead of passing data
+ roots as third parameter and optionally a `LocatorInterace` as fourth parameter, a `LocatorInterface`
+ should now be passed as third parameter. The data roots no longer need to be passed to the `FileSystemLoader` class
+ constructor but need to be passed to `LocatorInterface` class constructor instead.
+ Passing data roots as a third parameter to the `FileSystemLoader` class constructor is still allowed, but deprecated
+ and will be removed in `2.0`.
+ Passing both data roots as well as a `LocatorInterface` instance to the `FileSystemLoader` class constructor is still
+ supported but deprecated and will be removed in `2.0` as well. Note that when you this, any data root that was already
+ set in the `LocatorInteface` will be overwritten by de data roots passed to the `FileSystemLoader`.
+
 ## 1.8.0
 
  - __[Routing]__ The `Resources/config/routing.xml` file has been deprecated and will be removed in `2.0`. Use the new