max-age on HTML responses wrongly set to `max-age=0, private` instead of `max-age=N, public` (breaks reverse proxies and client-side caching)

Thanks for creating the issue, I was too lazy again. +1 obviously, since it's my own suggestion :)

My take on that is:

- Make the default -1 == CACHE::Permanent and expose it in the UI
- External proxy and internal cache override max-age with the configured value IF the user is anonymous, else max-age = 0

We should support max-age bubbling for page cache and external proxies at some other point though.

I'll take a shot at this one.

Hey @jan.tsoeckler, are you still working on this one?

Sorry this took so long, I can't for the life of me figure out why the two added tests don't work as intended.

1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -176,9 +174,6 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
        // If an explicit non-infinite max-age is specified by a part of the page,
        // respect that by applying it to the response's headers.
   

   The comment should also be reverted.

2. +++ b/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
   @@ -227,6 +227,20 @@ function testPageCache() {
   +    // Check that max-age on HTML responses is persistent.
   +    $config->set('cache.page.max_age', 0);
   +    $config->save();
   +    $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Foo', 'value' => 'bar')));
   +    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control max-age (0) header is persistent.');
   +
   +    // @todo: make the following line nicer.
   +    drupal_flush_all_caches();
   +
   +    $config->set('cache.page.max_age', 300);
   +    $config->save();
   +    $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Foo', 'value' => 'bar')));
   +    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'max-age=300, public', 'Cache-Control max-age (300) header is persistent.');
   +
   

   If I understand correctly, this issue is about the fact that when #cache max-age is set explicitly on any element, it bubbles up the stack up until it causes setMaxAge() to be called on the response (which is the change being reverted in this issue), and causes the the Cache-Control header to contain "max-age: 0, private" instead of the standard cache header (with max-age set to page.cache.max_age).

   This test however seems to only test setting max_age on the page cache, not on elements itself, so I don't think correctly provides coverage for this issue.

   In any case, could you upload the test separately? That way you can verify that the test fails, while your patch together with the test succeeds (if that is the case, it proves that your test correctly covers the bug.)

Thanks for your review! I'm not quite sure whether or not bubbling is supposed to be supported at this point (I may well misconstrue the issue summary, though), so maybe this issue is just a bit too out of my wheelhouse. Anyway, uploaded the updated patch & the test separately.

Forgot to change status, sorry (again).

I'm taking myself off this issue, there's no way I'm going to fix it in an appropriate time frame.

I talked to Wim Leers to determine how to best test this, patch attached.

RTBC, both patch and test look great to me.

We can always bring the code back when we properly support it.

+++ b/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
@@ -236,6 +236,13 @@ function testPageCache() {
+    $this->assertNotEqual($this->drupalGetHeader('Cache-Control'), 'max-age=0, private');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'max-age=300, public');

Surely only the positive assertion is necessary? And we is the 300 coming from? The default configuration? We should get it from whether it is coming from.

• The max-age=300 is set at the start of the test.
• Removed the unnecessary assertion.

Back to RTBC, we indeed only need to check once.

... and fixed a typo in a comment.

cross-post.

+1. Is there any issue to make the max-age bubbling actually work?

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3bc5a5c and pushed to 8.0.x. Thanks!

I think the issue to fix this is #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache