Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass roots to FileSystemLocator during construction (1.0 branch) #932

Closed
wants to merge 2 commits into from
Closed

Pass roots to FileSystemLocator during construction (1.0 branch) #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
131e8c1
Pass roots to FileSystemLocator during construction
rpkamp May 13, 2017
2533081
CS fixes
rpkamp May 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions Binary/Loader/FileSystemLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

use Liip\ImagineBundle\Binary\Locator\FileSystemLocator;
use Liip\ImagineBundle\Binary\Locator\LocatorInterface;
use Liip\ImagineBundle\Exception\InvalidArgumentException;
use Liip\ImagineBundle\Model\FileBinary;
use Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesserInterface;
use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
Expand All @@ -38,30 +37,41 @@ class FileSystemLoader implements LoaderInterface
/**
* @param MimeTypeGuesserInterface $mimeGuesser
* @param ExtensionGuesserInterface $extensionGuesser
* @param string[] $dataRoots
* @param LocatorInterface $locator
* @param LocatorInterface|string[] $locatorOrDataRoots
*/
public function __construct(
MimeTypeGuesserInterface $mimeGuesser,
ExtensionGuesserInterface $extensionGuesser,
$dataRoots
/* LocatorInterface $locator */
$locatorOrDataRoots
) {
$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)) { // BC
if (func_num_args() === 4 && func_get_arg(3) instanceof LocatorInterface) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering about this one. To me this seems like a BC break, meaning this cannot be in a 1.x version, but according to semver should bump the version to 2.0.
On the other hand this is an internal class and people aren't supposed to create it themselves, so maybe it's okay.

Copy link
Collaborator

@robfrawley robfrawley May 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my take:

  • The constructor never actually had a fourth parameter in the signature (it was commented out), so those extending this class won't have their code break.
  • According to semvar we just need to increment the minor version to introduce a deprecation.
  • As long as you haven't broken any of the following constructor usage, we haven't broken any code:
    • MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoots (where $dataRoots can be a single string root or an array of roots)
    • MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoots, LocatorInterface $locator (where $dataRoots can be a single string root or an array of roots)
    • MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, LocatorInterface $locator
  • Ensure your changes to the tests didn't remove any of the aforementioned signature checks, as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree with my assertions @antoligy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoots, LocatorInterface $locator (where $dataRoots can be a single string root or an array of roots)

exactly that use case is broken in these proposed changes, because you can't pass data roots to the LocatorInterface anymore, since it's a constructor argument and cannot (and should not, imho) be changed at runtime.

Maybe it's better to just add deprecation notices to 1.0 but don't actually change anything, and then use 2.0 to actually perform the change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't even realize you'd removed setOptions from the Loader; that needs to stay with a deprecation notice. Then, in the Locator constructor, you should pass the array to setOptions, as before. As far as the other changes, of defaulting to 2.0 usage, that's fine, as long as we support the old use cases.

Copy link
Contributor Author

@rpkamp rpkamp May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as the other changes, of defaulting to 2.0 usage, that's fine, as long as we support the old use cases.

Do you mean the setOptions must stay in 2.0? If so this PR is a step backward because it only adds complexity instead of making things more simple. I was hoping for removing setOptions altogether in 2.0 since it's basically a bundle-internal class and 2.0 already indicates there will be BC breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could then change this PR to only generate E_DEPRECATED warnings in 1.0, without actually changing anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was saying to keep setOptions for 1.x with a deprecation added; it can be removed for 2.x, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I'll close this PR and create a new one with deprecation warnings only 😃

throw new \InvalidArgumentException(
sprintf(
'Passing a LocatorInterface as fourth parameter to %s() is no longer allowed. 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__
)
);
}
@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
);

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();
$this->locator = new FileSystemLocator($locatorOrDataRoots);
} elseif ($locatorOrDataRoots instanceof LocatorInterface) {
$this->locator = $locatorOrDataRoots;
} else {
throw new \InvalidArgumentException(sprintf('Method %s() expects a LocatorInterface for the third argument.', __METHOD__));
}

$this->locator->setOptions(array('roots' => (array) $dataRoots));
}

/**
Expand Down
18 changes: 4 additions & 14 deletions Binary/Locator/FileSystemLocator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -23,21 +21,13 @@ class FileSystemLocator implements LocatorInterface
*/
private $roots = array();

/**
* @param array[] $options
*/
public function setOptions(array $options = array())
public function __construct(array $roots = array())
{
$resolver = new OptionsResolver();
$resolver->setDefaults(array('roots' => array()));

try {
$options = $resolver->resolve($options);
} catch (ExceptionInterface $e) {
throw new InvalidArgumentException(sprintf('Invalid options provided to %s()', __METHOD__), null, $e);
if (count($roots) === 0) {
throw new InvalidArgumentException('One or more data root paths must be specified');
}

$this->roots = array_map(array($this, 'sanitizeRootPath'), (array) $options['roots']);
$this->roots = array_map(array($this, 'sanitizeRootPath'), $roots);
}

/**
Expand Down
5 changes: 0 additions & 5 deletions Binary/Locator/LocatorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@

interface LocatorInterface
{
/**
* @param array $options[]
*/
public function setOptions(array $options = array());

/**
* @param string $path
*
Expand Down
31 changes: 10 additions & 21 deletions DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,10 @@
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\Reference;
use Symfony\Component\DependencyInjection\DefinitionDecorator;

class FileSystemLoaderFactory extends AbstractLoaderFactory
{
Expand All @@ -25,9 +24,15 @@ class FileSystemLoaderFactory extends AbstractLoaderFactory
*/
public function create(ContainerBuilder $container, $loaderName, array $config)
{
$locatorParent = sprintf('liip_imagine.binary.locator.%s', $config['locator']);
$locatorDefinition = class_exists('\Symfony\Component\DependencyInjection\ChildDefinition') ?
new ChildDefinition($locatorParent) : new DefinitionDecorator($locatorParent);

$dataRoots = $this->resolveDataRoots($config['data_root'], $config['bundle_resources'], $container);
$locatorDefinition->replaceArgument(0, $dataRoots);

$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, $locatorDefinition);

return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
}
Expand Down Expand Up @@ -163,20 +168,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);
}
}
2 changes: 2 additions & 0 deletions Resources/config/imagine.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
82 changes: 34 additions & 48 deletions Tests/Binary/Loader/FileSystemLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Liip\ImagineBundle\Binary\Loader\FileSystemLoader;
use Liip\ImagineBundle\Binary\Locator\FileSystemLocator;
use Liip\ImagineBundle\Binary\Locator\LocatorInterface;
use Liip\ImagineBundle\Exception\InvalidArgumentException;
use Liip\ImagineBundle\Model\FileBinary;
use Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesser;
use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser;
Expand Down Expand Up @@ -83,7 +84,7 @@ public function testLoad($root, $path)
* @dataProvider provideLoadCases
*
* @group legacy
* @expectedDeprecation Method %s() will have a forth `LocatorInterface $locator` argument in version 2.0. Not defining it is deprecated since version 1.7.2
* @expectedDeprecation 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
*
* @param string $root
* @param string $path
Expand All @@ -99,6 +100,31 @@ public function testLegacyConstruction($root, $path)
$this->assertValidLoaderFindReturn($loader->find($path));
}

/**
* @expectedException InvalidArgumentException
*/
public function testInstantiationWithDataRootsAndLocatorNotAllowed()
{
new FileSystemLoader(
MimeTypeGuesser::getInstance(),
ExtensionGuesser::getInstance(),
array(__DIR__),
new FileSystemLocator(array(__DIR__.'/../'))
);
}

/**
* @expectedException InvalidArgumentException
*/
public function testInstantiationFailsWhenThirdParameterInvalidType()
{
new FileSystemLoader(
MimeTypeGuesser::getInstance(),
ExtensionGuesser::getInstance(),
'not-array-and-not-instance-of-locator'
);
}

/**
* @return array[]
*/
Expand Down Expand Up @@ -126,47 +152,6 @@ public function testMultipleRootLoadCases($root, $path)
$this->assertValidLoaderFindReturn($this->getFileSystemLoader($root)->find($path));
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessageRegExp {Method .+ expects a LocatorInterface for the forth argument}
*/
public function testThrowsIfConstructionArgumentsInvalid()
{
new FileSystemLoader(
MimeTypeGuesser::getInstance(),
ExtensionGuesser::getInstance(),
array(__DIR__),
'not-instance-of-locator'
);
}

/**
* @expectedException \Liip\ImagineBundle\Exception\InvalidArgumentException
* @expectedExceptionMessage One or more data root paths must be specified
*/
public function testThrowsIfZeroCountRootPathArray()
{
$this->getFileSystemLoader(array());
}

/**
* @expectedException \Liip\ImagineBundle\Exception\InvalidArgumentException
* @expectedExceptionMessage Root image path not resolvable
*/
public function testThrowsIfEmptyRootPath()
{
$this->getFileSystemLoader('');
}

/**
* @expectedException \Liip\ImagineBundle\Exception\InvalidArgumentException
* @expectedExceptionMessage Root image path not resolvable
*/
public function testThrowsIfRootPathDoesNotExist()
{
$this->getFileSystemLoader('/a/bad/root/path');
}

/**
* @return array[]
*/
Expand Down Expand Up @@ -204,11 +189,13 @@ public function testThrowsIfFileDoesNotExist()
}

/**
* @param string[] $roots
*
* @return FileSystemLocator
*/
private function getFileSystemLocator()
private function getFileSystemLocator($roots)
{
return new FileSystemLocator();
return new FileSystemLocator($roots);
}

/**
Expand All @@ -220,18 +207,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($roots = array(), LocatorInterface $locator = null)
{
return new FileSystemLoader(
MimeTypeGuesser::getInstance(),
ExtensionGuesser::getInstance(),
null !== $root ? $root : $this->getDefaultDataRoots(),
null !== $locator ? $locator : $this->getFileSystemLocator()
null !== $locator ? $locator : $this->getFileSystemLocator(!empty($roots) ? (array) $roots : $this->getDefaultDataRoots())
);
}

Expand Down
20 changes: 10 additions & 10 deletions Tests/Binary/Locator/AbstractFileSystemLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,16 @@ abstract protected function getFileSystemLocator($paths);

public function testImplementsLocatorInterface()
{
$this->assertInstanceOf('\Liip\ImagineBundle\Binary\Locator\LocatorInterface', new FileSystemLocator());
$this->assertInstanceOf('\Liip\ImagineBundle\Binary\Locator\LocatorInterface', new FileSystemLocator(array(__DIR__)));
}

/**
* @expectedException \Liip\ImagineBundle\Exception\InvalidArgumentException
* @expectedExceptionMessage One or more data root paths must be specified
*/
public function testRequiresAtLeastOneDataRoot()
{
$this->getFileSystemLocator(array());
}

/**
Expand Down Expand Up @@ -55,15 +64,6 @@ public function testThrowsIfFileDoesNotExist()
$this->getFileSystemLocator(__DIR__)->locate('fileNotExist');
}

/**
* @expectedException \Liip\ImagineBundle\Exception\InvalidArgumentException
* @expectedExceptionMessage Invalid options provided to
*/
public function testThrowsIfInvalidOptionProvided()
{
$this->getFileSystemLocator(__DIR__)->setOptions(array('foo' => 'bar'));
}

/**
* @expectedException \Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException
* @expectedExceptionMessage Invalid root placeholder "invalid-placeholder" for path
Expand Down
5 changes: 1 addition & 4 deletions Tests/Binary/Locator/FileSystemInsecureLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
5 changes: 1 addition & 4 deletions Tests/Binary/Locator/FileSystemLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
Loading