Problem/Motivation

Until #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees landed, the Masquerade module was impossible to get to work correctly in Drupal 8. The Masquerade menu links contain a CSRF token and have a dynamic (uncacheable) menu link text (e.g. Stop masquerading as ). Due to the menu block's incorrect caching, this link wasn't updated dynamically as it should be.

Proposed resolution

Add an integration test covering this particular case, the problem was fixed in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, but more integration tests covering complex use cases are of course welcome :)

Remaining tasks

None.

User interface changes

None.

API changes

None.

Original report by andypost

Problem/Motivation

Currently blocks with menus are cached by default, varying only by a few hardcoded cache contexts. This is completely wrong.

@andypost is porting the Masquerade module to Drupal 8. The Masquerade menu links contain a CSRF token and have a dynamic (uncacheable) menu link text (e.g. Stop masquerading as ). Due to the menu block's incorrect caching, this link isn't updated dynamically as it should be.

Proposed resolution

Do not cache menu blocks. Menu blocks will be made cacheable in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it now just adds test coverage.
Issue priority Normal, because while its important its not majorly important.
Unfrozen changes Unfrozen because it only changes tests.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
1.22 KB

Let's see the tests

Status: Needs review » Needs work

The last submitted patch, 1: 2463659-menu-block-cache-1.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
676 bytes

We have tests, so just prevent caching and bubble up contexts

Wim Leers’s picture

andypost talked to me in IRC before filing this issue. (Updating the IS with that information.) Andy is porting the Masquerade module to Drupal 8. The Masquerade menu links contain a CSRF token and have a dynamic (uncacheable) menu link text (e.g. Stop masquerading as ). Due to the menu block's incorrect caching, this link isn't updated dynamically as it should be.


#2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags incorrectly introduced this:

  /**
   * {@inheritdoc}
   */
  protected function getRequiredCacheContexts() {
    // Menu blocks must be cached per URL and per role: the "active" menu link
    // may differ per URL and different roles may have access to different menu
    // links.
    return array('cache_context.url', 'cache_context.user.roles');
  }

Back then, in that issue, we (stupidly!) didn't realize that not all access checking is role-based. So whenever any menu link is cacheable per something else than URL or user roles… then it breaks. I've known this for a year, but surprisingly nobody has reported this, until now! (People are starting to build actual sites, port more modules, so now this bug is being noticed.)

We've been not wanting to make this change to not make D8 slower, but we should first and foremost make Drupal 8 correct. This caching is effectively breaking correctness, as @andypost's problem demonstrates. And it comes with an important consequence: it makes it impossible to port certain contributed modules.

andypost’s picture

That's only a half of problem, the second one in related issue #2463753: [regression] Do not bypass route access with 'link to any page' permissions for menu links
So here's a test from that, let's see how caching affects this

PS: I think it would be easy to add title callback with counter.

andypost’s picture

FileSize
5.35 KB
6.83 KB

#5 has wrong assertion, so fixed
this test exactly for that, still thinking to extend the title of link with callback

andypost’s picture

It looks that test outdated, so maybe to clean it here or related issue?

The last submitted patch, 6: 2463659-test-only-6.patch, failed testing.

andypost’s picture

Category: Task » Bug report
Wim Leers’s picture

Fabianx’s picture

FileSize
5.35 KB

I know Wim wanted to close that, but I want to see if the test-only patch now passes, we could still add the test coverage here.

Wim Leers’s picture

Ohhh, good call!

Wim Leers’s picture

Title: Do not cache menu blocks by default » Regression test coverage: integration test for an uncacheable menu link that depends on session data
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Hurray! We didn't screw up :)

Fabianx’s picture

Category: Bug report » Task
Priority: Major » Normal
Issue summary: View changes

Classified as normal task and added beta eval that test only changes are unfrozen.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2463659-test-only-6.patch, failed testing.

Status: Needs work » Needs review

Fabianx queued 11: 2463659-test-only-6.patch for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2463659-test-only-6.patch, failed testing.

Status: Needs work » Needs review

Fabianx queued 11: 2463659-test-only-6.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2463659-test-only-6.patch, failed testing.

andypost’s picture

So test now fails but in Drupal\system\Tests\Routing\RouterTest

Wim Leers’s picture

Issue tags: +Needs reroll

Weird.

dawehner queued 6: 2463659-6.patch for re-testing.

The last submitted patch, 6: 2463659-6.patch, failed testing.

Wim Leers’s picture

Issue tags: +Novice

We should do a reroll and land this.

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.35 KB
6.52 KB

Rerolled and fixed the filename of test-only.

andypost’s picture

hm, test-only passes, so no idea how to proceed here

Fabianx’s picture

Status: Needs review » Needs work

#27: The problem was already fixed, this is just the test coverage.

RTBC for the test-only patch.

Could we upload only that patch, please to not confuse core committers?

Thanks!

andypost’s picture

Status: Needs work » Needs review
FileSize
665 bytes
6 KB

I'd like to see fix for book navigation in the patch because this was broken before fixed (see interdiff - the needed part of non-test-only patch)

  1. +++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
    @@ -182,10 +182,8 @@ public function build() {
    -    // The "Book navigation" block must be cached per role and book navigation
    -    // context.
    +    // The "Book navigation" block varies by the book navigation cache context.
         return [
    -      'user.roles',
    

    since #4

Fabianx’s picture

+++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
@@ -182,10 +182,8 @@ public function build() {
-    // The "Book navigation" block must be cached per role and book navigation
-    // context.
+    // The "Book navigation" block varies by the book navigation cache context.
     return [
-      'user.roles',
...
     ];

Can we do that in a follow-up, please? It does not match the beta eval of test only changes and should go in independently as a normal bug fix.

andypost’s picture

..otoh maybe better to file follow-up to make proper cache context for book navigation block because it depends on node access not roles

andypost’s picture

FileSize
5.35 KB
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, given that this originally once failed, this is fine to go in.

Wim Leers’s picture

Agreed with Fabian's reason for pushing back, and agreed with RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
--- a/core/modules/system/src/Tests/Menu/MenuTranslateTest.php
+++ b/core/modules/system/src/Tests/Menu/MenuTranslateTest.php

@@ -23,12 +22,26 @@ class MenuTranslateTest extends WebTestBase {
   public function testMenuTranslate() {

This looks like it is testing menu translation but it is doing nothing of the sort - let's rename it to something better.

Wim Leers’s picture

Issue tags: +Needs reroll
andypost’s picture

Status: Needs work » Needs review
FileSize
684 bytes
5.4 KB

here it is

Wim Leers’s picture

Shouldn't we also rename the test class?

andypost’s picture

FileSize
791 bytes
5.74 KB

Value '' is identical to value '/checkout/search/dvvtnsuj'. Other SearchConfigSettingsFormTest.php 276 Drupal\search\Tests\SearchConfigSettingsFormTest->testMultipleSearchPages()

fail was unrelated

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

googletorp’s picture

Issue tags: -Needs reroll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2463659-menu-block-cache-39.patch, failed testing.

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2463659-menu-block-cache-39.patch, failed testing.

Wim Leers’s picture

Issue tags: +php-novice, +Needs reroll
googletorp’s picture

Rerolled.

googletorp’s picture

Status: Needs work » Needs review

Forgot to update status.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reroll!

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/menu_test/src/Access/AccessCheck.php
@@ -0,0 +1,33 @@
+    if (!isset($_SESSION['menu_test'])) {
+      $result = AccessResult::allowed();
+    }
+    else {
+      $result = AccessResult::allowedIf($_SESSION['menu_test'] < 2);
+    }
+    return $result->setCacheMaxAge(0);

I don't get that, isn't the point of cache contexts that we vary by the session cache context and not disable caching directly? At least the issue title seems to say so as well.
We should maybe have something on top which tests with max age 0, don't you think so?

Wim Leers’s picture

This predates the session cache context and really is just an example. Replace $_SESSION with $_GLOBALS if you want. The point is that it's something completely uncacheable, and that it correctly bubbles, hence ensuring the menu is re-rendered on every page request.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I must be missing something but this issue doesn't add any assertions... which is quite surprising for a test only change.

Wim Leers’s picture

Ugh, good catch, we lost the assertions it in the #2463659-47: Regression test coverage: integration test for an uncacheable menu link that depends on session data reroll…

This is a straight reroll from #39, to ensure nothing is lost.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I just did a straight reroll, so it should be okay for me to re-RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is a test only change and therefore permitted in beta. Committed 9ba65aa and pushed to 8.0.x. Thanks!

  • alexpott committed 9ba65aa on 8.0.x
    Issue #2463659 by andypost, Wim Leers, rpayanm, Fabianx, googletorp:...
dawehner’s picture

+++ b/core/modules/system/tests/modules/menu_test/src/Access/AccessCheck.php
@@ -0,0 +1,33 @@
+    if (!isset($_SESSION['menu_test'])) {
...
+      $result = AccessResult::allowedIf($_SESSION['menu_test'] < 2);

+++ b/core/modules/system/tests/modules/menu_test/src/TestControllers.php
@@ -39,6 +39,17 @@ public function test2() {
+    if (!isset($_SESSION['menu_test'])) {
...
+    }
+    $_SESSION['menu_test']++;

Its sad that noone asked why we cannot use $request->GetSession()

Status: Fixed » Closed (fixed)

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