Introduce a CacheableRedirectResponse and use it where appropriate

Patch looks great :)

Looks great to me; boldly RTBCing.

GET http://ec2-52-27-70-16.us-west-2.compute.amazonaws.com/checkout/update.p... returned 0 (0 bytes).

Testbot--

This would be very valuable to have in 8.0.0, especially for modules that do redirects. Cacheable redirects make things much faster for the end user (because it also means the redirect can be cached in the browser's cache).

See https://groups.drupal.org/node/484788 for more information on the rc target tag. Could we put a bit on the impact vs. disruption in the summary to help decide when this patch can go in safely? Thanks! Maybe @catch could confirm if this is non-disruptive and important enough to be committed during RC.

+++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
@@ -8,14 +8,33 @@
-class LocalRedirectResponse extends SecuredRedirectResponse {
+class LocalRedirectResponse extends SecuredRedirectResponse implements CacheableResponseInterface {

+++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
@@ -8,14 +8,18 @@
-class TrustedRedirectResponse extends SecuredRedirectResponse {
+class TrustedRedirectResponse extends SecuredRedirectResponse implements CacheableResponseInterface {

I first thought these mean this can't go in after RC. But I think that's not true. Even if you've subclassed these objects, you won't have to change anything. So AFAICT there's no API change.

So I was mistaken in tagging this rc target earlier in any case. Sorry about that.

Added the impact vs disruption to the IS.

RTBC + 1 - it would be good to get this in before RC, but it potentially has big enough merit and less enough disruption to justify to go in later.

Re-roll

Redoing the reroll, because #46 is broken. This conflicted with #2579847: /node/add is lacking cacheability metadata, causes problems when cached by Dynamic Page Cache and "Use admin theme when editing or creating content" is turned off.

This is relative to #28.

Back to RTBC

Adding triage tag; it was also RTBC before RC deadline.

Given the recent discussions on #2527126: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them and the fact that that patch removes support for cacheable redirects (through a side effect) I think it is important that we introduce proper cacheable redirects during RC.

It'd been great if the IS can be updated with some of the discussions about previous support of redirect caching in Drupal 8 as a justification for this been done during RC.

+++ b/core/modules/user/user.routing.yml
@@ -12,6 +12,8 @@ user.logout:
+    no_cache: TRUE

Why is this necessary? Shouldn't caching for redirects be opt-in?

Or should 302s default to not cacheable and 301s default to cacheable?

I deliberately didn't re-read this issue, just trying to figure out the dependencies between this and #2527126: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them.

Lunch now, but I'll try to answer all questions here unless @znerol beats me to it.

It'd been great if the IS can be updated with some of the discussions about previous support of redirect caching in Drupal 8 as a justification for this been done during RC.

Done.

IMHO it is more than enough to say:

#2476407: Use CacheableResponseInterface to determine which responses should be cached made only CacheableResponseInterface responses cacheable by default.

We want to support that for redirects as well as there is a ContributedProjectBlocker for redirect module.

And cacheable redirects are a very common need.

Why is this necessary? Shouldn't caching for redirects be opt-in?

I agree. It looks like @znerol wanted to make as many redirects as possible cacheable. But of course, often times redirects happen after something happened. Which means the overall response is not actually cacheable.

Reviewing this patch again:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php
   @@ -39,7 +39,7 @@ public function redirect(GetResponseEvent $event) {
   -      $event->setResponse(new RedirectResponse($request->getUriForPath($path) . $qs));
   +      $event->setResponse(new CacheableRedirectResponse($request->getUriForPath($path) . $qs));
   

   This is indeed a perfectly cacheable redirect response, and it doesn't require any additional cacheability metadata, because it doesn't depend on any external information. It's a hard-coded redirect essentially.

2. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -28,6 +32,21 @@ class TrustedRedirectResponse extends SecuredRedirectResponse {
   +  protected function fromResponse(RedirectResponse $response) {
   +    parent::fromResponse($response);
   +
   +    $metadata = $this->getCacheableMetadata();
   +    if ($response instanceof CacheableResponseInterface) {
   +      $metadata->merge($response->getCacheableMetadata());
   +    }
   +    else {
   +      $metadata->setCacheMaxAge(0);
   +    }
   +  }
   

   This is very much worth having, but test coverage is missing.

3. +++ b/core/lib/Drupal/Core/Routing/UrlGeneratorTrait.php
   @@ -35,13 +35,17 @@
   +   * Note that responses generated by this method are cacheable by default.
   +   * Calling code is responsible for adding appropriate cacheability metadata
   +   * and cache dependencies respectively.
   

   This is the API change that is not acceptable anymore.

4. +++ b/core/lib/Drupal/Core/Routing/UrlGeneratorTrait.php
   @@ -52,13 +56,14 @@ protected function url($route_name, $route_parameters = array(), $options = arra
   -    $url = $this->url($route_name, $route_parameters, $options);
   -    return new RedirectResponse($url, $status);
   +    $url = $this->url($route_name, $route_parameters, $options, TRUE);
   +    $response = new CacheableRedirectResponse($url->getGeneratedUrl(), $status);
   +    return $response->addCacheableDependency($url);
   

   i.e. these changes need to be reverted.

5. +++ b/core/modules/locale/locale.routing.yml
   @@ -10,6 +10,8 @@ locale.check_translation:
   +  options:
   +    no_cache: TRUE
   

   Then all of these can go away.

6. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -93,7 +94,9 @@ public function addPage() {
   -      return $this->redirect('node.add', array('node_type' => $type->id()));
   +      $response = $this->redirect('node.add', ['node_type' => $type->id()]);
   +      $response->getCacheableMetadata()->setCacheTags($list_cache_tags);
   +      return $response;
   

   This one actually is cacheable, but… on 99% of sites, node.add is an admin route, so this won't be cached. Also, the redirect only happens if there's only one node type, which is something even less likely.

   So we'd be introducing a change here during RC with very, very limited benefit. So let's revert this one too.

7. +++ b/core/modules/user/user.routing.yml
   --- a/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTraitTest.php
   +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTraitTest.php
   

   Given we no longer modify UrlGeneratorTrait::redirect(), all these changes become obsolete too.

8. --- a/core/modules/locale/tests/modules/locale_test/locale_test.module
   +++ b/core/modules/locale/tests/modules/locale_test/locale_test.module
   

   The changes in this file should also no longer be necessary.

9. +++ b/core/lib/Drupal/Core/Routing/UrlGeneratorTrait.php
   @@ -35,13 +35,17 @@
   -  protected function url($route_name, $route_parameters = array(), $options = array()) {
   -    return $this->getUrlGenerator()->generateFromRoute($route_name, $route_parameters, $options);
   +  protected function url($route_name, $route_parameters = array(), $options = array(), $collect_bubbleable_metadata = FALSE) {
   +    return $this->getUrlGenerator()->generateFromRoute($route_name, $route_parameters, $options, $collect_bubbleable_metadata);
   

   This is an appropriate bugfix, but belongs in a separate issue. Especially now that this is the only remaining change in this file.

10. +++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
    @@ -8,14 +8,33 @@
    -class LocalRedirectResponse extends SecuredRedirectResponse {
    +class LocalRedirectResponse extends SecuredRedirectResponse implements CacheableResponseInterface {
     
    +  use CacheableResponseTrait;
    

    Rather than doing this here and for TrustedRedirectResponse, it makes more sense to just do it ONCE inSecuredRedirectResponse.

With that drastic reduction in scope, the changes in this patch are now limited to:

1. Introducing CacheableRedirectResponse.
2. Using that in RedirectLeadingSlashesSubscriber.
3. Update SecuredRedirectResponse to extend CacheableRedirectResponse.
4. Add test coverage for that in TrustedRedirectResponseTest.

Only 4 files touched, only 6 KB patch, no disruption, no BC break, and much, much simpler to review :)

So now I know why #60.10 wasn't implemented in the original patch:

Checking for illegal reference to 'Drupal\Core' namespace in /Users/wim.leers/Work/drupal-tres/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
Failed asserting that an array is empty.

I guess I'll have to move that back to the individual subclasses.

Added abstract class CacheableSecuredRedirectResponse extends SecuredRedirectResponse implements CacheableResponseInterface {} to \Drupal\Core\Routing, which LocalRedirectResponse and TrustedRedirectResponse now both extend. Still prevents that duplication, so still better than duplicating the logic in both of them.

IS updated: updated proposed resolution and beta eval.

When/if this lands, we should also update https://www.drupal.org/developing/api/8/response.

#65: woah, nice command!

I agree that #65 is very valuable to do. But those responses weren't cached in Page Cache before either, right? This issue is both a contrib blocker and blocking #2527126: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them. I think it's therefore important to keep this patch as minimal as possible. I think it'd be easier to make the changes you're suggesting in a follow-up issue, since they're purely nice-to-haves.

Of course, if committers would like to see that happen here, then I'd be happy to oblige.

This is with d5c827e:

That is the commit before #2476407: Use CacheableResponseInterface to determine which responses should be cached landed. So your motivation for this issue is fixing the regression for redirects in core, caused by #2476407.

But, the "contributed project blocker" bit and the "blocking #2527126" bit are the reasons this is an rc target.

The regression caused by #2476407 is merely unfortunate, its impact is very low. And I think it can easily be argued that those redirects being cacheable was mostly accidental, because there was no test coverage for them.

So, in summary: this issue is an rc target because it blocks another rc target. This patch is now a pure API addition. That makes it much easier to commit during RC. I realize that that is maybe different from your original intent with this issue, but would you not be okay with moving the "make those 3 redirects cacheable" to a separate issue? I'm happy to open that issue and file the patch.

#69: created that issue: #2594835: Make high-impact redirects that can be cached cacheable.

RTBC

Committed e900ded and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
index 66cbf36..5a7d4bb 100644
--- a/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
+++ b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
@@ -39,6 +39,9 @@ public static function createFromRedirectResponse(RedirectResponse $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());

Missing @param fixed on commit.

Thanks!

Updated https://www.drupal.org/developing/api/8/response (diff: https://www.drupal.org/node/2590001/revisions/view/8990175/9006681).

Unpostponed #2573923: Introduce a CacheableRedirectResponse and use it where appropriate.

Also updated https://www.drupal.org/developing/api/8/response/cacheable-response-inte....

Note: This already broke redirect.module, because it returns trusted responses and this now replaced the manually set cache tags header. Fixed here: https://github.com/md-systems/redirect/commit/0e8a0e4f305ec086b61bc3aea0...

The good part is that this means that #2527126: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them is even less an API change now, since it no longer breaks redirect.module as it was already broken and fixed. That issue won't affect it anymore ;)