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

[Loader] [Locator] FileSystemLocator service must not be shared #875

Merged
merged 4 commits into from
Feb 27, 2017
Merged

[Loader] [Locator] FileSystemLocator service must not be shared #875

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94eebdf
locator should throw liip namespaced exceptions
robfrawley Feb 10, 2017
0695ad0
locator services must be non-shared
robfrawley Feb 12, 2017
f8ff666
feature toggle reference creation
robfrawley Feb 13, 2017
45c057c
test cleanup/additions and locator pass refactoring
robfrawley Feb 15, 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
16 changes: 11 additions & 5 deletions Binary/Locator/FileSystemLocator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,29 @@

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
{
/**
* @var string[]
*/
protected $roots;
private $roots;

/**
* @param array[] $options
*/
public function setOptions(array $options = array())
{
$optionsResolver = new OptionsResolver();
$optionsResolver->setDefaults(array('roots' => array()));
$options = $optionsResolver->resolve($options);
$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);
}

$this->roots = array_map(array($this, 'sanitizeRootPath'), (array) $options['roots']);
}
Expand Down Expand Up @@ -71,7 +77,7 @@ protected function generateAbsolutePath($root, $path)
*
* @return string
*/
protected function sanitizeRootPath($root)
private function sanitizeRootPath($root)
{
if (!empty($root) && false !== $realRoot = realpath($root)) {
return $realRoot;
Expand Down
41 changes: 41 additions & 0 deletions DependencyInjection/Compiler/LocatorsCompilerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\DependencyInjection\Compiler;

use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;

class LocatorsCompilerPass extends AbstractCompilerPass
{
/**
* {@inheritdoc}
*/
public function process(ContainerBuilder $container)
{
foreach (array_keys($container->findTaggedServiceIds('liip_imagine.binary.locator')) as $id) {
$this->disableSharedDefinition($container->getDefinition($id));
}
}

/**
* @param Definition $definition
*/
private function disableSharedDefinition(Definition $definition)
{
if (SymfonyFramework::hasDefinitionSharedToggle()) {
$definition->setShared(false);
} else {
$definition->setScope('prototype');
}
}
}
18 changes: 17 additions & 1 deletion DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@

namespace Liip\ImagineBundle\DependencyInjection\Factory\Loader;

use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Reference;

class FileSystemLoaderFactory extends AbstractLoaderFactory
Expand All @@ -24,7 +26,7 @@ public function create(ContainerBuilder $container, $loaderName, array $config)
{
$definition = $this->getChildLoaderDefinition();
$definition->replaceArgument(2, $config['data_root']);
$definition->replaceArgument(3, new Reference(sprintf('liip_imagine.binary.locator.%s', $config['locator'])));
$definition->replaceArgument(3, $this->createLocatorReference($config['locator']));

return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
}
Expand Down Expand Up @@ -61,4 +63,18 @@ public function addConfiguration(ArrayNodeDefinition $builder)
->end()
->end();
}

/**
* @param string $reference
*
* @return Reference
*/
private function createLocatorReference($reference)
{
return new Reference(
sprintf('liip_imagine.binary.locator.%s', $reference),
ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE,
SymfonyFramework::hasDefinitionSharedToggle()
Copy link
Contributor

@sebastianblum sebastianblum Feb 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The third argument (strict) was removed in symfony 3.0.
Should we use an switch like

if (symfony 2.x) {
    return new Reference(
        sprintf('liip_imagine.binary.locator.%s', $reference),
            ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE,
            false
    );

    return new Reference(sprintf('liip_imagine.binary.locator.%s', $config['locator']);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I left it like this is because that will always be true for Symfony >=2.8, and therefore it doesn't matter that it's ignored. We only care about it when it is false on Symfony <2.8. Otherwise, the extra parameter can be, and is, safely ignored (the PHP interpreter allows and ignores any extra parameters). Do you really think it's necessary to add a switch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good morning @robfrawley, not an easy question. You are right, the code is fine.
I would prefer a feature switch at the moment and remove it in future (maybe 2.0) when at least symfony 2.8 will be supported.

Copy link
Collaborator Author

@robfrawley robfrawley Feb 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ok; ammended the code to the following:

$name = sprintf('liip_imagine.binary.locator.%s', $reference);

if (SymfonyFramework::hasDefinitionSharedToggle()) {
    return new Reference($name);
}

return new Reference($name, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, false);

I didn't use a version toggle, as I want to use the exact mechanism that requires a non-strict reference, which can be found here. Also, I want to avoid a direct dependency on the Symfony kernel in-code for now.

);
}
}
2 changes: 2 additions & 0 deletions LiipImagineBundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Liip\ImagineBundle\DependencyInjection\Compiler\FiltersCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\LoadersCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\LocatorsCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\MetadataReaderCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\PostProcessorsCompilerPass;
use Liip\ImagineBundle\DependencyInjection\Compiler\ResolversCompilerPass;
Expand All @@ -35,6 +36,7 @@ public function build(ContainerBuilder $container)
{
parent::build($container);

$container->addCompilerPass(new LocatorsCompilerPass());
$container->addCompilerPass(new LoadersCompilerPass());
$container->addCompilerPass(new FiltersCompilerPass());
$container->addCompilerPass(new PostProcessorsCompilerPass());
Expand Down
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 @@ -247,9 +247,11 @@
<!-- Data loader locators -->

<service id="liip_imagine.binary.locator.filesystem" class="%liip_imagine.binary.locator.filesystem.class%" public="false">
<tag name="liip_imagine.binary.locator" />
</service>

<service id="liip_imagine.binary.locator.filesystem_insecure" class="%liip_imagine.binary.locator.filesystem_insecure.class%" public="false">
<tag name="liip_imagine.binary.locator" />
</service>

<!-- Cache resolver -->
Expand Down
11 changes: 11 additions & 0 deletions Tests/Binary/Locator/AbstractFileSystemLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,15 @@ public function testMultipleRootLoadCases($rootDirs, $path)
{
$this->assertNotNull($this->getLocator($rootDirs)->locate($path));
}

public function testThrowsExceptionOnInvalidOptions()
{
$this->setExpectedException(
'Liip\ImagineBundle\Exception\InvalidArgumentException',
'Invalid options provided to'
);

$locator = $this->getLocator(__DIR__);
$locator->setOptions(array('foo' => 'bar'));
}
}
51 changes: 51 additions & 0 deletions Tests/DependencyInjection/Compiler/LocatorsCompilerPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

/*
* This file is part of the `liip/LiipImagineBundle` project.
*
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
*
* For the full copyright and license information, please view the LICENSE.md
* file that was distributed with this source code.
*/

namespace Liip\ImagineBundle\Tests\DependencyInjection\Compiler;

use Liip\ImagineBundle\DependencyInjection\Compiler\LocatorsCompilerPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;

/**
* @covers \Liip\ImagineBundle\DependencyInjection\Compiler\AbstractCompilerPass
* @covers \Liip\ImagineBundle\DependencyInjection\Compiler\LocatorsCompilerPass
*/
class LocatorsCompilerPassTest extends \PHPUnit_Framework_TestCase
{
public function testProcess()
{
$locatorDefinition = new Definition();
$locatorDefinition->addTag('liip_imagine.binary.locator', array(
'shared' => true,
));

$container = new ContainerBuilder();
$container->setDefinition('liip_imagine.binary.locator.foo', $locatorDefinition);

$pass = new LocatorsCompilerPass();

//guard
if (method_exists($locatorDefinition, 'isShared')) {
$this->assertTrue($locatorDefinition->isShared());
} else {
$this->assertSame('container', $locatorDefinition->getScope());
}

$pass->process($container);

if (method_exists($locatorDefinition, 'isShared')) {
$this->assertFalse($locatorDefinition->isShared());
} else {
$this->assertSame('prototype', $locatorDefinition->getScope());
}
}
}
8 changes: 4 additions & 4 deletions Tests/DependencyInjection/Factory/Resolver/AwsS3ResolverFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@
namespace Liip\ImagineBundle\Tests\DependencyInjection\Factory\Resolver;

use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\AwsS3ResolverFactory;
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\Processor;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Kernel;

/**
* @covers Liip\ImagineBundle\DependencyInjection\Factory\Resolver\AwsS3ResolverFactory<extended>
* @covers \Liip\ImagineBundle\DependencyInjection\Factory\Resolver\AwsS3ResolverFactory<extended>
*/
class AwsS3ResolverFactoryTest extends \Phpunit_Framework_TestCase
{
Expand Down Expand Up @@ -122,7 +122,7 @@ public function testCreateS3ClientDefinitionOnCreate()

public function testCreateS3ClientDefinitionWithFactoryOnCreate()
{
if (version_compare(Kernel::VERSION_ID, '20600') < 0) {
if (SymfonyFramework::isKernelLessThan(2, 6)) {
$this->markTestSkipped('No need to test on symfony < 2.6');
}

Expand All @@ -147,7 +147,7 @@ public function testCreateS3ClientDefinitionWithFactoryOnCreate()

public function testLegacyCreateS3ClientDefinitionWithFactoryOnCreate()
{
if (version_compare(Kernel::VERSION_ID, '20600') >= 0) {
if (SymfonyFramework::isKernelGreaterThanOrEqualTo(2, 6)) {
$this->markTestSkipped('No need to test on symfony >= 2.6');
}

Expand Down
42 changes: 24 additions & 18 deletions Tests/DependencyInjection/LiipImagineExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@
use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\WebPathResolverFactory;
use Liip\ImagineBundle\DependencyInjection\LiipImagineExtension;
use Liip\ImagineBundle\Tests\AbstractTest;
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\HttpKernel\Kernel;
use Symfony\Component\Yaml\Parser;

/**
* @covers Liip\ImagineBundle\DependencyInjection\Configuration
* @covers Liip\ImagineBundle\DependencyInjection\LiipImagineExtension
* @covers \Liip\ImagineBundle\DependencyInjection\Configuration
* @covers \Liip\ImagineBundle\DependencyInjection\LiipImagineExtension
*/
class LiipImagineExtensionTest extends AbstractTest
{
Expand Down Expand Up @@ -77,7 +78,7 @@ public function testCustomRouteRequirements()
*/
public function testFactoriesConfiguration($service, $factory)
{
if (version_compare(Kernel::VERSION_ID, '20600') < 0) {
if (SymfonyFramework::isKernelLessThan(2, 6)) {
$this->markTestSkipped('No need to test on symfony < 2.6');
}

Expand All @@ -88,11 +89,12 @@ public function testFactoriesConfiguration($service, $factory)
}

/**
* @legacy
* @dataProvider factoriesProvider
*/
public function testLegacyFactoriesConfiguration($service, $factory)
{
if (version_compare(Kernel::VERSION_ID, '20600') >= 0) {
if (SymfonyFramework::isKernelGreaterThanOrEqualTo(2, 6)) {
$this->markTestSkipped('No need to test on symfony >= 2.6');
}

Expand All @@ -117,9 +119,6 @@ public function factoriesProvider()
);
}

/**
* @return ContainerBuilder
*/
protected function createEmptyConfiguration()
{
$this->containerBuilder = new ContainerBuilder();
Expand All @@ -130,9 +129,6 @@ protected function createEmptyConfiguration()
$this->assertTrue($this->containerBuilder instanceof ContainerBuilder);
}

/**
* @return ContainerBuilder
*/
protected function createFullConfiguration()
{
$this->containerBuilder = new ContainerBuilder();
Expand Down Expand Up @@ -183,29 +179,39 @@ protected function getFullConfig()
return $parser->parse($yaml);
}

/**
* @param string $value
* @param string $key
*/
private function assertAlias($value, $key)
{
$this->assertEquals($value, (string) $this->containerBuilder->getAlias($key), sprintf('%s alias is correct', $key));
}

/**
* @param string $value
* @param string $key
*/
private function assertParameter($value, $key)
{
$this->assertEquals($value, $this->containerBuilder->getParameter($key), sprintf('%s parameter is correct', $key));
}

/**
* @param string $id
*/
private function assertHasDefinition($id)
{
$this->assertTrue(($this->containerBuilder->hasDefinition($id) ?: $this->containerBuilder->hasAlias($id)));
}

private function assertNotHasDefinition($id)
{
$this->assertFalse(($this->containerBuilder->hasDefinition($id) ?: $this->containerBuilder->hasAlias($id)));
}

private function assertDICConstructorArguments($definition, $args)
/**
* @param Definition $definition
* @param array $arguments
*/
private function assertDICConstructorArguments(Definition $definition, array $arguments)
{
$this->assertEquals($args, $definition->getArguments(), "Expected and actual DIC Service constructor arguments of definition '".$definition->getClass()."' don't match.");
$this->assertEquals($arguments, $definition->getArguments(), "Expected and actual DIC Service constructor arguments of definition '".$definition->getClass()."' don't match.");
}

protected function tearDown()
Expand Down
8 changes: 4 additions & 4 deletions Tests/Form/Type/ImageTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@
namespace Liip\ImagineBundle\Tests\Form\Type;

use Liip\ImagineBundle\Form\Type\ImageType;
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
use Symfony\Component\Form\FormView;
use Symfony\Component\HttpKernel\Kernel;
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
* @covers Liip\ImagineBundle\Form\Type\ImageType
* @covers \Liip\ImagineBundle\Form\Type\ImageType
*/
class ImageTypeTest extends \PHPUnit_Framework_TestCase
{
Expand All @@ -37,7 +37,7 @@ public function testGetParent()

public function testConfigureOptions()
{
if (version_compare(Kernel::VERSION_ID, '20600') < 0) {
if (SymfonyFramework::isKernelLessThan(2, 6)) {
$this->markTestSkipped('No need to test on symfony < 2.6');
}

Expand All @@ -57,7 +57,7 @@ public function testConfigureOptions()

public function testLegacySetDefaultOptions()
{
if (version_compare(Kernel::VERSION_ID, '20600') >= 0) {
if (SymfonyFramework::isKernelGreaterThanOrEqualTo(2, 6)) {
$this->markTestSkipped('No need to test on symfony >= 2.6');
}

Expand Down
Loading