Currently, you have to enable page cache for Drupal to sending out cacheable headers, yet it doesn't really do anything unless you also set a cache.page.maximum.age. If you set max age, you then need to give the page cache a null backend if you're using varnish to prevent drupal from also using the internal cache.

I propose that we remove the cache.page.enabled setting and replace it with cache.page.use_internal. With the attached patch, Drupal will send out the appropriate cache headers based on max age only. If you want to use the internal cache, you explicitly check that box.

I also reordered those options and reworded the descriptions to reveal what they are actually used for.

CommentFileSizeAuthor
#28 remove_cache_enabled_setting-1853086-28.patch11.8 KBkatbailey
#23 remove_cache_enabled_setting-1853086-23.patch11.75 KBalexpott
#22 remove_cache_enabled_setting-1853086-22.patch11.93 KBkatbailey
#9 remove_cache_enabled_setting-1853086-9.patch12.65 KBkatbailey
#9 interdiff.txt1.64 KBkatbailey
#6 remove_cache_enabled_setting-1853086-6.patch12.28 KBmsonnabaum
#4 remove_cache_enabled_setting-1853086-4.patch12.28 KBmsonnabaum
remove_cache_enabled_setting.patch12.19 KBmsonnabaum
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, remove_cache_enabled_setting.patch, failed testing.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs work » Needs review

remove_cache_enabled_setting.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, remove_cache_enabled_setting.patch, failed testing.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs work » Needs review
FileSize
remove_cache_enabled_setting-1853086-4.patch12.28 KB

Status: Needs review » Needs work

The last submitted patch, remove_cache_enabled_setting-1853086-4.patch, failed testing.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs work » Needs review
FileSize
remove_cache_enabled_setting-1853086-6.patch12.28 KB

Status: Needs review » Needs work

The last submitted patch, remove_cache_enabled_setting-1853086-6.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Category: feature » task

This makes sense to me. When we originally put in max_age support reverse proxy usage was much less common, so requiring people to figure out the null backend was deemed better than having people figure out two settings in the UI (vs. Pressflow 6.x external page cache setting). Things have moved on a bit now so it makes sense to make this configurable I think. Also making it opt-in for 'internal' page caching is conceptually easier than external page caching.

On the patch itself:

('If a reverse proxy cache isn\'t available, use Drupal\'s internal cache system to store cached pages.'),

Should use double quotes rather than escaping - it's better for translators.

Also I'm not sure about putting "If a reverse proxy cache isn't available" first in that sentence, takes longer to get to what the settings actually does, although the second bit also duplicates the form label so meh.

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.64 KB
remove_cache_enabled_setting-1853086-9.patch12.65 KB

This should fix the remaining test; it also addresses @catch's point about quotes in #8.

katbailey’s picture

katbailey CreditAttribution: katbailey commented

Oh, hmm - we can't use a config setting as the basis on which to decide whether to wrap the kernel in HttpCache as per #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache. It needs to be a $conf var in settings.php because we can't get config before booting the kernel, but on cached pages we don't want to boot the kernel. Or am I missing something?

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

I think you're right, but it's not a sure thing that we'll even use the HttpCache wrapper, so probably not relevant for this thread.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

yeah, there's still a slim chance of killing the fake http insanity that symfony loves, so hopefully we'll never use HttpCache.

Crell’s picture

Crell CreditAttribution: Crell commented

Switching to HttpCache is still the goal. I really don't know what your obsession is with "Fake HTTP". The entire point of HttpFoundation is to abstract the request away from the "real" superglobals so that 99% of the code doesn't know what's "real" and what's "fake" because that's a meaningless distinction.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

This issue has nothing to do with HttpCache. Please take the discussion elsewhere.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

msonnabaum - sorry :-\

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Moving this to settings.php means there's no longer a UI for it. I could possibly live with that if we enable this by default and force people to disable it, although not sure that'll be a popular suggestion.

Crell’s picture

Crell CreditAttribution: Crell commented

I don't think we can make that determination until we're done deciding what the cache setup is going to look like, and therefore what the cache lifetime actually affects. Stupid related problem spaces...

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Anything settings.php related is an issue with potential HttpCache implementations.

I dont see how this issue affects that one way or the other.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#9: remove_cache_enabled_setting-1853086-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, remove_cache_enabled_setting-1853086-9.patch, failed testing.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Needs a reroll.

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Status: Needs work » Needs review
FileSize
remove_cache_enabled_setting-1853086-22.patch11.93 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
remove_cache_enabled_setting-1853086-23.patch11.75 KB

Rerolled due to hook_boot() removal. I like this change and I think it's ready to be RTBC'd...

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

I think we're good here.

Dries’s picture

Dries CreditAttribution: Dries commented

#23: remove_cache_enabled_setting-1853086-23.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, remove_cache_enabled_setting-1853086-23.patch, failed testing.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs work » Reviewed & tested by the community

This looks good to go to me (nice clean-up) but I think this may need another re-roll.

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Status: Reviewed & tested by the community » Needs review
FileSize
remove_cache_enabled_setting-1853086-28.patch11.8 KB
katbailey’s picture

katbailey CreditAttribution: katbailey commented
Status: Needs review » Reviewed & tested by the community

It was just a straight-up re-roll, so setting back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Since this already passed Dries's sniff test,

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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