From d896c23b24c9c91a126a7fa99bf2cf0d71a108fc Mon Sep 17 00:00:00 2001 From: Kotlyar Maksim Date: Mon, 7 Apr 2014 09:38:39 +0000 Subject: [PATCH] [config] Fix default loader not found bug. --- .../Compiler/FiltersCompilerPass.php | 26 ++++ .../Compiler/LoadersCompilerPass.php | 23 +--- .../Compiler/ResolversCompilerPass.php | 26 ++++ DependencyInjection/Configuration.php | 60 +++++---- .../Loader/FileSystemLoaderFactory.php | 1 - .../Factory/Loader/StreamLoaderFactory.php | 1 - .../Factory/Resolver/AwsS3ResolverFactory.php | 1 - .../Resolver/WebPathResolverFactory.php | 1 - Imagine/Cache/CacheManager.php | 7 +- Imagine/Data/DataManager.php | 11 +- LiipImagineBundle.php | 6 +- .../DependencyInjection/ConfigurationTest.php | 114 ++++++++++++++++-- .../LiipImagineExtensionTest.php | 6 + Tests/Imagine/Cache/CacheManagerTest.php | 4 +- Tests/Imagine/Data/DataManagerTest.php | 2 +- Tests/LiipImagineBundleTest.php | 42 ++++++- 16 files changed, 260 insertions(+), 71 deletions(-) create mode 100644 DependencyInjection/Compiler/FiltersCompilerPass.php create mode 100644 DependencyInjection/Compiler/ResolversCompilerPass.php diff --git a/DependencyInjection/Compiler/FiltersCompilerPass.php b/DependencyInjection/Compiler/FiltersCompilerPass.php new file mode 100644 index 000000000..e9f587355 --- /dev/null +++ b/DependencyInjection/Compiler/FiltersCompilerPass.php @@ -0,0 +1,26 @@ +findTaggedServiceIds('liip_imagine.filter.loader'); + + if (count($tags) > 0 && $container->hasDefinition('liip_imagine.filter.manager')) { + $manager = $container->getDefinition('liip_imagine.filter.manager'); + + foreach ($tags as $id => $tag) { + $manager->addMethodCall('addLoader', array($tag[0]['loader'], new Reference($id))); + } + } + } +} diff --git a/DependencyInjection/Compiler/LoadersCompilerPass.php b/DependencyInjection/Compiler/LoadersCompilerPass.php index 87ca4ab6b..84877db96 100644 --- a/DependencyInjection/Compiler/LoadersCompilerPass.php +++ b/DependencyInjection/Compiler/LoadersCompilerPass.php @@ -8,18 +8,11 @@ class LoadersCompilerPass implements CompilerPassInterface { + /** + * {@inheritDoc} + */ public function process(ContainerBuilder $container) { - $tags = $container->findTaggedServiceIds('liip_imagine.filter.loader'); - - if (count($tags) > 0 && $container->hasDefinition('liip_imagine.filter.manager')) { - $manager = $container->getDefinition('liip_imagine.filter.manager'); - - foreach ($tags as $id => $tag) { - $manager->addMethodCall('addLoader', array($tag[0]['loader'], new Reference($id))); - } - } - $tags = $container->findTaggedServiceIds('liip_imagine.binary.loader'); if (count($tags) > 0 && $container->hasDefinition('liip_imagine.data.manager')) { @@ -29,15 +22,5 @@ public function process(ContainerBuilder $container) $manager->addMethodCall('addLoader', array($tag[0]['loader'], new Reference($id))); } } - - $tags = $container->findTaggedServiceIds('liip_imagine.cache.resolver'); - - if (count($tags) > 0 && $container->hasDefinition('liip_imagine.cache.manager')) { - $manager = $container->getDefinition('liip_imagine.cache.manager'); - - foreach ($tags as $id => $tag) { - $manager->addMethodCall('addResolver', array($tag[0]['resolver'], new Reference($id))); - } - } } } diff --git a/DependencyInjection/Compiler/ResolversCompilerPass.php b/DependencyInjection/Compiler/ResolversCompilerPass.php new file mode 100644 index 000000000..efb222153 --- /dev/null +++ b/DependencyInjection/Compiler/ResolversCompilerPass.php @@ -0,0 +1,26 @@ +findTaggedServiceIds('liip_imagine.cache.resolver'); + + if (count($tags) > 0 && $container->hasDefinition('liip_imagine.cache.manager')) { + $manager = $container->getDefinition('liip_imagine.cache.manager'); + + foreach ($tags as $id => $tag) { + $manager->addMethodCall('addResolver', array($tag[0]['resolver'], new Reference($id))); + } + } + } +} diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index c73fb5626..14ba08dbe 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -41,43 +41,51 @@ public function getConfigTreeBuilder() $resolversPrototypeNode = $rootNode ->children() ->arrayNode('resolvers') - ->beforeNormalization() - ->ifTrue(function ($v) { return !is_array($v) || (is_array($v) && !array_key_exists('default', $v)); }) - ->then(function ($v) { - if (false == is_array($v)) { - $v = array(); - } - - $v['default'] = array('web_path' => null); - - return $v; - }) - ->end() - ->useAttributeAsKey('name') - ->prototype('array') + ->useAttributeAsKey('name') + ->prototype('array') ; $this->addResolversSections($resolversPrototypeNode); $loadersPrototypeNode = $rootNode ->children() ->arrayNode('loaders') - ->beforeNormalization() - ->ifTrue(function ($v) { return !is_array($v) || (is_array($v) && !array_key_exists('default', $v)); }) - ->then(function ($v) { - if (false == is_array($v)) { - $v = array(); - } - - $v['default'] = array('filesystem' => null); - - return $v; - }) - ->end() ->useAttributeAsKey('name') ->prototype('array') ; $this->addLoadersSections($loadersPrototypeNode); + $rootNode + ->beforeNormalization() + ->ifTrue(function ($v) { + return + empty($v['loaders']) || + !array_key_exists('default', $v['loaders']) || + empty($v['resolvers']) || + !array_key_exists('default', $v['resolvers']) + ; + }) + ->then(function ($v) { + if (false == array_key_exists('loaders', $v)) { + $v['loaders'] = array(); + } + + if (false == array_key_exists('default', $v['loaders'])) { + $v['loaders']['default'] = array('filesystem' => null); + } + + if (false == array_key_exists('resolvers', $v)) { + $v['resolvers'] = array(); + } + + if (false == array_key_exists('default', $v['resolvers'])) { + $v['resolvers']['default'] = array('web_path' => null); + } + + return $v; + }) + ->end() + ; + $rootNode ->fixXmlConfig('filter_set', 'filter_sets') ->children() diff --git a/DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php b/DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php index 43532c865..67da7d24b 100644 --- a/DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php +++ b/DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php @@ -38,7 +38,6 @@ public function getName() public function addConfiguration(ArrayNodeDefinition $builder) { $builder - ->addDefaultsIfNotSet() ->children() ->scalarNode('data_root')->defaultValue('%kernel.root_dir%/../web')->cannotBeEmpty()->end() ->end() diff --git a/DependencyInjection/Factory/Loader/StreamLoaderFactory.php b/DependencyInjection/Factory/Loader/StreamLoaderFactory.php index 46ce26c2b..8bc4cb602 100644 --- a/DependencyInjection/Factory/Loader/StreamLoaderFactory.php +++ b/DependencyInjection/Factory/Loader/StreamLoaderFactory.php @@ -39,7 +39,6 @@ public function getName() public function addConfiguration(ArrayNodeDefinition $builder) { $builder - ->addDefaultsIfNotSet() ->children() ->scalarNode('wrapper')->isRequired()->cannotBeEmpty()->end() ->scalarNode('context')->defaultValue(null)->end() diff --git a/DependencyInjection/Factory/Resolver/AwsS3ResolverFactory.php b/DependencyInjection/Factory/Resolver/AwsS3ResolverFactory.php index e50139603..332daab68 100644 --- a/DependencyInjection/Factory/Resolver/AwsS3ResolverFactory.php +++ b/DependencyInjection/Factory/Resolver/AwsS3ResolverFactory.php @@ -67,7 +67,6 @@ public function getName() public function addConfiguration(ArrayNodeDefinition $builder) { $builder - ->addDefaultsIfNotSet() ->children() ->scalarNode('bucket')->isRequired()->cannotBeEmpty()->end() ->scalarNode('cache')->defaultValue(false)->end() diff --git a/DependencyInjection/Factory/Resolver/WebPathResolverFactory.php b/DependencyInjection/Factory/Resolver/WebPathResolverFactory.php index 899f43191..a50028ab2 100644 --- a/DependencyInjection/Factory/Resolver/WebPathResolverFactory.php +++ b/DependencyInjection/Factory/Resolver/WebPathResolverFactory.php @@ -40,7 +40,6 @@ public function getName() public function addConfiguration(ArrayNodeDefinition $builder) { $builder - ->addDefaultsIfNotSet() ->children() ->scalarNode('web_root')->defaultValue('%kernel.root_dir%/../web')->cannotBeEmpty()->end() ->scalarNode('cache_prefix')->defaultValue('media/cache')->cannotBeEmpty()->end() diff --git a/Imagine/Cache/CacheManager.php b/Imagine/Cache/CacheManager.php index ae634e17f..e9c6f6810 100644 --- a/Imagine/Cache/CacheManager.php +++ b/Imagine/Cache/CacheManager.php @@ -84,12 +84,13 @@ protected function getResolver($filter) { $config = $this->filterConfig->get($filter); - $resolverName = empty($config['cache']) - ? $this->defaultResolver : $config['cache']; + $resolverName = empty($config['cache']) ? $this->defaultResolver : $config['cache']; if (!isset($this->resolvers[$resolverName])) { throw new \OutOfBoundsException(sprintf( - 'Could not find resolver for "%s" filter type', $filter + 'Could not find resolver "%s" for "%s" filter type', + $resolverName, + $filter )); } diff --git a/Imagine/Data/DataManager.php b/Imagine/Data/DataManager.php index 6c90430df..a929183ee 100644 --- a/Imagine/Data/DataManager.php +++ b/Imagine/Data/DataManager.php @@ -72,20 +72,21 @@ public function addLoader($filter, LoaderInterface $loader) * * @param string $filter * - * @return LoaderInterface - * * @throws \InvalidArgumentException + * + * @return LoaderInterface */ public function getLoader($filter) { $config = $this->filterConfig->get($filter); - $loaderName = empty($config['data_loader']) - ? $this->defaultLoader : $config['data_loader']; + $loaderName = empty($config['data_loader']) ? $this->defaultLoader : $config['data_loader']; if (!isset($this->loaders[$loaderName])) { throw new \InvalidArgumentException(sprintf( - 'Could not find data loader for "%s" filter type', $filter + 'Could not find data loader "%s" for "%s" filter type', + $loaderName, + $filter )); } diff --git a/LiipImagineBundle.php b/LiipImagineBundle.php index c153c7d94..0d0ec3424 100644 --- a/LiipImagineBundle.php +++ b/LiipImagineBundle.php @@ -2,7 +2,9 @@ namespace Liip\ImagineBundle; +use Liip\ImagineBundle\DependencyInjection\Compiler\FiltersCompilerPass; use Liip\ImagineBundle\DependencyInjection\Compiler\LoadersCompilerPass; +use Liip\ImagineBundle\DependencyInjection\Compiler\ResolversCompilerPass; use Liip\ImagineBundle\DependencyInjection\Factory\Loader\FileSystemLoaderFactory; use Liip\ImagineBundle\DependencyInjection\Factory\Loader\StreamLoaderFactory; use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\AwsS3ResolverFactory; @@ -20,7 +22,9 @@ public function build(ContainerBuilder $container) { parent::build($container); - $container->addCompilerPass(new LoadersCompilerPass()); + $container->addCompilerPass(new LoadersCompilerPass); + $container->addCompilerPass(new FiltersCompilerPass); + $container->addCompilerPass(new ResolversCompilerPass); /** @var $extension LiipImagineExtension */ $extension = $container->getExtension('liip_imagine'); diff --git a/Tests/DependencyInjection/ConfigurationTest.php b/Tests/DependencyInjection/ConfigurationTest.php index 9921b6f30..d7f7a5b6b 100644 --- a/Tests/DependencyInjection/ConfigurationTest.php +++ b/Tests/DependencyInjection/ConfigurationTest.php @@ -28,7 +28,15 @@ public function testCouldBeConstructedWithResolversAndLoadersFactoriesAsArgument public function testInjectLoaderFactoryConfig() { $config = $this->processConfiguration( - new Configuration(array(), array(new FooLoaderFactory, new FileSystemLoaderFactory)), + new Configuration( + array( + new WebPathResolverFactory + ), + array( + new FooLoaderFactory, + new FileSystemLoaderFactory + ) + ), array(array( 'loaders' => array( 'aLoader' => array( @@ -51,7 +59,15 @@ public function testInjectLoaderFactoryConfig() public function testAllowToUseLoaderFactorySeveralTimes() { $config = $this->processConfiguration( - new Configuration(array(), array(new FooLoaderFactory, new FileSystemLoaderFactory)), + new Configuration( + array( + new WebPathResolverFactory + ), + array( + new FooLoaderFactory, + new FileSystemLoaderFactory + ) + ), array(array( 'loaders' => array( 'aLoader' => array( @@ -77,7 +93,14 @@ public function testAllowToUseLoaderFactorySeveralTimes() public function testSetFilesystemLoaderAsDefaultLoaderIfNotDefined() { $config = $this->processConfiguration( - new Configuration(array(), array(new FileSystemLoaderFactory)), + new Configuration( + array( + new WebPathResolverFactory + ), + array( + new FileSystemLoaderFactory + ) + ), array(array( 'loaders' => array( ) @@ -89,10 +112,56 @@ public function testSetFilesystemLoaderAsDefaultLoaderIfNotDefined() $this->assertArrayHasKey('filesystem', $config['loaders']['default']); } + public function testSetFilesystemLoaderAsDefaultIfLoadersSectionNotDefined() + { + $config = $this->processConfiguration( + new Configuration( + array( + new WebPathResolverFactory + ), + array( + new FileSystemLoaderFactory + ) + ), + array(array()) + ); + + $this->assertArrayHasKey('loaders', $config); + $this->assertArrayHasKey('default', $config['loaders']); + $this->assertArrayHasKey('filesystem', $config['loaders']['default']); + } + + public function testSetWebPathResolversAsDefaultIfResolversSectionNotDefined() + { + $config = $this->processConfiguration( + new Configuration( + array( + new WebPathResolverFactory + ), + array( + new FileSystemLoaderFactory + ) + ), + array(array()) + ); + + $this->assertArrayHasKey('resolvers', $config); + $this->assertArrayHasKey('default', $config['resolvers']); + $this->assertArrayHasKey('web_path', $config['resolvers']['default']); + } + public function testShouldNotOverwriteDefaultLoaderIfDefined() { $config = $this->processConfiguration( - new Configuration(array(), array(new FooLoaderFactory, new FileSystemLoaderFactory)), + new Configuration( + array( + new WebPathResolverFactory + ), + array( + new FooLoaderFactory, + new FileSystemLoaderFactory + ) + ), array(array( 'loaders' => array( 'default' => array( @@ -113,7 +182,14 @@ public function testShouldNotOverwriteDefaultLoaderIfDefined() public function testInjectResolverFactoryConfig() { $config = $this->processConfiguration( - new Configuration(array(new BarResolverFactory, new WebPathResolverFactory), array()), + new Configuration( + array( + new BarResolverFactory, + new WebPathResolverFactory + ), array( + new FileSystemLoaderFactory + ) + ), array(array( 'resolvers' => array( 'aResolver' => array( @@ -136,7 +212,15 @@ public function testInjectResolverFactoryConfig() public function testAllowToUseResolverFactorySeveralTimes() { $config = $this->processConfiguration( - new Configuration(array(new BarResolverFactory, new WebPathResolverFactory), array()), + new Configuration( + array( + new BarResolverFactory, + new WebPathResolverFactory + ), + array( + new FileSystemLoaderFactory + ) + ), array(array( 'resolvers' => array( 'aResolver' => array( @@ -162,7 +246,13 @@ public function testAllowToUseResolverFactorySeveralTimes() public function testSetWebPathAsDefaultResolverIfNotDefined() { $config = $this->processConfiguration( - new Configuration(array(new WebPathResolverFactory), array()), + new Configuration( + array( + new WebPathResolverFactory + ), array( + new FileSystemLoaderFactory + ) + ), array(array( 'resolvers' => array( ) @@ -177,7 +267,15 @@ public function testSetWebPathAsDefaultResolverIfNotDefined() public function testShouldNotOverwriteDefaultResolverIfDefined() { $config = $this->processConfiguration( - new Configuration(array(new BarResolverFactory, new WebPathResolverFactory), array()), + new Configuration( + array( + new BarResolverFactory, + new WebPathResolverFactory + ), + array( + new FileSystemLoaderFactory + ) + ), array(array( 'resolvers' => array( 'default' => array( diff --git a/Tests/DependencyInjection/LiipImagineExtensionTest.php b/Tests/DependencyInjection/LiipImagineExtensionTest.php index 1a561180c..3e6aa0b2f 100644 --- a/Tests/DependencyInjection/LiipImagineExtensionTest.php +++ b/Tests/DependencyInjection/LiipImagineExtensionTest.php @@ -2,6 +2,8 @@ namespace Liip\ImagineBundle\Tests\DependencyInjection; +use Liip\ImagineBundle\DependencyInjection\Factory\Loader\FileSystemLoaderFactory; +use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\WebPathResolverFactory; use Liip\ImagineBundle\Tests\AbstractTest; use Liip\ImagineBundle\DependencyInjection\LiipImagineExtension; use Symfony\Component\DependencyInjection\ContainerBuilder; @@ -65,6 +67,8 @@ protected function createEmptyConfiguration() { $this->containerBuilder = new ContainerBuilder(); $loader = new LiipImagineExtension(); + $loader->addLoaderFactory(new FileSystemLoaderFactory); + $loader->addResolverFactory(new WebPathResolverFactory); $loader->load(array(array()), $this->containerBuilder); $this->assertTrue($this->containerBuilder instanceof ContainerBuilder); } @@ -76,6 +80,8 @@ protected function createFullConfiguration() { $this->containerBuilder = new ContainerBuilder(); $loader = new LiipImagineExtension(); + $loader->addLoaderFactory(new FileSystemLoaderFactory); + $loader->addResolverFactory(new WebPathResolverFactory); $loader->load(array($this->getFullConfig()), $this->containerBuilder); $this->assertTrue($this->containerBuilder instanceof ContainerBuilder); } diff --git a/Tests/Imagine/Cache/CacheManagerTest.php b/Tests/Imagine/Cache/CacheManagerTest.php index fff8ca1ef..31f72debe 100644 --- a/Tests/Imagine/Cache/CacheManagerTest.php +++ b/Tests/Imagine/Cache/CacheManagerTest.php @@ -45,7 +45,7 @@ public function testGetBrowserPathWithoutResolver() $cacheManager = new CacheManager($config, $this->createRouterMock(), new UriSigner('secret')); - $this->setExpectedException('OutOfBoundsException', 'Could not find resolver for "thumbnail" filter type'); + $this->setExpectedException('OutOfBoundsException', 'Could not find resolver "default" for "thumbnail" filter type'); $cacheManager->getBrowserPath('cats.jpeg', 'thumbnail'); } @@ -147,7 +147,7 @@ public function testThrowsIfConcreteResolverNotExists() { $cacheManager = new CacheManager($this->createFilterConfigurationMock(), $this->createRouterMock(), new UriSigner('secret')); - $this->setExpectedException('OutOfBoundsException', 'Could not find resolver for "thumbnail" filter type'); + $this->setExpectedException('OutOfBoundsException', 'Could not find resolver "default" for "thumbnail" filter type'); $this->assertFalse($cacheManager->resolve('cats.jpeg', 'thumbnail')); } diff --git a/Tests/Imagine/Data/DataManagerTest.php b/Tests/Imagine/Data/DataManagerTest.php index 2ea20b47a..5674875ac 100644 --- a/Tests/Imagine/Data/DataManagerTest.php +++ b/Tests/Imagine/Data/DataManagerTest.php @@ -239,7 +239,7 @@ public function testThrowIfLoaderNotRegisteredForGivenFilterOnFind() $dataManager = new DataManager($this->getMockMimeTypeGuesser(), $this->getMockExtensionGuesser(), $config); - $this->setExpectedException('InvalidArgumentException', 'Could not find data loader for "thumbnail" filter type'); + $this->setExpectedException('InvalidArgumentException', 'Could not find data loader "" for "thumbnail" filter type'); $dataManager->find('thumbnail', 'cats.jpeg'); } diff --git a/Tests/LiipImagineBundleTest.php b/Tests/LiipImagineBundleTest.php index 5193568b8..2f88b567a 100644 --- a/Tests/LiipImagineBundleTest.php +++ b/Tests/LiipImagineBundleTest.php @@ -29,7 +29,7 @@ public function testAddLoadersCompilerPassOnBuild() ->will($this->returnValue($this->createExtensionMock())) ; $containerMock - ->expects($this->once()) + ->expects($this->at(0)) ->method('addCompilerPass') ->with($this->isInstanceOf('Liip\ImagineBundle\DependencyInjection\Compiler\LoadersCompilerPass')) ; @@ -39,6 +39,46 @@ public function testAddLoadersCompilerPassOnBuild() $bundle->build($containerMock); } + public function testAddFiltersCompilerPassOnBuild() + { + $containerMock = $this->createContainerBuilderMock(); + $containerMock + ->expects($this->atLeastOnce()) + ->method('getExtension') + ->with('liip_imagine') + ->will($this->returnValue($this->createExtensionMock())) + ; + $containerMock + ->expects($this->at(1)) + ->method('addCompilerPass') + ->with($this->isInstanceOf('Liip\ImagineBundle\DependencyInjection\Compiler\FiltersCompilerPass')) + ; + + $bundle = new LiipImagineBundle; + + $bundle->build($containerMock); + } + + public function testAddResolversCompilerPassOnBuild() + { + $containerMock = $this->createContainerBuilderMock(); + $containerMock + ->expects($this->atLeastOnce()) + ->method('getExtension') + ->with('liip_imagine') + ->will($this->returnValue($this->createExtensionMock())) + ; + $containerMock + ->expects($this->at(2)) + ->method('addCompilerPass') + ->with($this->isInstanceOf('Liip\ImagineBundle\DependencyInjection\Compiler\ResolversCompilerPass')) + ; + + $bundle = new LiipImagineBundle; + + $bundle->build($containerMock); + } + public function testAddWebPathResolverFactoryOnBuild() { $extensionMock = $this->createExtensionMock();