Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#21 | 2461087-21.patch | 9.63 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
star-szrMakes sense to me, very good docs.
Comment #3
BerdirDoes 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?
Comment #4
Wim LeersYou're referring to this hunk in that patch, I think?
If so: yes. Great point. Let's postpone the other issue on this one.
Comment #5
Fabianx CreditAttribution: Fabianx for Drupal Association commentedTotally 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
Comment #6
Wim LeersYes. See FinishResponseSubscriber.
Comment #8
Wim LeersTestbot fail:
Comment #9
Wim LeersComment #11
Wim LeersRaising priority, since this bug in HEAD really means that a
curl -O http://drupal8-site.com/cron/<key>
will only actually run cron ONCE!Comment #12
dawehnerTrying to work on it.
Comment #13
Wim LeersAlex 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:
(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.
Comment #14
dawehnerWe should make it possible to opt out on the routing level in the first place.
Comment #15
Wim LeersThis is still using that ugly superglobal. We can make it much more elegant! :)
Quoting #13:
Comment #16
Wim LeersDid what I described in #15.
Comment #17
Wim LeersTiny bit of clean-up.
Comment #19
BerdirLooks 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 ;)
Comment #20
Fabianx CreditAttribution: Fabianx for Drupal Association commentedThis line is unfortunately superfluous now and not used.
This change looks unrelated and wrong?
As much as I dislike it: Back to CNW unfortunately ...
Comment #21
Wim Leers#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:
Comment #22
Wim LeersUpdated title, tags.
Added CR: https://www.drupal.org/node/2463533.
Comment #23
Wim LeersBeta evaluation added.
Comment #24
alexpottThis looks great - thanks for the CR and Beta evaluation. Committed 144aa58 and pushed to 8.0.x. Thanks!
Did some more clean up on commit.
Comment #26
star-szrLove this. Great work!
Comment #27
Wim LeersGreat!
This unblocked:
All 3 have been rerolled!