Blocks #606840: Enable internal page cache by default.

This fixes the 3 failures o/t parent issue in:

  • Drupal\system\Tests\System\CronQueueTest

The bug in HEAD means that a curl -O http://drupal8-site.com/cron/<key> will only actually run cron ONCE!


#2458289: CronRunTest::testAutomaticCron() should test with an authenticated user committed a fix for CronRunTest, but this is the more appropriate fix, so as part of this patch, we revert the changes made over there.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because many routes (including the cron run route) have uncacheable responses, but don't mark them as uncacheable.
Issue priority Major because this can easily lead to a site not having cron running.
Prioritized changes The main goal of this issue is performance/DX.
Disruption Zero disruption.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: Cron run page should not be cacheable » Cron run response should not be cacheable
Status: Active » Needs review
FileSize
3.56 KB
star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me, very good docs.

Berdir’s picture

Does this mean that we no longer have to worry about the cron run in #2460911: Search reindexing should invalidate cache tags and keep that patch there shorter?

Wim Leers’s picture

You're referring to this hunk in that patch, I think?

+++ b/core/modules/search/src/Tests/SearchCommentTest.php
@@ -159,8 +159,8 @@ function testSearchResultsComment() {
     // Invoke search index update.
-    $this->drupalLogout();
     $this->cronRun();
+    $this->drupalLogout();
 

If so: yes. Great point. Let's postpone the other issue on this one.

Fabianx’s picture

+++ b/core/modules/system/src/CronController.php
@@ -49,6 +63,11 @@ public static function create(ContainerInterface $container) {
+    // \Drupal\system\Access\CronAccessCheck only cares that the given cron key
+    // is correct; it may very well be triggered by anonymous users, which means
+    // that the page should never be cached.
+    $this->pageCacheKillSwitch->trigger();

Totally out of scope for this issue, but I wondered:

Will this also set the headers to private for non-page cache enabled so that reverse proxies don't cache it?

---

To the issue:

RTBC + 1

Wim Leers’s picture

Will this also set the headers to private for non-page cache enabled so that reverse proxies don't cache it?

Yes. See FinishResponseSubscriber.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: cron_run_reponse_uncacheable-2461087-1.patch, failed testing.

Wim Leers’s picture

Testbot fail:

failed to checkout from [git://git.drupal.org/project/drupal.git]
Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Wim Leers’s picture

Priority: Normal » Major
Issue summary: View changes

Raising priority, since this bug in HEAD really means that a curl -O http://drupal8-site.com/cron/<key> will only actually run cron ONCE!

dawehner’s picture

Trying to work on it.

Wim Leers’s picture

Assigned: Unassigned » dawehner
Status: Reviewed & tested by the community » Needs work

Alex Pott, dawehner, Fabianx and I discussed how we've been using the page cache kill switch in more and more places, and how that's less than great. It'd be much better if you could do this for a route:

  options:
    _no_cache: TRUE

(i.e. mimicking HTTP's Cache-Control: no-cache)

Combined with a page cache response policy that prevents responses from those routes to be cached, that's a much more elegant (because declarative) than using the page cache kill switch, which is basically a superglobal.

That's what @dawehner is working on in #12.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
10.96 KB
11.47 KB

We should make it possible to opt out on the routing level in the first place.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -103,6 +125,11 @@ public function onRespond(FilterResponseEvent $event) {
+    // Mark the response as not cacheable if the route was configured like that.
+    if (($route = $this->routeMatch->getRouteObject()) && $route->getOption('no_cache')) {
+      $this->killSwitch->trigger();
+    }

This is still using that ugly superglobal. We can make it much more elegant! :)

Quoting #13:

Combined with a page cache response policy that prevents responses from those routes to be cached, that's a much more elegant (because declarative) than using the page cache kill switch, which is basically a superglobal.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
11.52 KB
5.51 KB

Did what I described in #15.

Wim Leers’s picture

FileSize
11.24 KB
715 bytes

Tiny bit of clean-up.

The last submitted patch, 16: 2461087-16.patch, failed testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine.

Noticed the _csrf_token requirement in the admin cron route, makes me wonder if routes that have that shouldn't automatically get excluded from page cache as well. Also not quite sure what would happen if you have a link that anonymous users can access with _csrf_token ;)

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/core.services.yml
    @@ -950,7 +956,7 @@ services:
    -    arguments: ['@language_manager', '@config.factory', '@page_cache_request_policy', '@page_cache_response_policy', '@cache_contexts']
    +    arguments: ['@language_manager', '@config.factory', '@page_cache_request_policy', '@page_cache_response_policy', '@cache_contexts', '@current_route_match', '@page_cache_kill_switch']
    

    This line is unfortunately superfluous now and not used.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -79,7 +79,7 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
    -   *   The cache contexts service.
    +   *   The cache contexts
    

    This change looks unrelated and wrong?

As much as I dislike it: Back to CNW unfortunately ...

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.63 KB
1.89 KB

#19: excellent point. Let's add a response policy for that in a follow-up :) Filed #2463303: Exempt all routes with a _csrf_token requirement from the page cache.

#20:

  1. Fixed.
  2. Fixed.
Wim Leers’s picture

Title: Cron run response should not be cacheable » Add 'no_cache' route option to mark a route's responses as uncacheable (was: Cron run response should not be cacheable)
Issue tags: +API addition, +DX (Developer Experience)

Updated title, tags.

Added CR: https://www.drupal.org/node/2463533.

Wim Leers’s picture

Issue summary: View changes

Beta evaluation added.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This looks great - thanks for the CR and Beta evaluation. Committed 144aa58 and pushed to 8.0.x. Thanks!

diff -u b/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php
--- b/core/modules/user/src/Controller/UserController.php
+++ b/core/modules/user/src/Controller/UserController.php
@@ -10,7 +10,6 @@
 use Drupal\Component\Utility\Xss;
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Datetime\DateFormatter;
-use Drupal\Core\PageCache\ResponsePolicy\KillSwitch;
 use Drupal\user\UserDataInterface;
 use Drupal\user\UserInterface;
 use Drupal\user\UserStorageInterface;
@@ -44,13 +43,6 @@
   protected $userData;
 
   /**
-   * The page cache killswitch.
-   *
-   * @var \Drupal\Core\PageCache\ResponsePolicy\KillSwitch
-   */
-  protected $pageCacheKillSwitch;
-
-  /**
    * Constructs a UserController object.
    *
    * @param \Drupal\Core\Datetime\DateFormatter $date_formatter
@@ -59,8 +51,6 @@
    *   The user storage.
    * @param \Drupal\user\UserDataInterface $user_data
    *   The user data service.
-   * @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $page_cache_kill_switch
-   *   The page cache killswitch.
    */
   public function __construct(DateFormatter $date_formatter, UserStorageInterface $user_storage, UserDataInterface $user_data) {
     $this->dateFormatter = $date_formatter;

Did some more clean up on commit.

  • alexpott committed 144aa58 on 8.0.x
    Issue #2461087 by Wim Leers, dawehner: Add 'no_cache' route option to...
star-szr’s picture

Love this. Great work!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.