Skip to content

Commit 7767b32

Browse files
robfrawleyrobfrawley
authored
Merge pull request #875 from robfrawley/bugfix-locator-use-liip-namespace-exceptions
[Loader] [Locator] FileSystemLocator service must not be shared and tests cleanup/additions
2 parents 30aea40 + 45c057c commit 7767b32

25 files changed

+796
-164
lines changed

Diff for: ‎Binary/Locator/FileSystemLocator.php

+11-5
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,29 @@
1313

1414
use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException;
1515
use Liip\ImagineBundle\Exception\InvalidArgumentException;
16+
use Symfony\Component\OptionsResolver\Exception\ExceptionInterface;
1617
use Symfony\Component\OptionsResolver\OptionsResolver;
1718

1819
class FileSystemLocator implements LocatorInterface
1920
{
2021
/**
2122
* @var string[]
2223
*/
23-
protected $roots;
24+
private $roots;
2425

2526
/**
2627
* @param array[] $options
2728
*/
2829
public function setOptions(array $options = array())
2930
{
30-
$optionsResolver = new OptionsResolver();
31-
$optionsResolver->setDefaults(array('roots' => array()));
32-
$options = $optionsResolver->resolve($options);
31+
$resolver = new OptionsResolver();
32+
$resolver->setDefaults(array('roots' => array()));
33+
34+
try {
35+
$options = $resolver->resolve($options);
36+
} catch (ExceptionInterface $e) {
37+
throw new InvalidArgumentException(sprintf('Invalid options provided to %s()', __METHOD__), null, $e);
38+
}
3339

3440
$this->roots = array_map(array($this, 'sanitizeRootPath'), (array) $options['roots']);
3541
}
@@ -71,7 +77,7 @@ protected function generateAbsolutePath($root, $path)
7177
*
7278
* @return string
7379
*/
74-
protected function sanitizeRootPath($root)
80+
private function sanitizeRootPath($root)
7581
{
7682
if (!empty($root) && false !== $realRoot = realpath($root)) {
7783
return $realRoot;

Diff for: ‎DependencyInjection/Compiler/AbstractCompilerPass.php

+20-4
Original file line numberDiff line numberDiff line change
@@ -11,26 +11,42 @@
1111

1212
namespace Liip\ImagineBundle\DependencyInjection\Compiler;
1313

14+
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
1415
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
1516
use Symfony\Component\DependencyInjection\ContainerBuilder;
17+
use Symfony\Component\DependencyInjection\Definition;
1618

1719
abstract class AbstractCompilerPass implements CompilerPassInterface
1820
{
1921
/**
20-
* {@inheritdoc}
22+
* @param ContainerBuilder $container
23+
* @param string $message
24+
* @param mixed[] $replacements
2125
*/
2226
protected function log(ContainerBuilder $container, $message, array $replacements = array())
2327
{
2428
if (count($replacements) > 0) {
2529
$message = vsprintf($message, $replacements);
2630
}
2731

28-
if (method_exists($container, 'log')) {
32+
if (SymfonyFramework::hasDirectContainerBuilderLogging()) {
2933
$container->log($this, $message);
3034
} else {
3135
$compiler = $container->getCompiler();
32-
$formatter = $compiler->getLoggingFormatter();
33-
$compiler->addLogMessage($formatter->format($this, $message));
36+
$compiler->addLogMessage($compiler->getLoggingFormatter()->format($this, $message));
37+
}
38+
}
39+
40+
/**
41+
* @param Definition $definition
42+
* @param bool $enable
43+
*/
44+
protected function setDefinitionSharing(Definition $definition, $enable)
45+
{
46+
if (SymfonyFramework::hasDefinitionSharing()) {
47+
$definition->setShared($enable);
48+
} else {
49+
$definition->setScope($enable ? 'container' : 'prototype');
3450
}
3551
}
3652
}

Diff for: ‎DependencyInjection/Compiler/LocatorsCompilerPass.php

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the `liip/LiipImagineBundle` project.
5+
*
6+
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
7+
*
8+
* For the full copyright and license information, please view the LICENSE.md
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Liip\ImagineBundle\DependencyInjection\Compiler;
13+
14+
use Symfony\Component\DependencyInjection\ContainerBuilder;
15+
16+
class LocatorsCompilerPass extends AbstractCompilerPass
17+
{
18+
/**
19+
* {@inheritdoc}
20+
*/
21+
public function process(ContainerBuilder $container)
22+
{
23+
foreach ($container->findTaggedServiceIds('liip_imagine.binary.locator') as $id => $tags) {
24+
if (isset($tags[0]['shared'])) {
25+
$this->setDefinitionSharing($container->getDefinition($id), $tags[0]['shared']);
26+
}
27+
}
28+
}
29+
}

Diff for: ‎DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php

+19-1
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@
1111

1212
namespace Liip\ImagineBundle\DependencyInjection\Factory\Loader;
1313

14+
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
1415
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
1516
use Symfony\Component\DependencyInjection\ContainerBuilder;
17+
use Symfony\Component\DependencyInjection\ContainerInterface;
1618
use Symfony\Component\DependencyInjection\Reference;
1719

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

2931
return $this->setTaggedLoaderDefinition($loaderName, $definition, $container);
3032
}
@@ -61,4 +63,20 @@ public function addConfiguration(ArrayNodeDefinition $builder)
6163
->end()
6264
->end();
6365
}
66+
67+
/**
68+
* @param string $reference
69+
*
70+
* @return Reference
71+
*/
72+
private function createLocatorReference($reference)
73+
{
74+
$name = sprintf('liip_imagine.binary.locator.%s', $reference);
75+
76+
if (SymfonyFramework::hasDefinitionSharing()) {
77+
return new Reference($name);
78+
}
79+
80+
return new Reference($name, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, false);
81+
}
6482
}

Diff for: ‎LiipImagineBundle.php

+2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Liip\ImagineBundle\DependencyInjection\Compiler\FiltersCompilerPass;
1515
use Liip\ImagineBundle\DependencyInjection\Compiler\LoadersCompilerPass;
16+
use Liip\ImagineBundle\DependencyInjection\Compiler\LocatorsCompilerPass;
1617
use Liip\ImagineBundle\DependencyInjection\Compiler\MetadataReaderCompilerPass;
1718
use Liip\ImagineBundle\DependencyInjection\Compiler\PostProcessorsCompilerPass;
1819
use Liip\ImagineBundle\DependencyInjection\Compiler\ResolversCompilerPass;
@@ -35,6 +36,7 @@ public function build(ContainerBuilder $container)
3536
{
3637
parent::build($container);
3738

39+
$container->addCompilerPass(new LocatorsCompilerPass());
3840
$container->addCompilerPass(new LoadersCompilerPass());
3941
$container->addCompilerPass(new FiltersCompilerPass());
4042
$container->addCompilerPass(new PostProcessorsCompilerPass());

Diff for: ‎Resources/config/imagine.xml

+2
Original file line numberDiff line numberDiff line change
@@ -247,9 +247,11 @@
247247
<!-- Data loader locators -->
248248

249249
<service id="liip_imagine.binary.locator.filesystem" class="%liip_imagine.binary.locator.filesystem.class%" public="false">
250+
<tag name="liip_imagine.binary.locator" shared="false" />
250251
</service>
251252

252253
<service id="liip_imagine.binary.locator.filesystem_insecure" class="%liip_imagine.binary.locator.filesystem_insecure.class%" public="false">
254+
<tag name="liip_imagine.binary.locator" shared="false" />
253255
</service>
254256

255257
<!-- Cache resolver -->

Diff for: ‎Tests/AbstractTest.php

+16
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,22 @@ protected function createImagineMock()
110110
return $this->getMockBuilder('Imagine\Image\ImagineInterface')->getMock();
111111
}
112112

113+
/**
114+
* @param object $object
115+
* @param string $name
116+
*
117+
* @return \ReflectionMethod
118+
*/
119+
protected function getVisibilityRestrictedMethod($object, $name)
120+
{
121+
$r = new \ReflectionObject($object);
122+
123+
$m = $r->getMethod($name);
124+
$m->setAccessible(true);
125+
126+
return $m;
127+
}
128+
113129
protected function tearDown()
114130
{
115131
if (!$this->filesystem) {

Diff for: ‎Tests/Binary/Locator/AbstractFileSystemLocatorTest.php

+11
Original file line numberDiff line numberDiff line change
@@ -74,4 +74,15 @@ public function testMultipleRootLoadCases($rootDirs, $path)
7474
{
7575
$this->assertNotNull($this->getLocator($rootDirs)->locate($path));
7676
}
77+
78+
public function testThrowsExceptionOnInvalidOptions()
79+
{
80+
$this->setExpectedException(
81+
'Liip\ImagineBundle\Exception\InvalidArgumentException',
82+
'Invalid options provided to'
83+
);
84+
85+
$locator = $this->getLocator(__DIR__);
86+
$locator->setOptions(array('foo' => 'bar'));
87+
}
7788
}

Diff for: ‎Tests/DependencyInjection/Compiler/AbstractCompilerPassTest.php

+168
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the `liip/LiipImagineBundle` project.
5+
*
6+
* (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
7+
*
8+
* For the full copyright and license information, please view the LICENSE.md
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Liip\ImagineBundle\Tests\DependencyInjection\Compiler;
13+
14+
use Liip\ImagineBundle\DependencyInjection\Compiler\AbstractCompilerPass;
15+
use Liip\ImagineBundle\Utility\Framework\SymfonyFramework;
16+
use Symfony\Component\DependencyInjection\Compiler\Compiler;
17+
use Symfony\Component\DependencyInjection\ContainerBuilder;
18+
use Symfony\Component\DependencyInjection\Definition;
19+
20+
/**
21+
* @covers \Liip\ImagineBundle\DependencyInjection\Compiler\AbstractCompilerPass
22+
*/
23+
class AbstractCompilerPassTest extends AbstractCompilerPassTestCase
24+
{
25+
/**
26+
* @param string[] $methods
27+
*
28+
* @return \PHPUnit_Framework_MockObject_MockObject|AbstractCompilerPass
29+
*/
30+
private function createAbstractCompilerPassMock(array $methods = array())
31+
{
32+
return $this
33+
->getMockBuilder('\Liip\ImagineBundle\DependencyInjection\Compiler\AbstractCompilerPass')
34+
->setMethods($methods)
35+
->getMock();
36+
}
37+
38+
/**
39+
* @param string[] $methods
40+
*
41+
* @return \PHPUnit_Framework_MockObject_MockObject|ContainerBuilder
42+
*/
43+
private function createContainerBuilderMock(array $methods = array())
44+
{
45+
return $this
46+
->getMockBuilder('\Symfony\Component\DependencyInjection\ContainerBuilder')
47+
->setMethods($methods)
48+
->getMock();
49+
}
50+
51+
/**
52+
* @param string[] $methods
53+
*
54+
* @return \PHPUnit_Framework_MockObject_MockObject|Compiler
55+
*/
56+
private function createCompilerMock(array $methods = array())
57+
{
58+
return $this
59+
->getMockBuilder('\Symfony\Component\DependencyInjection\Compiler\Compiler')
60+
->setMethods($methods)
61+
->getMock();
62+
}
63+
64+
/**
65+
* @param string[] $methods
66+
*
67+
* @return \PHPUnit_Framework_MockObject_MockObject|Compiler
68+
*/
69+
private function createLoggingFormatterMock(array $methods = array())
70+
{
71+
return $this
72+
->getMockBuilder('\Symfony\Component\DependencyInjection\Compiler\LoggingFormatter')
73+
->setMethods($methods)
74+
->getMock();
75+
}
76+
77+
/**
78+
* @param string[] $methods
79+
*
80+
* @return \PHPUnit_Framework_MockObject_MockObject|Definition
81+
*/
82+
private function createDefinitionMock(array $methods = array())
83+
{
84+
return $this
85+
->getMockBuilder('\Symfony\Component\DependencyInjection\Definition')
86+
->setMethods($methods)
87+
->getMock();
88+
}
89+
90+
public function testCompilerLogging()
91+
{
92+
$pass = $this->createAbstractCompilerPassMock();
93+
94+
$message = 'Compiler log: %d %s message';
95+
$messageReplaces = array(1, 'foo-bar');
96+
$messageCompiled = vsprintf($message, $messageReplaces);
97+
98+
if (SymfonyFramework::hasDirectContainerBuilderLogging()) {
99+
$container = $this->createContainerBuilderMock();
100+
$container
101+
->expects($this->atLeastOnce())
102+
->method('log')
103+
->with($pass, $messageCompiled);
104+
} else {
105+
$container = $this->createContainerBuilderMock(array('getCompiler'));
106+
$formatter = $this->createLoggingFormatterMock(array('format'));
107+
$formatter
108+
->expects($this->atLeastOnce())
109+
->method('format')
110+
->with($pass, $messageCompiled);
111+
112+
$compiler = $this->createCompilerMock(array('addLogMessage', 'getLoggingFormatter'));
113+
$compiler
114+
->expects($this->atLeastOnce())
115+
->method('addLogMessage');
116+
$compiler
117+
->expects($this->atLeastOnce())
118+
->method('getLoggingFormatter')
119+
->willReturn($formatter);
120+
121+
$container
122+
->expects($this->atLeastOnce())
123+
->method('getCompiler')
124+
->willReturn($compiler);
125+
}
126+
127+
$log = $this->getVisibilityRestrictedMethod($pass, 'log');
128+
$log->invoke($pass, $container, $message, $messageReplaces);
129+
}
130+
131+
public function testSetDefinitionSharing()
132+
{
133+
$p = $this->createAbstractCompilerPassMock();
134+
$m = $this->getVisibilityRestrictedMethod($p, 'setDefinitionSharing');
135+
136+
if (SymfonyFramework::hasDefinitionSharing()) {
137+
$definition = $this->createDefinitionMock(array('setShared'));
138+
$definition
139+
->expects($this->atLeastOnce())
140+
->method('setShared')
141+
->with(false);
142+
} else {
143+
$definition = $this->createDefinitionMock(array('setScope'));
144+
$definition
145+
->expects($this->atLeastOnce())
146+
->method('setScope')
147+
->with('prototype');
148+
}
149+
150+
$m->invoke($p, $definition, false);
151+
152+
if (SymfonyFramework::hasDefinitionSharing()) {
153+
$definition = $this->createDefinitionMock(array('setShared'));
154+
$definition
155+
->expects($this->atLeastOnce())
156+
->method('setShared')
157+
->with(true);
158+
} else {
159+
$definition = $this->createDefinitionMock(array('setScope'));
160+
$definition
161+
->expects($this->atLeastOnce())
162+
->method('setScope')
163+
->with('container');
164+
}
165+
166+
$m->invoke($p, $definition, true);
167+
}
168+
}

0 commit comments

Comments
 (0)