From d61e294ff37368b55067dad1f9a245c68cf90384 Mon Sep 17 00:00:00 2001 From: Rob Frawley 2nd Date: Sat, 26 May 2018 23:53:26 -0400 Subject: [PATCH] add controller redirect response code configuration Signed-off-by: Rob Frawley 2nd --- Config/Controller/ControllerConfig.php | 37 +++++++ Controller/ImagineController.php | 61 +++++++----- DependencyInjection/Configuration.php | 9 ++ DependencyInjection/LiipImagineExtension.php | 4 + Resources/config/imagine.xml | 7 ++ Resources/doc/configuration.rst | 7 +- Tests/AbstractTest.php | 6 ++ .../Controller/ControllerConfigTest.php | 56 +++++++++++ Tests/Controller/ImagineControllerTest.php | 99 ++++++++++++++++++- .../LiipImagineExtensionTest.php | 1 + .../Controller/ImagineControllerTest.php | 10 +- Tests/Functional/app/config/config.yml | 4 + 12 files changed, 271 insertions(+), 30 deletions(-) create mode 100644 Config/Controller/ControllerConfig.php create mode 100644 Tests/Config/Controller/ControllerConfigTest.php diff --git a/Config/Controller/ControllerConfig.php b/Config/Controller/ControllerConfig.php new file mode 100644 index 000000000..bc99069ff --- /dev/null +++ b/Config/Controller/ControllerConfig.php @@ -0,0 +1,37 @@ +redirectResponseCode = $redirectResponseCode; + } + + public function getRedirectResponseCode(): int + { + return $this->redirectResponseCode; + } +} diff --git a/Controller/ImagineController.php b/Controller/ImagineController.php index 16cd485b4..e2dd205da 100644 --- a/Controller/ImagineController.php +++ b/Controller/ImagineController.php @@ -12,6 +12,7 @@ namespace Liip\ImagineBundle\Controller; use Imagine\Exception\RuntimeException; +use Liip\ImagineBundle\Config\Controller\ControllerConfig; use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException; use Liip\ImagineBundle\Exception\Imagine\Filter\NonExistingFilterException; use Liip\ImagineBundle\Imagine\Cache\Helper\PathHelper; @@ -41,15 +42,29 @@ class ImagineController private $signer; /** - * @param FilterService $filterService - * @param DataManager $dataManager - * @param SignerInterface $signer + * @var ControllerConfig */ - public function __construct(FilterService $filterService, DataManager $dataManager, SignerInterface $signer) + private $controllerConfig; + + /** + * @param FilterService $filterService + * @param DataManager $dataManager + * @param SignerInterface $signer + * @param ControllerConfig|null $controllerConfig + */ + public function __construct(FilterService $filterService, DataManager $dataManager, SignerInterface $signer, ?ControllerConfig $controllerConfig = null) { $this->filterService = $filterService; $this->dataManager = $dataManager; $this->signer = $signer; + + if (null === $controllerConfig) { + @trigger_error(sprintf( + 'Instantiating "%s" without a forth argument of type "%s" is deprecated since 2.2.0 and will be required in 3.0.', self::class, ControllerConfig::class + ), E_USER_DEPRECATED); + } + + $this->controllerConfig = $controllerConfig ?? new ControllerConfig(301); } /** @@ -73,19 +88,9 @@ public function filterAction(Request $request, $path, $filter) $path = PathHelper::urlPathToFilePath($path); $resolver = $request->get('resolver'); - try { - return new RedirectResponse($this->filterService->getUrlOfFilteredImage($path, $filter, $resolver), 301); - } catch (NotLoadableException $e) { - if (null !== $this->dataManager->getDefaultImageUrl($filter)) { - return new RedirectResponse($this->dataManager->getDefaultImageUrl($filter)); - } - - throw new NotFoundHttpException(sprintf('Source image for path "%s" could not be found', $path)); - } catch (NonExistingFilterException $e) { - throw new NotFoundHttpException(sprintf('Requested non-existing filter "%s"', $filter)); - } catch (RuntimeException $e) { - throw new \RuntimeException(sprintf('Unable to create image for path "%s" and filter "%s". Message was "%s"', $path, $filter, $e->getMessage()), 0, $e); - } + return $this->createRedirectResponse(function () use ($path, $filter, $resolver) { + return $this->filterService->getUrlOfFilteredImage($path, $filter, $resolver); + }, $path, $filter); } /** @@ -109,6 +114,7 @@ public function filterAction(Request $request, $path, $filter) public function filterRuntimeAction(Request $request, $hash, $path, $filter) { $resolver = $request->get('resolver'); + $path = PathHelper::urlPathToFilePath($path); $runtimeConfig = $request->query->get('filters', []); if (!\is_array($runtimeConfig)) { @@ -124,18 +130,29 @@ public function filterRuntimeAction(Request $request, $hash, $path, $filter) )); } + return $this->createRedirectResponse(function () use ($path, $filter, $runtimeConfig, $resolver) { + return $this->filterService->getUrlOfFilteredImageWithRuntimeFilters($path, $filter, $runtimeConfig, $resolver); + }, $path, $filter, $hash); + } + + private function createRedirectResponse(\Closure $url, string $path, string $filter, ?string $hash = null): RedirectResponse + { try { - return new RedirectResponse($this->filterService->getUrlOfFilteredImageWithRuntimeFilters($path, $filter, $runtimeConfig, $resolver), 301); - } catch (NotLoadableException $e) { + return new RedirectResponse($url(), $this->controllerConfig->getRedirectResponseCode()); + } catch (NotLoadableException $exception) { if (null !== $this->dataManager->getDefaultImageUrl($filter)) { return new RedirectResponse($this->dataManager->getDefaultImageUrl($filter)); } throw new NotFoundHttpException(sprintf('Source image for path "%s" could not be found', $path)); - } catch (NonExistingFilterException $e) { + } catch (NonExistingFilterException $exception) { throw new NotFoundHttpException(sprintf('Requested non-existing filter "%s"', $filter)); - } catch (RuntimeException $e) { - throw new \RuntimeException(sprintf('Unable to create image for path "%s" and filter "%s". Message was "%s"', $hash.'/'.$path, $filter, $e->getMessage()), 0, $e); + } catch (RuntimeException $exception) { + throw new \RuntimeException(vsprintf('Unable to create image for path "%s" and filter "%s". Message was "%s"', [ + $hash ? sprintf('%s/%s', $hash, $path) : $path, + $filter, + $exception->getMessage(), + ]), 0, $exception); } } } diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index 88ebc634a..1b787dbcb 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -11,6 +11,7 @@ namespace Liip\ImagineBundle\DependencyInjection; +use Liip\ImagineBundle\Config\Controller\ControllerConfig; use Liip\ImagineBundle\Controller\ImagineController; use Liip\ImagineBundle\DependencyInjection\Factory\Loader\LoaderFactoryInterface; use Liip\ImagineBundle\DependencyInjection\Factory\Resolver\ResolverFactoryInterface; @@ -123,6 +124,14 @@ public function getConfigTreeBuilder() ->children() ->scalarNode('filter_action')->defaultValue(sprintf('%s::filterAction', ImagineController::class))->end() ->scalarNode('filter_runtime_action')->defaultValue(sprintf('%s::filterRuntimeAction', ImagineController::class))->end() + ->integerNode('redirect_response_code')->defaultValue(301) + ->validate() + ->ifTrue(function ($redirectResponseCode) { + return !\in_array($redirectResponseCode, ControllerConfig::REDIRECT_RESPONSE_CODES, true); + }) + ->thenInvalid('Invalid redirect response code "%s" (must be 201, 301, 302, 303, 307, or 308).') + ->end() + ->end() ->end() ->end() ->arrayNode('filter_sets') diff --git a/DependencyInjection/LiipImagineExtension.php b/DependencyInjection/LiipImagineExtension.php index 6265ecb4d..be7b3af88 100644 --- a/DependencyInjection/LiipImagineExtension.php +++ b/DependencyInjection/LiipImagineExtension.php @@ -87,6 +87,10 @@ public function load(array $configs, ContainerBuilder $container) $container->setParameter('liip_imagine.driver_service', 'liip_imagine.'.$config['driver']); + $container + ->getDefinition('liip_imagine.controller.config') + ->replaceArgument(0, $config['controller']['redirect_response_code']); + $container->setAlias('liip_imagine', new Alias('liip_imagine.'.$config['driver'])); $container->setAlias(CacheManager::class, new Alias('liip_imagine.cache.manager', false)); $container->setAlias(DataManager::class, new Alias('liip_imagine.data.manager', false)); diff --git a/Resources/config/imagine.xml b/Resources/config/imagine.xml index e9c734158..53eae709c 100644 --- a/Resources/config/imagine.xml +++ b/Resources/config/imagine.xml @@ -139,12 +139,19 @@ + + + + + + + diff --git a/Resources/doc/configuration.rst b/Resources/doc/configuration.rst index 1b1300e55..435e6d188 100644 --- a/Resources/doc/configuration.rst +++ b/Resources/doc/configuration.rst @@ -26,8 +26,9 @@ The default configuration for the bundle looks like this: data_loader: default default_image: null controller: - filter_action: liip_imagine.controller:filterAction - filter_runtime_action: liip_imagine.controller:filterRuntimeAction + filter_action: liip_imagine.controller:filterAction + filter_runtime_action: liip_imagine.controller:filterRuntimeAction + redirect_response_code: 301 filter_sets: # Prototype @@ -62,6 +63,8 @@ There are several configuration options available: Default value: ``liip_imagine.controller:filterAction`` * ``filter_runtime_action`` - name of the controller action to use in the route loader for runtimeconfig images. Default value: ``liip_imagine.controller:filterRuntimeAction`` + * ``redirect_response_code`` - The HTTP redirect response code to return from the imagine controller, + one of ``201``, ``301``, ``302``, ``303``, ``307``, or ``308``. Default value: ``301`` * ``driver`` - one of the three drivers: ``gd``, ``imagick``, ``gmagick``. Default value: ``gd`` * ``filter_sets`` - specify the filter sets that you want to define and use. diff --git a/Tests/AbstractTest.php b/Tests/AbstractTest.php index c12f41f4c..c99c7d835 100644 --- a/Tests/AbstractTest.php +++ b/Tests/AbstractTest.php @@ -16,6 +16,7 @@ use Imagine\Image\Metadata\MetadataBag; use Liip\ImagineBundle\Binary\Loader\LoaderInterface; use Liip\ImagineBundle\Binary\MimeTypeGuesserInterface; +use Liip\ImagineBundle\Config\Controller\ControllerConfig; use Liip\ImagineBundle\Imagine\Cache\CacheManager; use Liip\ImagineBundle\Imagine\Cache\Resolver\ResolverInterface; use Liip\ImagineBundle\Imagine\Cache\SignerInterface; @@ -242,6 +243,11 @@ protected function createDataManagerMock() return $this->createObjectMock(DataManager::class, [], false); } + protected function createControllerConfigInstance(int $redirectResponseCode = null): ControllerConfig + { + return new ControllerConfig($redirectResponseCode ?? 301); + } + /** * @param string $object * @param string[] $methods diff --git a/Tests/Config/Controller/ControllerConfigTest.php b/Tests/Config/Controller/ControllerConfigTest.php new file mode 100644 index 000000000..17793d702 --- /dev/null +++ b/Tests/Config/Controller/ControllerConfigTest.php @@ -0,0 +1,56 @@ +assertSame($redirectResponseCode, (new ControllerConfig($redirectResponseCode))->getRedirectResponseCode()); + } + + public static function provideInvalidRedirectResponseCodeData(): \Generator + { + foreach ([200, 202, 304, 405, 406, 309, 310] as $code) { + yield [$code]; + } + } + + /** + * @dataProvider provideInvalidRedirectResponseCodeData + */ + public function testInvalidRedirectResponseCode(int $redirectResponseCode): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage(sprintf( + 'Invalid redirect response code "%s" (must be 201, 301, 302, 303, 307, or 308).', $redirectResponseCode + )); + $this->assertSame($redirectResponseCode, (new ControllerConfig($redirectResponseCode))->getRedirectResponseCode()); + } +} diff --git a/Tests/Controller/ImagineControllerTest.php b/Tests/Controller/ImagineControllerTest.php index 90d7f14be..9043f8fec 100644 --- a/Tests/Controller/ImagineControllerTest.php +++ b/Tests/Controller/ImagineControllerTest.php @@ -11,15 +11,24 @@ namespace Liip\ImagineBundle\Tests\Controller; +use Liip\ImagineBundle\Config\Controller\ControllerConfig; use Liip\ImagineBundle\Controller\ImagineController; +use Liip\ImagineBundle\Exception\InvalidArgumentException; use Liip\ImagineBundle\Tests\AbstractTest; +use Liip\ImagineBundle\Tests\Config\Controller\ControllerConfigTest; +use Symfony\Component\HttpFoundation\RedirectResponse; +use Symfony\Component\HttpFoundation\Request; /** * @covers \Liip\ImagineBundle\Controller\ImagineController */ class ImagineControllerTest extends AbstractTest { - public function testConstruction() + /** + * @group legacy + * @expectedDeprecation Instantiating "%s" without a forth argument of type "%s" is deprecated since 2.2.0 and will be required in 3.0. + */ + public function testDeprecatedConstruction(): void { $controller = new ImagineController( $this->createFilterServiceMock(), @@ -29,4 +38,92 @@ public function testConstruction() $this->assertInstanceOf(ImagineController::class, $controller); } + + public function testConstruction(): void + { + $controller = new ImagineController( + $this->createFilterServiceMock(), + $this->createDataManagerMock(), + $this->createSignerInterfaceMock(), + $this->createControllerConfigInstance() + ); + + $this->assertInstanceOf(ImagineController::class, $controller); + } + + public static function provideRedirectResponseCodeData(): \Generator + { + yield from ControllerConfigTest::provideRedirectResponseCodeData(); + } + + /** + * @dataProvider provideRedirectResponseCodeData + */ + public function testRedirectResponseCode(int $redirectResponseCode): void + { + $controller = $this->createControllerInstance( + $path = '/foo', + $filter = 'filter', + $hash = 'hash', + $redirectResponseCode + ); + + $response = $controller->filterAction(new Request(), $path, $filter); + $this->assertInstanceOf(RedirectResponse::class, $response); + $this->assertSame($redirectResponseCode, $response->getStatusCode()); + + $response = $controller->filterRuntimeAction(new Request(), $hash, $path, $filter); + $this->assertInstanceOf(RedirectResponse::class, $response); + $this->assertSame($redirectResponseCode, $response->getStatusCode()); + } + + public static function provideInvalidRedirectResponseCodeData(): \Generator + { + yield from ControllerConfigTest::provideInvalidRedirectResponseCodeData(); + } + + /** + * @dataProvider provideInvalidRedirectResponseCodeData + */ + public function testInvalidRedirectResponseCode(int $redirectResponseCode): void + { + $this->expectException(InvalidArgumentException::class); + $this->createControllerInstance( + $path = '/foo', + $filter = 'filter', + $hash = 'hash', + $redirectResponseCode, + false + ); + } + + private function createControllerInstance(string $path, string $filter, string $hash, int $redirectResponseCode, bool $expectation = true): ImagineController + { + $filterService = $this->createFilterServiceMock(); + $filterService + ->expects($expectation ? $this->atLeastOnce() : $this->never()) + ->method('getUrlOfFilteredImage') + ->with($path, $filter, null) + ->willReturn(sprintf('/resolved/image%s', $path)); + + $filterService + ->expects($expectation ? $this->once() : $this->never()) + ->method('getUrlOfFilteredImageWithRuntimeFilters') + ->with($path, $filter, [], null) + ->willReturn(sprintf('/resolved/image%s', $path)); + + $signer = $this->createSignerInterfaceMock(); + $signer + ->expects($expectation ? $this->once() : $this->never()) + ->method('check') + ->with($hash, $path, []) + ->willReturn(true); + + return new ImagineController( + $filterService, + $this->createDataManagerMock(), + $signer, + new ControllerConfig($redirectResponseCode) + ); + } } diff --git a/Tests/DependencyInjection/LiipImagineExtensionTest.php b/Tests/DependencyInjection/LiipImagineExtensionTest.php index 84a87a032..ce4152e8e 100644 --- a/Tests/DependencyInjection/LiipImagineExtensionTest.php +++ b/Tests/DependencyInjection/LiipImagineExtensionTest.php @@ -55,6 +55,7 @@ public function testLoadWithDefaults() new Reference('liip_imagine.service.filter'), new Reference('liip_imagine.data.manager'), new Reference('liip_imagine.cache.signer'), + new Reference('liip_imagine.controller.config'), ] ); } diff --git a/Tests/Functional/Controller/ImagineControllerTest.php b/Tests/Functional/Controller/ImagineControllerTest.php index 9df50233f..faa3a5ae6 100644 --- a/Tests/Functional/Controller/ImagineControllerTest.php +++ b/Tests/Functional/Controller/ImagineControllerTest.php @@ -36,7 +36,7 @@ public function testShouldResolvePopulatingCacheFirst() $response = $this->client->getResponse(); $this->assertInstanceOf(RedirectResponse::class, $response); - $this->assertSame(301, $response->getStatusCode()); + $this->assertSame(302, $response->getStatusCode()); $this->assertSame('http://localhost/media/cache/thumbnail_web_path/images/cats.jpeg', $response->getTargetUrl()); $this->assertFileExists($this->cacheRoot.'/thumbnail_web_path/images/cats.jpeg'); @@ -54,7 +54,7 @@ public function testShouldResolveFromCache() $response = $this->client->getResponse(); $this->assertInstanceOf(RedirectResponse::class, $response); - $this->assertSame(301, $response->getStatusCode()); + $this->assertSame(302, $response->getStatusCode()); $this->assertSame('http://localhost/media/cache/thumbnail_web_path/images/cats.jpeg', $response->getTargetUrl()); $this->assertFileExists($this->cacheRoot.'/thumbnail_web_path/images/cats.jpeg'); @@ -126,7 +126,7 @@ public function testShouldResolveWithCustomFiltersPopulatingCacheFirst() $response = $this->client->getResponse(); $this->assertInstanceOf(RedirectResponse::class, $response); - $this->assertSame(301, $response->getStatusCode()); + $this->assertSame(302, $response->getStatusCode()); $this->assertSame('http://localhost/media/cache/'.$expectedCachePath, $response->getTargetUrl()); $this->assertFileExists($this->cacheRoot.'/'.$expectedCachePath); @@ -161,7 +161,7 @@ public function testShouldResolveWithCustomFiltersFromCache() $response = $this->client->getResponse(); $this->assertInstanceOf(RedirectResponse::class, $response); - $this->assertSame(301, $response->getStatusCode()); + $this->assertSame(302, $response->getStatusCode()); $this->assertSame('http://localhost/media/cache'.'/'.$expectedCachePath, $response->getTargetUrl()); $this->assertFileExists($this->cacheRoot.'/'.$expectedCachePath); @@ -181,7 +181,7 @@ public function testShouldResolvePathWithSpecialCharactersAndWhiteSpaces() $response = $this->client->getResponse(); $this->assertInstanceOf(RedirectResponse::class, $response); - $this->assertSame(301, $response->getStatusCode()); + $this->assertSame(302, $response->getStatusCode()); $this->assertSame('http://localhost/media/cache/thumbnail_web_path/images/foo%20bar.jpeg', $response->getTargetUrl()); $this->assertFileExists($this->cacheRoot.'/thumbnail_web_path/images/foo bar.jpeg'); diff --git a/Tests/Functional/app/config/config.yml b/Tests/Functional/app/config/config.yml index f55be851c..17bddb6f6 100644 --- a/Tests/Functional/app/config/config.yml +++ b/Tests/Functional/app/config/config.yml @@ -25,6 +25,10 @@ framework: liip_imagine: + controller: + + redirect_response_code: 302 + loaders: default: