#cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age

So, is it really only this ?

This will be able to copy/paste the test coverage added in #2493047: Cache redirects should be stored in the same cache bin and adjust it.

What I'm missing here is a thorough explanation for why we want to cache contexts permanently.

E.g.:

1. a block is render cached by contexts A, B and C, with a max-age of 3600, and is using a cache redirect.
2. context C is present because an access checker returned an access result with context C and max-age 3600
3. after 3600 seconds have passed, the access checker no longer returns context C and max-age 3600, but context D, without a max-age

Then my question is: why should context C be permanently included in the cache redirect?

Aha! Good point. The last max-age indeed always wins.

But we can easily retrieve the previous max-age: if the cache item's expired === -1, then it was Cache::PERMANENT. Otherwise, it is expire - created. Then we can use Cache::mergeMaxAges().

Or what am I missing?

#2493047: Cache redirects should be stored in the same cache bin landed, which makes it much easier to write the necessary test coverage in this issue.

Et voila. A simple, elegant patch, thanks to all of the refactoring that went into RenderCache. And thanks to the test coverage that has been refined in #2493047: Cache redirects should be stored in the same cache bin and other issues, adding the necessary test coverage is also incredibly easy :)

Update per #3 + #4.

Rebased. Conflicted with #2483781: Move cache contexts classes from \Drupal\Core\Cache to \Drupal\Core\Cache\Context.

+++ b/core/lib/Drupal/Core/Render/RenderCache.php
@@ -234,14 +228,16 @@ public function set(array &$elements, array $pre_bubbling_elements) {
+            'contexts' => $redirect_cacheability_updated->getCacheContexts(),
             // The union of the current element's and stored cache tags.
-            'tags' => Cache::mergeTags($stored_cache_tags, $data['#cache']['tags']),
+            'tags' => $redirect_cacheability_updated->getCacheTags(),
+            // The union of the current element's and stored cache max-ages.
+            'max-age' => $redirect_cacheability_updated->getCacheMaxAge(),
             // The same cache bin as the one for the actual render cache items.
             'bin' => $bin,
           ],
         ];

This could use additional documentation I think.

Based on comment #3, the redirect cache item always needs to have a TTL that is the same as the shortest redirected cache item at any one time - so that it expires when any of them do (in case things change on the expiry).

What we're essentially doing is bubbling max ages of cache contexts to cache redirect items - that's a lot of concepts to keep straight all at once.

#13 It's just merging the cacheability metadata of the existing cache redirect (if any) with what would be written to the updated cache redirect. HEAD is already doing this for cache contexts and cache tags. All this patch does, is also do it for max-age. And hence, just do it for all bits of cacheability metadata. That's why this patch is able to simplify things.

Back to RTBC for more maintainer feedback.

Test failures were not reproducible locally. Betting it was a testbot thing.

Was broken by #2511472: Refactor all usages of drupal_render()/Renderer::render() that break #2450993.

Rebased, conflict automatically resolved by git.

@Wim yes I understand #14 but I think we should add extra docs explaining why exactly we have to do it.

Done.

Committed/pushed to 8.0.x, thanks!