Enable internal page cache by default

I dunno. The first thing people do on their Drupal site is not serve loads of traffic. First they build their site. And caching is pretty intrusive -- and confusing -- when you're trying to build a site and test things as an anonymous user. Performance tuning comes much later in the process. Maybe add something to the status report pointing out that caching is disabled, and that when they're done building their site they should turn it on?

If you have the knowledge to build a site (which most of the time is spent in admin mode, so no cache) and have to test things as anonymous, you have the knowledge to turn off page cache. Defaulting in at on is a good idea.

I think we should variable_set() this in the default profile, but leave it off by default in the expert profile.

I still think the standard profile should do this, bumping version.

Patch attached now that #1986090: Profile config does not overwrite module default config on install (system.cron.yml) is in.

5: drupal-standard-profile-page-caching-606840-4.patch queued for re-testing.

Recycling a >5 year old issue! :)

With #2381217: Views should set cache tags on its render arrays, and bubble the output's cache tags to the cache items written to the Views output cache having landed, we now have 99% coverage for cache tags:

• ~99% of things that can be changed and affect the rendered output have cache tags
• ~99% of those have test coverage, see e.g. EntityCacheTagsTestBase and subclasses

Those cache tags are present in the X-Drupal-Cache-Tags header, which the page cache uses as the cache tags for its cache items. This means that already, whenever something changes, the affected cached pages in the page cache are invalidated.

The benefits are enormous:

1. all Drupal 8 sites will be fast by default for anonymous traffic
2. "fast by default": roughly 10 milliseconds per page load instead of 100–300 — so ten to thirty times faster!
3. all simpler sites (think: Drupal developers' blogs, local sports club, small businesses) will be plenty fast, even on cheap hosting — thus resulting in greater satisfaction with D8
4. in the typical, poorly done benchmarks, Drupal 8 will leapfrog Drupal 7, and all of its competitors
5. … and still see results show up instantaneously!

Nice! Finally it is safe to do so!

#2445743: Allow views base tables and entity types to define additional cache contexts unless we want to add a security issue.

Maybe i'm wrong, sorry.

Thank you very much for the RTBC, Fabian, but this patch will most likely fail :)

Many tests assume no page caching is on, so I expect some odd things to break. I think the patch in #10 should be modified to opt-out the testing install profile from page caching, or perhaps only opt out certain tests.

#13 + #14: yeah, the page cache is unaffected by cache contexts, because it only caches for anonymous users.

why not move the config to Standard profile instead?

#17: that's an option, but I'd prefer that all install profiles (including contrib ones) are Fast By Default. I.e. I'd like page caching to be opt-out, not opt-in.

Actually, there are very good reasons to keep page caching enabled even for tests:

12:16:17 Fabianx-screen: WimLeers: Just 66 fails and 18 exceptions :)
12:19:46 Fabianx-screen: WimLeers: Actually it is a very good idea to run tests with page cache enabled.
12:19:51 Fabianx-screen: You find security thigns that way.
12:19:56 WimLeers: agreed
12:20:00 Fabianx-screen: HTTP response expected 403, actual 200
12:20:01 WimLeers: but I was unable to convince people of that last time
12:20:10 Fabianx-screen: e.g. Drupal\contact\Tests\ContactPersonalTest
12:20:24 WimLeers: Also finds bugs: pages that should be cacheable but aren't, and vice versa
12:20:30 WimLeers: (or aren't invalidated when they should be)
12:21:51 Fabianx-screen: yes
12:21:53 Fabianx-screen: indeed

Fixing these tests next, but first: a nice lunch to celebrate we're finally at this point :)

Its amazing to see how many sites still have page caching disabled because they think it could cause them issues, and fair to say, sometimes it did. But with all the cache tags goodness we now have in core and the 'it just works' experience it gives to both site builders and end-users, there's almost no place left for having it configurable, let alone keeping as an opt-in.

all my cents onto this!

Fixed the first failing test (12 failures). A form relied directly on a field definition/FieldConfig config entity, so the associated cache tag should've been associated.

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -94,6 +94,10 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Vary per role, because we check a permission above and attach an asset
   +    // library only for authenticated users.
   +    $form['#cache']['contexts'][] = 'user.roles';
   

   Should we then not vary on user.roles:anonymous and add a @todo for when CC hierarchy is in?

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -210,7 +214,12 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['#cache']['tags'] = Cache::mergeTags(
   +      isset($form['#cache']['tags']) ? $form['#cache']['tags'] : [],
   +      $config->getCacheTags(),
   +      // The form depends on the field definition.
   +      $field_definition->getConfig($entity->bundle())->getCacheTags()
   +    );
   

   I am looking forward to the new API :).

Next I looked at ContactPersonalTest. The failures there are because a 403 is expected, but a 200 is given. The 403 is expected because a permission is revoked from the anonymous users role.

The requested URL corresponds to the entity.user.contact_form route. That route's access checking uses the ContactPageAccess access check. The cache tags associated with that route-level access result must be reflected by the cache tags on the eventual response — otherwise the page cache item for that response will not be invalidated when the cache tags associated with the access result are invalidated.
This is a problem we knew we were going to have to solve at this point, and I think it makes sense to solve it here :)

See interdiff-contact_personal.txt.

Looking at the other two tests in the Contact module that are failing, similar problems exist. The 4 other failing tests in Contact module are due to flood control not working. That's another bug this issue has uncovered: flood control does *not* work for anonymous users when the page cache is on. The solution is simple: if flood control is active, then don't cache the page. Ideally, we'd communicate this with #cache['max-age'] === 0. But until #2443073: Add #cache[max-age] to disable caching and bubble the max-age lands, we'll have to use the page cache kill switch.

See interdiff-contact_rest.txt.

This should bring down the number of failures to <=48.

See interdiff.txt.

#25:

1. Indeed! (For others: he's referring to #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles').)
2. Same :) (For others: he's referring to #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()").)

+++ b/core/lib/Drupal/Core/Access/AccessResult.php
@@ -339,7 +339,8 @@ public function setCacheMaxAge($max_age) {
-    $this->addCacheContexts(array('user.roles'));
+    $this->addCacheContexts(['user.roles'])
+      ->addCacheTags(['config:user_role_list']);

This change would not be necessary if we choose to do #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?").

1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -145,6 +146,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +    // Merge the request's access result cacheability metadata, if it has any.
   +    $access_result = $request->attributes->get('_access_result');
   +    if ($access_result instanceof CacheableInterface) {
   +      $cache_contexts = Cache::mergeTags($cache_tags, $access_result->getCacheContexts());
   +      $cache_tags = Cache::mergeTags($cache_tags, $access_result->getCacheTags());
   +    }
   

   I'm curious, can we add a constant for that magic attribute?

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -92,25 +92,21 @@ public function matchRequest(Request $request) {
   -    $this->checkAccess($request);
   +    // The cacheability (if any) of this request's access check result must be
   +    // applied to the response.
   +    $access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
   +    if (!$request->attributes->has('_access_result')) {
   +      $request->attributes->set('_access_result', $access_result);
   +    }
   +    if (!$access_result->isAllowed()) {
   +      throw new AccessDeniedHttpException();
   +    }
   

   Why do we not just move the new logic into the checkAccess() method?

1. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -92,25 +92,21 @@ public function matchRequest(Request $request) {
   +    $access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
   +    if (!$request->attributes->has('_access_result')) {
   +      $request->attributes->set('_access_result', $access_result);
   +    }
   

   What happens if this is already set?

2. +++ b/core/modules/contact/src/Access/ContactPageAccess.php
   @@ -62,7 +62,7 @@ public function access(UserInterface $user, AccountInterface $account) {
        if ($contact_account->isAnonymous()) {
   -      return AccessResult::forbidden();
   +      return AccessResult::forbidden()->cachePerRole();
        }
   

   Interesting, is setting something to cache on not mandatory or are there cases where we don't need that?

3. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -339,7 +339,8 @@ public function setCacheMaxAge($max_age) {
   -    $this->addCacheContexts(array('user.roles'));
   +    $this->addCacheContexts(['user.roles'])
   +      ->addCacheTags(['config:user_role_list']);
   

I think this shows nicely why cache tags per context are a good thing ...

Can we add a @todo on that if this is always needed when the user.roles cache context is set?

I personally would have never thought about adding this tag to cache something per role ...

Nice work so far!

• #25.1: fixed.
• #28.1: Sure — but can you tell me where? I can only find RouteObjectInterface, which lives in Symfony. I can't find other examples. From IRC: WimLeers:add the constant onto the AccessAwareRouterInterface? — done.
• #28.2: d'oh, you're right, no need to make those changes I did. Thanks, fixed :)
• #30.1: this is to deal with the 403/404 response handling: that is rendered through a subrequest, but we want the access result of the main request to be used (for its cacheability metadata)
• #30.2: Yes, cacheability metadata on access checks is mandatory.
• #30.3: Done.

See interdiff-reviews.txt.

Also fixed the PHP fatals introduced in #26.

See interdiff-fatals.txt.

Both interdiffs above together form interdiff.txt.

I just found another massive bug thanks to the test failures above.

With page cache on, and with cached pages in the page cache… if you turn on maintenance mode, anon users continue to see the site itself instead of the maintenance mode version. And much worse: when disabling maintenance mode, the site doesn't show up again :P

This makes SiteMaintenanceTest pass again.

Tentatively marking for backport, because we should at least check that page cache and maintenance mode work correctly in D7.

Many failures in #31, I wonder why…

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -145,6 +146,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
+      $cache_contexts = Cache::mergeTags($cache_tags, $access_result->getCacheContexts());

… HAHA OOPS!

Also fixed the 6 failures in TwigRegistryLoaderTest.

I hid the wrong patch in #33, hence causing testbot to ignore my patch. Stupid.

I think we should split the individual bug fixes out to separate issues.

i.e. with contact module I don't think the problem is that flood control is incompatible with page caching, instead it's that flood control needs to be handled in validate/submit, not when viewing forms.

Less than 100 failures again.

(I also fixed Views plugins which declared cache.context.url as their cache context, that should've been url. I should open an issue to validate cache contexts, so that that is not possible anymore.)

Per #39, opened issues to file patches for most problems uncovered & fixed so far:

• #23: #2453143: CommentForm depends on configured field, but doesn't associate that cache tag
• #26 + #31: #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view
• #32: #2453351: Maintenance mode message ends up in page cache, served endlessly

Related #1032936: Maintenance mode should neither create nor retrieve cache, probably fixed already, so duplicate?

2 out of the 3 child issues filed in #42 have already been committed last week. The third is now up for final review at #2453341-13: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view — it now fixed Contact module's flooding checks as @catch requested in #39.

Re-uploading my original #10 patch, which only changed default config. Let's see how many failures we have left.

So, 62 failures. Two new child issues to bring that down further:

• #2458289: CronRunTest::testAutomaticCron() should test with an authenticated user
• #2458349: Route's access result's cacheability not applied to the response's cacheability — this moves a significant part of #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view into its own issue, since it's the cause behind many of the failures

Due to periodic misleading benchmarks especially with concurrent requests, I am marking this as critical.

If the default configuration is not fast, no one takes Drupal 8 seriously.

There is a case for this being critical with https://www.drupal.org/node/1744302#critical-perf-issues -- worth more discussion for sure.

Much as I'd love to see this in, I don't agree with it being Critical, and there's no reason it can't land as a Major. It doesn't match any of the criteria of https://www.drupal.org/node/1744302#critical-perf-issues. While it does have a >10ms benefit with warm caches, it doesn't meet the "and would have to be deferred to 9.x" part of those bullet points. I don't see a problem with this even landing in 8.0.1 if for some reason it doesn't before then, since it's just a default setting for new sites, not a forced change to existing sites.

Agreed with #51 — but for the public perception of Drupal 8, this may very well be an essential issue…

People have been doing rubbish benchmarks of Drupal core for a decade. While it would be fun for Drupal 8 to magically improve in those rubbish benchmarks compared to every previous release (unless Drupal 8 with page caching is slower than Drupal 4.5 without, who knows?), I don't see how the fact people do those rubbish benchmarks could in any way affect the priority of this issue.

However also not sure about changing this in a patch-level release. If we set this in the standard profile then that's probably fine (and in minimal too I'd be OK with). But if the config default changes then it would affect custom install profiles. You build your in-development project with drush si on 8.0.0 and there's no page caching; then 8.0.1 there is. That's not really a 'new site' in those situations and you wouldn't expect behaviour to change. In a minor release seems completely fine though.

New child issues:

1. #2460677: Tests testing config_test routes should use an authenticated user
2. #2460911: Search reindexing should invalidate cache tags
3. #2461063: AJAX forms using #ajax broken when page caching is enabled
4. #2461081: Lock test pages are uncacheable but aren't marked as such
5. #2461087: Add 'no_cache' route option to mark a route's responses as uncacheable (was: Cron run response should not be cacheable)
6. #2461097: Make TwigThemeTestController:::registryLoaderRender()'s response uncacheable
7. #2461105: One-time password reset page should never be cached

In the order of test failures, the following patches fix those test failures:

1. 15 failures (ConfigDependencyWebTest, ConfigEntityFormOverrideTest, ConfigEntityTest): #2460677: Tests testing config_test routes should use an authenticated user
2. 6 failures (ContactPersonalTest, ContactSitewideTest, ContactStorageTest): #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view — blocked on #2458349: Route's access result's cacheability not applied to the response's cacheability
3. 2 failures (LanguageUILanguageNegotiationTest): still debugging, very tricky
4. 3 failures (SearchCommentTest): #2460911: Search reindexing should invalidate cache tags
5. 13 failures (ElementValidationTest, FrameworkTest): #2461063: AJAX forms using #ajax broken when page caching is enabled
6. 3 failures (LockFunctionalTest): #2461081: Lock test pages are uncacheable but aren't marked as such
7. 1 failure (SessionTest): should be fixed as part of this issue*
8. 3 failures (CronQueueTest): #2461087: Add 'no_cache' route option to mark a route's responses as uncacheable (was: Cron run response should not be cacheable)
9. 6 failures (TwigRegistryLoaderTest): #2461097: Make TwigThemeTestController:::registryLoaderRender()'s response uncacheable
10. 1 failure (UserPasswordResetTest): #2461105: One-time password reset page should never be cached
11. 6 failures (UserRegistrationTest): [I know the fix, will post tomorrow]
12. 1 failure (GlossaryTest): should be fixed as part of this issue*
13. 1 failure (AccessTest): should be fixed as part of this issue*

*: the simple, sane fix for these test failures only makes sense if page caching is enabled by default, and therefore should land as part of this patch.

So that makes for 3 failures that should be fixed as part of this issue, 2 failures that I'm still debugging, and 6 failures that will get a patch tomorrow.

That adds up to 61 failures that are going to be fixed. The last patch had 62 failures. But 1 failure has already been fixed since: #2458289: CronRunTest::testAutomaticCron() should test with an authenticated user has landed.

Please help get those child issues committed!

I agree with @catch that we should not change this in a patch-level release. A minor could be okay but we'd need to think about how to roll it out. Even in a minor, we wouldn't want to change the configuration of existing sites. However, adding something like a reports page warning that says "Hey buddy, your page cache isn't on" in a minor could make sense.

Wow, came to review the children and found they'd all been rtbc'd - amazing.

Patch for #54.11: #2463029: EntityFormDisplay should update $form with cache tags of FieldConfig, FieldStorageConfig, EntityFormDisplay config entities.

#2461063: AJAX forms using #ajax broken when page caching is enabled, #2461081: Lock test pages are uncacheable but aren't marked as such, #2461105: One-time password reset page should never be cached and #2458349: Route's access result's cacheability not applied to the response's cacheability have landed. Reposting #45 to visualize progress (i.e. less failures on testbot).

No interdiff, because it's the same patch as in #45.

Next: the patch for #54.3 in a child issue, then a reroll of this patch that includes #54.7, #54.12 and #54.13. Once all child issues land, that patch should then become green.

Hah, 73 failures, which is more! But this is because UserRegistrationTest went from 6 to 34 failures. I had been seeing that number locally too. So no worries, we're still getting closer :)

Down to 55! More failures should be fixed now. Re-testing again.

49 failures! Most next steps are very close:

• #2460911: Search reindexing should invalidate cache tags
• #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view
• #2463029: EntityFormDisplay should update $form with cache tags of FieldConfig, FieldStorageConfig, EntityFormDisplay config entities

Once those 3 are in, I'll still need to create an issue+patch for #54.3. The solutions for #54.7, #54.12 and #54.13 will be part of this patch.

And finally, this patch should update example.settings.local.php to mention that the line disabling the render cache also disabled the page cache. i.e. this patch will not introduce a regression in DX, since you have to enable the local settings already to disable render caching, that already (in HEAD) disabled page caching too (because they use the same cache bin — which doesn't make all that much sense, but changing that is for a follow-up issue).

#2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view landed. Re-testing again, the number of failures should now drop from 49 to 43.

This should have only 37 failures remaining: 3 for SearchCommentTest (being fixed in #2460911: Search reindexing should invalidate cache tags, 34 for UserRegistrationTest (being fixed in #2463029: EntityFormDisplay should update $form with cache tags of FieldConfig, FieldStorageConfig, EntityFormDisplay config entities). All other failures are addressed by this patch, as described in #66.

A <10K patch! (Previous patch was a single line only, hence no interdiff.)

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -139,6 +139,22 @@ public function onRespond(FilterResponseEvent $event) {
+    // The 'user.permissions' cache context ensures that if the permissions for
+    // a role are modified, users aren't served stale render cache content. But,
+    // when entire responses are cached in reverse proxies, the value for the
+    // cache context is never calculated, causing the stale response to not be
+    // invalidated. Therefore, when varying by permissions and the current user
+    // is the anonymous user, also add the cache tag for the 'anonymous' role.
+    $cache_contexts = $response->headers->get('X-Drupal-Cache-Contexts');
+    if ($cache_contexts && in_array('user.permissions', explode(' ', $cache_contexts)) && \Drupal::currentUser()->isAnonymous()) {
+      $cache_tags = ['config:user.role.anonymous'];
+      if ($response->headers->get('X-Drupal-Cache-Tags')) {
+        $existing_cache_tags = explode(' ', $response->headers->get('X-Drupal-Cache-Tags'));
+        $cache_tags = Cache::mergeTags($existing_cache_tags, $cache_tags);
+      }
+      $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $cache_tags));
+    }

The new failures are due to this. This is also the most contentious part of this patch.

It's necessary to fix the two failures in ContactPersonalTest that you can see in the prior test run.

The test failures happen in this area:

    // Test that anonymous users can access the contact form.
    $this->drupalLogout();
    user_role_grant_permissions(RoleInterface::ANONYMOUS_ID, array('access user contact forms'));
    $this->drupalGet('user/' . $this->contactUser->id() . '/contact');
    $this->assertResponse(200);

    // Test that anonymous users can access admin user's contact form.
    $this->drupalGet('user/' . $this->adminUser->id() . '/contact');
    $this->assertResponse(200);
    $this->assertCacheContext('user.permissions');

    // Revoke the personal contact permission for the anonymous user.
    user_role_revoke_permissions(RoleInterface::ANONYMOUS_ID, array('access user contact forms'));
    $this->drupalGet('user/' . $this->contactUser->id() . '/contact');
    $this->assertResponse(403);
    $this->assertCacheContext('user.permissions');
    $this->drupalGet('user/' . $this->adminUser->id() . '/contact');
    $this->assertResponse(403);

You can see that permissions are being granted and removed for the anonymous user. That's what the cited code fixes.

Ideally, I'd get at least one +1 before rerolling to fix those new failures.

Oh and both of the last blockers are RTBC!

• #2460911: Search reindexing should invalidate cache tags
• #2463029: EntityFormDisplay should update $form with cache tags of FieldConfig, FieldStorageConfig, EntityFormDisplay config entities

@effulgentsia in chat:

[02/04/15 18:52:30] +1, but can that be moved to a separate issue with its own test that explicitly turns page caching on (rather than relying on site default)
[02/04/15 18:52:57] also, an explicit test rather than part of contact

So, filing a separate issue for that.

Also retesting, because #2460911: Search reindexing should invalidate cache tags and #2463029: EntityFormDisplay should update $form with cache tags of FieldConfig, FieldStorageConfig, EntityFormDisplay config entities have landed!

Reroll that:

• removes the changes cited in #72, per #74. They will be done in #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag.
• adds a link for the only unlinked @todo in the patch, which IMHO can be resolved after this is committed. This is a lesser problem. Links to #2464657: Remove unnecessary cache clear in Views tests.

This patch is now blocked on #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag, but should have only 2 failures, in ContactPersonalTest.

1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
   @@ -17,7 +17,6 @@
     *   weight = -2,
   - *   cache = 0,
     *   name = @Translation("Browser"),
   

   What does that do?

2. +++ b/core/modules/system/src/Tests/System/CronRunTest.php
   @@ -49,6 +49,12 @@ function testCronRun() {
   +    // Test with a logged in user; anonymous users likely don't cause Drupal to
   +    // fully bootstrap, because of the internal page cache or an external
   +    // reverse proxy. Reuse this user for disabling cron later in the test.
   +    $admin_user = $this->drupalCreateUser(array('administer site configuration'));
   +    $this->drupalLogin($admin_user);
   
   @@ -68,8 +74,6 @@ function testAutomaticCron() {
   -    $admin_user = $this->drupalCreateUser(array('administer site configuration'));
   -    $this->drupalLogin($admin_user);
   

   This should no longer be needed as the cron path is now page cache protected.

3. +++ b/core/modules/views/src/Tests/GlossaryTest.php
   @@ -70,10 +70,20 @@ public function testGlossaryView() {
   -    $this->drupalGet('glossary');
   +    $this->drupalGet($url);
   

   I think this cleanup can be moved to another issue?

4. +++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
   @@ -101,6 +102,14 @@ function testStaticAccessPlugin() {
   +    //   Remove as part of https://www.drupal.org/node/2464657.
   +    Cache::invalidateTags(['rendered']);
   

   This should be okay as a work around for now.

#2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag now has a patch! That's the last blocker. Please review!

What I feared has come true: the failure I was seeing locally was not due to my environment. Look at the test results (https://qa.drupal.org/pifr/test/1014238), the remaining failures in UserRegistrationTest are happening in the hunk that is testing user registration, at the very end:

    // Check that the 'add more' button works.
    $field_storage->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED);
    $field_storage->save();
    foreach (array('js', 'nojs') as $js) {
      …

i.e. it test the exact same thing, but with different input values, both with JS (AJAXy form) and without JS (regular form). Each separately works fine. But running both in succession causes this failure on my machine:

[31-Mar-2015 15:47:14 Europe/Brussels] Uncaught PHP Exception Drupal\Core\Entity\EntityStorageException: "SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '4a0944f2-9f46-4187-9f09-9b3743105ef6' for key 'user_field__uuid__value': INSERT INTO {users} (uid, uuid, langcode) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array	

And clearly also on testbot. I really have NFI why. I don't see why page caching would cause this. They are getting different values. I'll do some further digging.

#2462851: Improve Views entity row renderer plugins' cache contexts broke this; reroll needed.

#77:

1. It disables the browser language negotiation plugin's indication that it only works when the page cache is disabled. Was necessary to fix the language UI negotation test failures.
   Instead of saying that it's incompatible with page caching, this change makes it say it's compatible with page caching, and then instead it calls the page cache kill switch. As the @todo says, this will be fixed in #2430335: Browser language detection is not cache aware. This is not worse than before IMO.
2. Actually, no. That's what I thought too. But this basically is bringing back #2458289: CronRunTest::testAutomaticCron() should test with an authenticated user. Because it turns out it was necessary after all.
3. I needed to move things around, and figured I'd make that tiny change to improve consistency at the same time.

To fix the problem described in #78, we've opened #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state.

So once both #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag and #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state land, this issue will finally be green.

Fabianx pointed out that #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state is actually independently critical. We don't need to block this issue on that. We can just prevent the user registration page from being cached here, and then fix #2465053 separately.

That makes this now only blocked on #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag.

This is RTBC, except for the blocker.

I am curious if this will make the tests pass.

I suppose this is the proper way to clear the page cache.

While that works, it is not the right fix.

The problem is that the internal page cache does not take cache contexts into account.

It might be as simple as the attached patch:

#84/#85/#86 are all out of place here. The #81 patch will become green once #2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag lands, which contains the proper fix for this issue.

#86: that would slow the page cache down significantly, because it'd require the cache contexts service plus all individual cache context services to be initialized when processing page cache hits. The page cache is designed to be simple, and should remain simple: it should solely be URL-based. Reverse proxies other than the built-in page cache also won't work with your proposed solution.

Let's mark this as postponed to prevent any further confusion.

#2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag landed! Which means we can now finally land this patch! Re-uploading a rebased but otherwise unchanged #81. RTBC per #83.

1. +++ b/core/modules/system/config/install/system.performance.yml
   @@ -1,6 +1,6 @@
      page:
   -    use_internal: false
   +    use_internal: true
        max_age: 0
    css:
   

   So.. what does this mean exactly, in combination with max_age: 0?

   Looking at the code, the internal page cache doesn't care about max_age. It looks at Expires(), but we never actually set that at the moment, apart from setting it to a past date if we can't cache. So page cache is always permanent right now?

   Should we set expires to max-age? I guess that might not be safe when invalidations happen, as it won't re-vaidate then?

   Should we also set a different default value for max_age then, or are we explicitly not doing that, as you will need some sort of cache purging when using varnish or so?

2. +++ b/core/modules/system/src/Tests/System/CronRunTest.php
   @@ -49,6 +49,12 @@ function testCronRun() {
       * need the exact time when cron is triggered.
   ...
      function testAutomaticCron() {
   +    // Test with a logged in user; anonymous users likely don't cause Drupal to
   +    // fully bootstrap, because of the internal page cache or an external
   +    // reverse proxy. Reuse this user for disabling cron later in the test.
   +    $admin_user = $this->drupalCreateUser(array('administer site configuration'));
   +    $this->drupalLogin($admin_user);
   

   This is kind of interesting.

   It essentially means that, when page cache is enabled and not invalidated until something changes, your automatic cron will no longer work.

   That's not new and not directly related to this issue, as this is just changing the default. One more data point that using this feature is a bad idea ;)

3. +++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
   @@ -101,6 +102,14 @@ function testStaticAccessPlugin() {
   +    // Clear the page cache.
   +    // @todo Remove this. The root cause is that the access plugins alters the
   +    //   route's access requirements. That means that the 403 from above does
   +    //   not have any cache tags, so modifying the View entity does not cause
   +    //   the cached 403 page to be invalidated.
   +    //   Remove as part of https://www.drupal.org/node/2464657.
   

   Super-nitpick: Remove.. is a bit repetitive ;)

I'd expect this to be set in the standard install profile (or even standard and minimal), but no so much in the default configuration.

If we do #2368987: Move internal page caching to a module to avoid relying on config get on runtime then since we have no concept of a default module, we'd have to move this into the standard/minimal profiles then anyway, which would be a behaviour change when that patch lands.

#90:

1. Page cache indeed caches pages even when system.performance.page.max_age is zero. There actually is a pretty good reason for this. And none of this is new in this issue, it's all what's been in HEAD for a long, long time. The reason:
   1. system.performance.page.use_internal is only about whether Drupal's internal page cache should cache pages. The page cache caches pages indefinitely by default. It relies on cache tag invalidation to update its cached pages. It's independent of system.performance.page.max_age.
   2. system.performance.page.max_age is only about the Cache-Control header. Its default value of zero causes a header like this: Cache-Control: must-revalidate, no-cache, post-check=0, pre-check=0, private. This causes the end user's browser to talk to the server for every page load. Setting it to a non-zero value causes Cache-Control: max-age=0, private… which still causes revalidations on every page load. (See http://stackoverflow.com/questions/1046966/whats-the-difference-between-....) So, basically: the effects of this setting make little sense.

   So, summarizing: use_internal controls whether the internal page cache is or isn't used, max_age controls the Cache-Control header on responses. They appear strongly related, but they're actually independent. Also, max_age's effects make very little sense, but improving that is for a follow-up issue, since it's not caused by this issue.

   Explained from a different angle: use_internal affects the server side, max_age affects the client side.

2. Hehe, exactly. Basically, this is being more honest about the situations in which this Bad Idea Feature works and doesn't work.
3. Fixed :)

#91: done, as discussed in IRC with @catch.

RTBC + 1, that is nicer to have that in the profile.

Re #92

Setting it to a non-zero value causes Cache-Control: max-age=0, private… which still causes revalidations on every page load.

Really? Setting system.performance.page.max_age to n should actually translate to Cache-Control: public, max-age=n. See FinishResponseSubscriber::setResponseCacheable().

1. Yeah, I guess I was mostly thinking out loud, kind of know most of that myself :) 1.2 looks definitely broken, however, that's new and we're apparently missing test coverage there, seems to be related to the new max-age bubbling?

Enable internal cache (not relevant I think) and set max age to 600.

Expected cache-control: max-age: 600, public but it's max-age=0, private.

And this is happen in HtmlRenderer::renderContent(), which detects that some blocks or so set max-age: 0, then setMaxAge(0) is called. Then, in FinishResponseSubscriber, where we add the cache/nocache headers, we also call isCacheControlCustomized(), which returns TRUE, because of the previous setMaxAge() call. That means we never go into setResponseCacheable(), so we never do the things in there, which seem important, to ensure that headers like Vary: Cookie, Expires and Last-Modified are actually set.

That completely breaks reverse proxies? And If that's not a release-blocking bug, then I don't know what is? :)

The max_age setting and the max-age bubbling seem to contradict each other, and we know that bubbling doesn't actually work yet, as long as many things are still setting max-age: 0. So I think we should disable that setMaxAge() until we know that it actually works as expected *and* is implemented in a way that doesn't break the other headers? Which we should probably be set unconditionally anyway? And then we can also remove the global max_age setting.

#96 is right, this addition to HtmlRenderer from almost a month ago in #2443073: Add #cache[max-age] to disable caching and bubble the max-age broke it:

    // 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.
    if ($cache_max_age !== Cache::PERMANENT) {
      $response->setMaxAge($cache_max_age);
    }

We're indeed missing test coverage there. In the issue where it was added, it seemed like a harmless little step in the right direction, but clearly it wasn't, because there was no test coverage.

In any case: that's not a problem caused by this patch, it was merely discovered by me, trying to answer @Berdir's out-loud thinking. Let's continue this discussion/the fix in #2467041: 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) before we derail this issue.

Next step after this lands: #2467071: [meta] Find, fix & finish cache tag support: try to find broken scenarios as the anonymous user.

This would be excellent for novice contributors to productively help make Drupal 8.0.0 better! The bugs that that will uncover will each have a clear scope and will usually be easy to fix and test. Plus, it'll be easy to spread the work across many people.

Sent the word out. When you're ready, please put forth some concrete actions that novices can take to help out in that issue.

Wim, at risk of being a total fangirl, I just wanted to say kudos for the hard work on this. It's a really important fix... thank you. So exciting! Almost there...!
I will help get the word out for the followup #2467071: [PP-1] [meta] Find, fix & finish cache tag support. as soon as this lands, and try to put some GCDfolk on it as well.

+++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
index 0000000..5921966
--- /dev/null

--- /dev/null
+++ b/core/profiles/minimal/config/install/system.performance.yml

+++ b/core/profiles/minimal/config/install/system.performance.yml
+++ b/core/profiles/minimal/config/install/system.performance.yml
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+# @todo: Remove this file in https://www.drupal.org/node/2368987, update the
+#   install profile's YAML file instead.
+cache:
+  page:
+    use_internal: true
diff --git a/core/profiles/standard/config/install/system.performance.yml b/core/profiles/standard/config/install/system.performance.yml

diff --git a/core/profiles/standard/config/install/system.performance.yml b/core/profiles/standard/config/install/system.performance.yml
new file mode 100644

new file mode 100644
index 0000000..5921966

index 0000000..5921966
--- /dev/null

--- /dev/null
+++ b/core/profiles/standard/config/install/system.performance.yml

+++ b/core/profiles/standard/config/install/system.performance.yml
+++ b/core/profiles/standard/config/install/system.performance.yml
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+# @todo: Remove this file in https://www.drupal.org/node/2368987, update the
+#   install profile's YAML file instead.
+cache:
+  page:
+    use_internal: true
diff --git a/core/profiles/testing/config/install/system.performance.yml b/core/profiles/testing/config/install/system.performance.yml

diff --git a/core/profiles/testing/config/install/system.performance.yml b/core/profiles/testing/config/install/system.performance.yml
new file mode 100644

new file mode 100644
index 0000000..5921966

index 0000000..5921966
--- /dev/null

--- /dev/null
+++ b/core/profiles/testing/config/install/system.performance.yml

+++ b/core/profiles/testing/config/install/system.performance.yml
+++ b/core/profiles/testing/config/install/system.performance.yml
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+#   install profile's YAML file instead.
+cache:
+  page:
+    use_internal: true
diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php

Overrides don't work like this, you need to copy the whole file and then make the change. Was confused that this didn't cause any test fails, so I actually confirmed it manually, and yes, after installation, system.performance has just this value.

The @todo is also confusing, I guess you meant the install profile's info file? This is a YAML File too? There's also no need for the @todo IMHO, there's nothing wrong with how it is now, we have to change it in the other issue because it will just work differently there.

And last, not sure if we really want to add it to minimal. And for testing, I totally see that we want to add it, but it will have to install an additional module, which results in another iteration of container-building and yaml-parsing for every test :(

And last, not sure if we really want to add it to minimal.

Me neither. Although I just noticed that CSS/JS aggregation is enabled in the Minimal profile in Drupal 8. And that is a setting that causes at least as much pain for a typical developer as enabling the page cache would. I don't know the history of when that was done, but I can see the argument for making them all consistent... however, I think it might really be better for the Minimal profile to leave all of them off.

Very interested to see what happens with test failures when we try a version of this patch for Drupal 7, by the way. Could expose some fun bugs :)

#100: Will do!

#101: You're welcome, and thank you! :)

#102:

• RE: "overrides don't work like this". Huh, that's weird. I looked at core/profiles/testing/config/install/system.theme.yml for inspiration. This being wrong automatically means that that file is wrong, too. I also tested this manually, and it worked just fine. I just tried again and it still works fine. I tried both locally and on simplytest.me.
  EDIT: ah, I see, the problem is that all other settings in the config file then are missing/aren't imported. Interesting that that didn't cause any failures. Fixed!
• RE: the @todo, clarified that.
• RE: minimal. I strongly disagree. We very very much want to add this to the minimal install profile, because that's the profile most distributions/install profile are built on top of.

#103: to make a Drupal site developer-friendly, we have example.settings.local.php. This is nothing new. See https://www.drupal.org/node/2259531. Changing that policy/approach should be discussed in a new issue, not here.

Very interested to see what happens with test failures when we try a version of this patch for Drupal 7, by the way. Could expose some fun bugs :)

Haha, indeed! :D

Curious about:

We very very much want to add this to the minimal install profile, because that's the profile most distributions/install profile are built on top of.

Is there data to support this? I mean I believe you, it's just that anecdotally, I have always copy/pasted stuff from standard because in any install profile I make, I almost always want to do the same types of things standard is doing: enabling optional modules, setting up blocks, content types, setting default theme, etc. Most profiles/distrubutions are invented either because people want to do "Standard+" for their clients, or a full-blown product in Drupal with all the bells and whistles, so it's surprising to me that they would start with Minimal, which offers very little help in either area.

No data. That's paraphrasing a discussion I had with catch. (I think it was with catch and that was the reasoning, I *might* be misremembering.)

We never start with minimal

Drupal is now fast-by-default - regardless of profile.

I agree with Wim that it is not like 'minimal' == developer friendly AND 'standard' == 'use for production'.

To paraphrase webchick from another issue (I don't remember the exact quote): "But what if they install in minimal, develop and then push to production. If they never configured the development settings they would not know where to find them." (That for a patch where I was adding a UI to toggle dev mode).

I believe the same reasoning applies here and BECAUSE the page cache is disabled by disabling the render_cache via example.settings.local.php, this should be fine and is consistent with both js / css aggregation and the overall render_cache.

In the end for developing, just disabling page_cache is not enough, because render_cache will give as much "pain" during development.

Therefore back to RTBC, but great spot by berdir, that the overrides are wrong as config needs to be complete.

I think it is likely that committing this patch will find more bugs that we need to fix. I don't think anything is gained by trying to find them before committing this. I discussed the addition of system.performance default config with @Wim Leers in IRC he pointed out that these will be removed once #2368987: Move internal page caching to a module to avoid relying on config get on runtime lands.

Committed 25c41d0 and pushed to 8.0.x. Thanks!

Yes! Thanks so much @all!

HURRAY!!!!!!!!!!

Unblocked #2467071: [meta] Find, fix & finish cache tag support: try to find broken scenarios as the anonymous user, which is now updated with step-by-step instructions to make it easy to help out at Drupal Dev Days Montpellier next week to find all remaining problems!

https://www.drupal.org/node/2259531 updated.

This is awesome. Congrats @Wim Leers @Fabianx et al!

Do you folks think we could we create a separate issue for the D7 "backport" since its scope is going to be very different?

@Wim Leers Yes, I thought I was being naïve.

@xjm: Yes, +1. It's a very different task for 7.x (More like, let's see what happens if we do it and if we happen to catch a few things that are actual bugs). Let's close this and open a new one for 8.x.

Why would this be any different for Drupal 7? Couldn't we still turn it on by default in D7 core install profiles if we want to, which is the same thing that the Drupal 8 patch did?

We might not want to turn it on by default while tests are running (to avoid changing assumptions out from under contrib module tests)... although even then, if this breaks tests in contrib modules there's a reasonable chance that most of those are because of an actual bug in the module.

This issue is not critical for Drupal 7 though. (I'm not really sure why it was critical for Drupal 8 either - finding the bugs may have been critical, but actually changing the setting, not clear why.)

For starters, though, let's just see what fails.

The attached patch would NOT ever be committed to Drupal 7, but it should do the best possible job at flushing out any page caching bugs that could possibly be caught by the existing tests. Interesting that no one ever thought to let the testbot do this earlier, before this issue got recently revived :)

#103: to make a Drupal site developer-friendly, we have example.settings.local.php. This is nothing new. See https://www.drupal.org/node/2259531. Changing that policy/approach should be discussed in a new issue, not here.

In my earlier comment I guess I should have said "typical advanced site builder who dabbles in code" rather than "typical developer" (that's really more what I was talking about), but in any case, issue filed here: #2467929: Don't enable cache-related settings when installing via the Minimal profile

And wow, lots of test failures... On a quick glance through them I think a lot are just tests that didn't take into account the possibility that page caching would be turned on, but some are probably actual bugs.

Why would this be any different for Drupal 7? Couldn't we still turn it on by default in D7 core install profiles if we want to, which is the same thing that the Drupal 8 patch did?

It's vastly different in Drupal 7.

In Drupal 7, there are two big reasons to not turn page cache on by default:

1. Site builders/admins/content editors will be confused and then frustrated when they see that the changes they just made don't appear for anonymous users. The entire reason we only enabled it this late in the Drupal 8 cycle is that it took this long to make sure that problem doesn't exist. We didn't "just enable page caching", we first made sure that all changes appear instantaneously for anonymous users!
   (Thanks to cache tags: every rendered thing that depends on some entity or some configuration has a cache tag, that is bubbled all the way throughout the render tree to the response level, where they appear in the X-Drupal-Cache-Tags header. The page cache creates cache items with those cache tags. And whenever the entities/configuration associated with those cache tags is modified, the cache tags are invalidated, and the affected pages are invalidated in the page cache.)
2. The page cache in Drupal 7 is extremely inefficient. (See Cache invalidation is hard at http://wimleers.com/talk-really-fast-drupal-8/#/4/4.) So what Drupal 7 does is that whenever any node or comment is posted, it clears the entire page cache.

Suddenly starting to ship Drupal 7 with page caching enabled by default IMHO is a bad idea because it makes Drupal seem broken. It's an expectation that we've set many years ago in Drupal 7 (that out of the box everything updates instantaneously). That's why I think it's bad to enable it in a D7 point release: it breaks the expectations for new D7 installations.

I agree with #123, but enabling it for tests might make sense as we see that test failures could point to stale pages being served in one direction or another.

Oh, yes, this is still excellent to find bugs in D7. No question about that. But that's not the same as actually intending to enable page caching.

I agree with xjm and Berdir: this belongs in a separate issue, because the scope is different (assuming David Rothstein agrees with #124): the D7 scope is about finding pages incompatible with page cache (and by extension: with any reverse proxy), not about enabling page cache by default in D7.

@Wim Leers This is great work.

I need not have been given credit for the patch I posted. But I have learned a lot by reading this issue and the related issues.

I don't agree, at least not yet. If the page cache is so broken that we shouldn't put it in the standard install profile, why do so many people use it on their production sites? :)

Without the fixes that are in Drupal 8, it basically means that with the page cache enabled some types of changes you make on a Drupal 7 site don't show up for anonymous users until the next cron run (or next otherwise-triggered cache clear). It would definitely be great to see that improved (I've used https://www.drupal.org/project/cachetags once in the past and it occurs to me there'd be some easy wins if we simply had that in Drupal 7 core and used it in a few key places). But I don't see what that has to do with enabling this in the standard install profile. If those limitations/bugs are a problem for people I'd much rather see Drupal sites notice them early, rather than having them only appear once their site is about to launch and they turn page caching on for the first time then. Right? (Unless we think of the standard profile as more of a marketing tool than an actual base for building sites...)

I think whether or not to turn it on for the standard profile is more about whether it's a useful feature for the average Drupal 7 site:

1. Is the anonymous traffic expected to be high enough for page caching to be worth it?
2. Is the rate at which the site is changing expected to be low enough for page caching to be worth it?

This gets into "what does a typical Drupal site look like" and other fun questions, I guess...

I agree with #123, but enabling it for tests might make sense as we see that test failures could point to stale pages being served in one direction or another.

So that's what I did in #118. I am running the test failures from that locally now, to see what they're caused by... I'll check back later and look at some of them.

I am starting to think a lot of them (more than I originally thought) are probably of the "calling variable_set never clears the page cache" variety. If so, we should definitely spin off a new issue to talk about what, if anything, should be done about that (or any other bugs that this reveals should be new issues too).

I am thinking we would probably NOT want to commit an actual patch that enables the page cache for all tests in Drupal 7 (I was on the fence above)... while we might uncover some real bugs by doing that, we'd also likely cause a bunch of contrib module tests to fail for reasons that are not really bugs in the contrib module itself.

So for clarity, here's the actual patch that I think should be reviewed for Drupal 7.

If we're going to consider turning on the page cache in the Standard install profile we'd probably want to consider turning on CSS/JS aggregation at the same time... but for now, this is just the page cache.

Here's the summary of all test failures from #118 and what appears to cause them:

1. 403 pages might be cached somehow? - needs more investigation (AccessDeniedTestCase)
2. Adding/removing role permissions never clears the page cache (CommentAnonymous, CommentBlockFunctionalTest, CommentInterfaceTest, ContactPersonalTestCase, ContactSitewideTestCase)
3. variable_set never clears the page cache (CommentInterfaceTest, JavaScriptTestCase, MenuRouterTestCase, OpenIDFunctionalTestCase, OpenIDInvalidIdentifierTransitionTestCase, SiteMaintenanceTestCase, ThemeDebugMarkupTestCase, UserRegistrationTestCase)
4. Direct API calls to node_save(), comment_save(), etc. never clear the page cache (CommentInterfaceTest)
5. Automated cron never runs on cached pages, so if you rely on that your site might not have cron running as often as it should (CronRunTestCase) - I believe that affects Drupal 8 too
6. locale_add_language() never clears the page cache (DrupalHTTPRequestTestCase)
7. theme_enable() never clears the page cache (MenuRouterTestCase)
8. Adding new OpenIDs never clears the page cache? - needs more investigation (OpenIDFunctionalTestCase, OpenIDInvalidIdentifierTransitionTestCase)
9. Some kind of situation where a test admin user couldn't log in during maintenance mode even though they had "access site in maintenance mode" permission - needs more investigation (SiteMaintenanceTestCase)
10. Various field API functions, e.g. field_create_field(), never clear the page cache (UserRegistrationTestCase)

So a lot of those are things we might expect (and could easily think of some others that don't happen to cause test failures but would be similar to the above), but a few surprises in the list also.

Interestingly only only of the failing test cases (LockFunctionalTest) seemed to not represent an actual bug... I think that one is just due to the test not considering the fact that the output from the test URL it's using might be cached.

Invalidating page cache in 7.x and 8.x is very different. Drupal 7 relies on the expiration settings (a mix of max and min page cache expiration that works for your site) and the jackhammer method cache_clear_all(). That's just how it is, it works as well as it can for most sites, but you can't expect to get tests passing with it enabled. Throwing in lots of additional cache_clear_all() calls might fix tests, but it's definitely not going to make performance on real sites better.

Drupal 8 has tags, tags and more tags, so we can do immediate *and targeted* cache clearing. We've spent a huge amount of time over the last year(s) to get to this point, you can't expect that this will just work for 7.x in a similar way.

Yes, automated cron might be not be called as much as expected in D8, we noticed that, but it's not reliable anyway. One form submission here and there should be enough to trigger it (unlike D7, because form submissions exit before this this code is run). And everything else in that list should work fine apart from some known bugs.

@Berdir That is the exactly the thing I need to get used to.

I have a question on caching.

Over on #2399211: Support all options from views fields in DateTime formatters, I was looking at some code for rendering Date/Time fields in Views, and thinking about cacheability. This also has implications for any rendering of a Date/Time entity field, or any date/time value displayed on the site, inside and outside of Views.

There are two issues I came up with:

a) If a site has the setting turned on that lets users choose their own time zone, then any rendered date/time using regular date/time formats will have a different output for each user time zone. And in either case (with or without that setting), any change to the sitewide default time zone should invalidate cached date/time containing pages. And if a user changes their time zone setting, it should invalidate any pages cached for them. Has someone already thought of this in the cache tags for rendering dates/times, everywhere they're being used?

Maybe we're still only caching things for Anonymous users though; if so only the overall setting would be a factor... but I think maybe we're caching or trying to cache more, and have contexts? If so the individual user account and the config for the timezone should be part of the context for any date/time output with a regular format.

b) For any date/time value that is formatted using "ago" or "hence" types of formats, such as "how long ago was Cron last run", the output is relative to the current moment. Thus, any page that has a date/time on it using this type of format in the output cannot be cached at all, because the output would potentially be different even a second later. This would include typical Forum topic list pages (which list the time of last comments), user profiles (typically shows how long the user account has been active), etc. Has this been taken account of in the cache tags, everywhere this type of format is being used?

Hopefully this is the right issue to bring this up on... if not, sorry, where would that be?

Hopefully this is the right issue to bring this up on... if not, sorry, where would that be?

You might get a quick answer in this issue, but if not:
- For anonymous users: #2467071: [meta] Find, fix & finish cache tag support: try to find broken scenarios as the anonymous user.
- For authenticated: #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance.

I still really think it'd be better to open a separate issue for this discussion.

I also still think this really doesn't make sense as a default option without all the work that's been done in D8 on cache invalidation.

#133:
A) Yes, we use the 'timezone' cache tag for this
B) Yes, one must use JS for relative time indications

#135: +1

A) cache context, not cache tag :) But yes.

LOL, yes. My excuse: typing on an iPad, with horrendously stupid auto-correct.

OK, adding discussion to the two metas mentioned in #134

If we make a new issue for Drupal 7, we lose a lot of the history here. However it will also be easier to get feedback from actual site builders (which is what we really need here), plus to enable CSS/JS aggregation at the same time (which I think we'd want to do if we do this at all).

So I'll create a new issue, but first need to make followups for the test failures above so they don't get lost.

Invalidating page cache in 7.x and 8.x is very different. Drupal 7 relies on the expiration settings (a mix of max and min page cache expiration that works for your site) and the jackhammer method cache_clear_all()... Throwing in lots of additional cache_clear_all() calls might fix tests, but it's definitely not going to make performance on real sites better.

For several of the issues identified above, cache_clear_all() is a perfectly reasonable solution that won't hurt real world performance. For example, something like changing anonymous user permissions, which doesn't happen frequently and which tends to affect a large number of pages of the site at once when it does, it's simply not worth the effort to do targeted cache clears.

But yes, as mentioned previously we would need cache tags to fix some of them (e.g. for variable_set(); I don't think we'd want to do a cache_clear_all() in that case).

Yes, automated cron might be not be called as much as expected in D8, we noticed that, but it's not reliable anyway. One form submission here and there should be enough to trigger it (unlike D7, because form submissions exit before this this code is run).

For D7, form submissions should almost always trigger it also, since after submitting the form you usually wind up on a unique, uncached page (e.g. with a drupal_set_message()).

I created #2598372: Enable page caching and CSS/JS aggregation by default in the Standard install profile for the Drupal 7 issue.

And created the following issues for the cache-related bugs in Drupal 7 that were mentioned above:

403 pages might be cached somehow? - needs more investigation (AccessDeniedTestCase)

#2598320: Access denied (403) pages might be cached by Drupal's page cache

Adding/removing role permissions never clears the page cache (CommentAnonymous, CommentBlockFunctionalTest, CommentInterfaceTest, ContactPersonalTestCase, ContactSitewideTestCase)

#2598330: Changing role permissions never clears the page cache

variable_set never clears the page cache (CommentInterfaceTest, JavaScriptTestCase, MenuRouterTestCase, OpenIDFunctionalTestCase, OpenIDInvalidIdentifierTransitionTestCase, SiteMaintenanceTestCase, ThemeDebugMarkupTestCase, UserRegistrationTestCase)

#2598340: variable_set() never clears the page cache

Direct API calls to node_save(), comment_save(), etc. never clear the page cache (CommentInterfaceTest)

#2598346: Programmatic entity saves never clear the page cache

Automated cron never runs on cached pages, so if you rely on that your site might not have cron running as often as it should (CronRunTestCase) - I believe that affects Drupal 8 too

Didn't bother creating an issue for this, as discussed above.

locale_add_language() never clears the page cache (DrupalHTTPRequestTestCase)

#2598350: Adding languages never clears the page cache

theme_enable() never clears the page cache (MenuRouterTestCase)

I actually think I made a mistake on this one - there isn't necessarily a reason for theme_enable() to clear any caches. The test failures are probably all due to variable_set('theme_default'), variable_set('admin_theme'), etc... and therefore already covered by one of the issues above.

Adding new OpenIDs never clears the page cache? - needs more investigation (OpenIDFunctionalTestCase, OpenIDInvalidIdentifierTransitionTestCase)

#2598352: OpenID module doesn't always properly clear the page cache

Some kind of situation where a test admin user couldn't log in during maintenance mode even though they had "access site in maintenance mode" permission - needs more investigation (SiteMaintenanceTestCase)

#2598354: SiteMaintenanceTestCase fails when page caching is turned on

Various field API functions, e.g. field_create_field(), never clear the page cache (UserRegistrationTestCase)

#2598356: Various field API functions never clear the page cache