Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
#2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter() prevents you from adding #cache tags in hook_page_attachments(_alter)(). That's useful because if you change settings in those methods it allows you to add cache tags to it.
Proposed resolution
Add a #cache to the list of allowed keys in invokePageAttachmentHooks() method.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#7 | allow_to_set_cache-2475749-7-interdiff.txt | 1.81 KB | mbovan |
#7 | allow_to_set_cache-2475749-7.patch | 4.56 KB | mbovan |
#4 | allow_to_set_cache-2475749-4-interdiff.txt | 805 bytes | mbovan |
#4 | allow_to_set_cache-2475749-4.patch | 4.37 KB | mbovan |
#1 | allow_to_set_cache-2475749-1.patch | 3.86 KB | mbovan |
Comments
Comment #1
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedProviding first patch. Added #cache tags, changed the exception messages and updated exception message in existing tests.
Where should we test this?
Comment #2
Wim LeersAdding additional assertions to
PageRenderTest
seems sufficient.In a follow-up, we should make all
hook_page_attachments()
,hook_page_attachments_alter()
,hook_page_top()
andhook_page_bottom()
implementations set the right cacheability metadata.For example,
quickedit_page_attachments()
only adds its asset library if the current user has the right permission (vary by the'user.permissions'
cache context) and it's not an admin route (no cache context for that yet).Comment #3
BerdirComment #4
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedAdded assertion for the cache tags in
PageRenderTest
.Comment #5
hass CreditAttribution: hass commentedThat is something I need for Google Analytics, too.
Comment #6
Wim LeersCan you also add a cache context, just for the sake of completeness? :) E.g. the
user.permissions
cache context, which is likely the one that'll be used most often in the real world.Then this is RTBC.
(But I should file a new issue for #2.)
Comment #7
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedAdded cache context assertion. :)
Comment #8
BerdirThis should be ready then, don't think testbot will fail on this.
Comment #9
alexpottCan someone open a followup for #2.
This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 5b560a5 and pushed to 8.0.x. Thanks!
Comment #11
hass CreditAttribution: hass commentedIs there any documentation how these cache metadata works and when and how modules need to implement it?
This is not clear to me from this case.
Comment #12
Wim Leers#11: https://www.drupal.org/developing/api/8/render/arrays/cacheability
Filed an issue for #2: #2478393: hook_page_(attachments|attachments_alter|top|bottom)() should specify the right cacheability metadata. Initial patch included.