From e900ded48f63a7f45e0beb51b48a234e3b396657 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 16 Oct 2015 18:27:56 +0100 Subject: [PATCH] Issue #2573923 by znerol, Wim Leers, visabhishek, Fabianx, catch: Introduce a CacheableRedirectResponse and use it where appropriate --- .../SecuredRedirectResponse.php | 14 ++++++- .../Core/Cache/CacheableRedirectResponse.php | 26 +++++++++++++ .../RedirectLeadingSlashesSubscriber.php | 4 +- .../CacheableSecuredRedirectResponse.php | 37 +++++++++++++++++++ .../Core/Routing/LocalRedirectResponse.php | 4 +- .../Core/Routing/TrustedRedirectResponse.php | 4 +- .../Routing/TrustedRedirectResponseTest.php | 32 ++++++++++++++++ 7 files changed, 111 insertions(+), 10 deletions(-) create mode 100644 core/lib/Drupal/Core/Cache/CacheableRedirectResponse.php create mode 100644 core/lib/Drupal/Core/Routing/CacheableSecuredRedirectResponse.php diff --git a/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php index 77c978c7d7a..5a7d4bbd654 100644 --- a/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php +++ b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php @@ -33,11 +33,21 @@ abstract class SecuredRedirectResponse extends RedirectResponse { */ public static function createFromRedirectResponse(RedirectResponse $response) { $safe_response = new static($response->getTargetUrl(), $response->getStatusCode(), $response->headers->allPreserveCase()); - $safe_response->setProtocolVersion($response->getProtocolVersion()); - $safe_response->setCharset($response->getCharset()); + $safe_response->fromResponse($response); return $safe_response; } + /** + * Copies over the values from the given response. + * + * @param \Symfony\Component\HttpFoundation\RedirectResponse $response + * The redirect reponse object. + */ + protected function fromResponse(RedirectResponse $response) { + $this->setProtocolVersion($response->getProtocolVersion()); + $this->setCharset($response->getCharset()); + } + /** * {@inheritdoc} */ diff --git a/core/lib/Drupal/Core/Cache/CacheableRedirectResponse.php b/core/lib/Drupal/Core/Cache/CacheableRedirectResponse.php new file mode 100644 index 00000000000..07d9831c7db --- /dev/null +++ b/core/lib/Drupal/Core/Cache/CacheableRedirectResponse.php @@ -0,0 +1,26 @@ +setResponse(new RedirectResponse($request->getUriForPath($path) . $qs)); + $event->setResponse(new CacheableRedirectResponse($request->getUriForPath($path) . $qs)); } } diff --git a/core/lib/Drupal/Core/Routing/CacheableSecuredRedirectResponse.php b/core/lib/Drupal/Core/Routing/CacheableSecuredRedirectResponse.php new file mode 100644 index 00000000000..c8d024416c4 --- /dev/null +++ b/core/lib/Drupal/Core/Routing/CacheableSecuredRedirectResponse.php @@ -0,0 +1,37 @@ +getCacheableMetadata(); + if ($response instanceof CacheableResponseInterface) { + $metadata->addCacheableDependency($response->getCacheableMetadata()); + } + else { + $metadata->setCacheMaxAge(0); + } + } + +} diff --git a/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php index d8ad719bcc6..4d0e275ae17 100644 --- a/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php +++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php @@ -7,12 +7,10 @@ namespace Drupal\Core\Routing; -use Drupal\Component\HttpFoundation\SecuredRedirectResponse; - /** * Provides a redirect response which cannot redirect to an external URL. */ -class LocalRedirectResponse extends SecuredRedirectResponse { +class LocalRedirectResponse extends CacheableSecuredRedirectResponse { use LocalAwareRedirectResponseTrait { LocalAwareRedirectResponseTrait::isLocal as isSafe; diff --git a/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php index c7a43a21c6b..3beb17646e5 100644 --- a/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php @@ -7,14 +7,12 @@ namespace Drupal\Core\Routing; -use Drupal\Component\HttpFoundation\SecuredRedirectResponse; - /** * Provides a redirect response which contains trusted URLs. * * Use this class in case you know that you want to redirect to an external URL. */ -class TrustedRedirectResponse extends SecuredRedirectResponse { +class TrustedRedirectResponse extends CacheableSecuredRedirectResponse { use LocalAwareRedirectResponseTrait; diff --git a/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php b/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php index 4ac1333fac8..2414798880c 100644 --- a/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php +++ b/core/tests/Drupal/Tests/Core/Routing/TrustedRedirectResponseTest.php @@ -7,10 +7,14 @@ namespace Drupal\Tests\Core\Routing; +use Drupal\Core\Cache\CacheableMetadata; +use Drupal\Core\Cache\CacheableRedirectResponse; +use Drupal\Core\Cache\CacheableResponseInterface; use Drupal\Core\Routing\RequestContext; use Drupal\Core\Routing\TrustedRedirectResponse; use Drupal\Tests\UnitTestCase; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\HttpFoundation\RedirectResponse; /** * @coversDefaultClass \Drupal\Core\Routing\TrustedRedirectResponse @@ -54,4 +58,32 @@ class TrustedRedirectResponseTest extends UnitTestCase { $this->assertEquals('http://good-external-url.com/example', $redirect_response->getTargetUrl()); } + /** + * @covers ::createFromRedirectResponse + * @dataProvider providerCreateFromRedirectResponse + */ + public function testCreateFromRedirectResponse($redirect_response) { + $trusted_redirect_response = TrustedRedirectResponse::createFromRedirectResponse($redirect_response); + + // The trusted redirect response is always a CacheableResponseInterface instance. + $this->assertTrue($trusted_redirect_response instanceof CacheableResponseInterface); + + // But it is only actually cacheable (non-zero max-age) if the redirect + // response passed to TrustedRedirectResponse::createFromRedirectResponse() + // is itself cacheable. + $expected_cacheability = ($redirect_response instanceof CacheableResponseInterface) ? $redirect_response->getCacheableMetadata() : (new CacheableMetadata())->setCacheMaxAge(0); + $this->assertEquals($expected_cacheability, $trusted_redirect_response->getCacheableMetadata()); + } + + /** + * @return array + */ + public function providerCreateFromRedirectResponse() { + return [ + 'cacheable-with-tags' => [(new CacheableRedirectResponse('/example'))->addCacheableDependency((new CacheableMetadata())->addCacheTags(['foo']))], + 'cacheable-with-max-age-0' => [(new CacheableRedirectResponse('/example'))->addCacheableDependency((new CacheableMetadata())->setCacheMaxAge(0))], + 'uncacheable' => [new RedirectResponse('/example')], + ]; + } + }