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

CommentFileSizeAuthor
#7 allow_to_set_cache-2475749-7-interdiff.txt1.81 KBmbovan
#7 allow_to_set_cache-2475749-7.patch4.56 KBmbovan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,824 pass(es). View
#4 allow_to_set_cache-2475749-4-interdiff.txt805 bytesmbovan
#4 allow_to_set_cache-2475749-4.patch4.37 KBmbovan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,821 pass(es). View
#1 allow_to_set_cache-2475749-1.patch3.86 KBmbovan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,747 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mbovan’s picture

Status: Active » Needs review
FileSize
3.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,747 pass(es). View

Providing first patch. Added #cache tags, changed the exception messages and updated exception message in existing tests.

Where should we test this?

Wim Leers’s picture

Adding additional assertions to PageRenderTest seems sufficient.


In a follow-up, we should make all hook_page_attachments(), hook_page_attachments_alter(), hook_page_top() and hook_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).

Berdir’s picture

Status: Needs review » Needs work
mbovan’s picture

Status: Needs work » Needs review
FileSize
4.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,821 pass(es). View
805 bytes

Added assertion for the cache tags in PageRenderTest.

hass’s picture

For example, quickedit_page_attachments() only adds its asset library if the current user has the right permission

That is something I need for Google Analytics, too.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Tests/Common/PageRenderTest.php
@@ -54,6 +54,9 @@ function assertPageRenderHookExceptions($module, $hook) {
+    // Assert that hooks can set cache tags.
+    $this->assertEqual($page['#cache']['tags'], ['example']);

+++ b/core/modules/system/tests/modules/common_test/common_test.module
@@ -224,6 +224,7 @@ function common_test_cron() {
+  $page['#cache']['tags'] = ['example'];

@@ -248,6 +249,7 @@ function common_test_page_attachments_alter(array &$page) {
+  $page['#cache']['tags'] = ['example'];

Can 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.)

mbovan’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,824 pass(es). View
1.81 KB

Added cache context assertion. :)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This should be ready then, don't think testbot will fail on this.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Can 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!

  • alexpott committed 5b560a5 on 8.0.x
    Issue #2475749 by mbovan: Allow to set #cache metadata in...
hass’s picture

Is 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.

Status: Fixed » Closed (fixed)

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