Book module breadcrumb remains cached when node title (or access) changes

In addition, it seems likely that if there is some custom node access scheme, the breadcrumb would be cached and could be shown to users that don't have access.

Is this critical because of this?

Patch looks good.

NW for tests.

Test coverage looks good. Just nits for it. Upon fixing the nits & merging #2, this is RTBC as far as I'm concerned.

1. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -754,4 +754,52 @@ public function testHookNodeLoadAccess() {
   +    /*
   

   Super nit: missing trailing asterisk.

2. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -754,4 +754,52 @@ public function testHookNodeLoadAccess() {
   +    $got_breadcrumb = array();
   ...
   +    $got_breadcrumb = array();
   

   Nit: []

3. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -754,4 +754,52 @@ public function testHookNodeLoadAccess() {
   +  }
    }
   

   Nit: needs newline in between.

#2485683: REST entity resource missing entity & field access cacheability metadata is an example of such a test, that just went in.

So, am I missing somethign - seems like the access result cache tags are not being propagated?

I don't see any cache tag assertions in #13/#14.

+++ b/core/modules/book/tests/modules/book_breadcrumb_test/book_breadcrumb_test.module
@@ -0,0 +1,26 @@
+  $access->addCacheableDependency($config);

Since the access depends on the node title shouldn't this add the $node as a cacheable dependency too?

Just confirming the patch in #13 works while manually testing.

Though possibly we should add that as a cacheable dependency even if access is not allowed?

Yes it needs to be a dependency either way.

Though possibly we should add that as a cacheable dependency even if access is not allowed?

Yes.

But it's already doing that:

+++ b/core/modules/book/tests/modules/book_breadcrumb_test/book_breadcrumb_test.module
@@ -0,0 +1,26 @@
+  if ($config->get('hide') && $node->getTitle() == "you can't see me" && $operation == 'view') {
+    $access = new AccessResultForbidden();
+  }
+  else {
+    $access = new AccessResultNeutral();
+  }
+  $access->addCacheableDependency($config);
+  return $access;

Unless I'm overlooking some code? Or some other code in book module is short-circuiting this incorrectly?

Oops, I misread. Yes, the access result's cacheability metadata should already include the information it depends on, so that would indicate a bug in the access check logic involved there.

The test implementation only sets $config as a dependency, but it also varies on $node->title(), so surely should set $node as a dependency too.

Here's a patch that does that, untested though...

OK that fixes 2 out of 4 assertions.

I think the other issue is that static config objects don't have cache tags by default, so

+++ b/core/modules/book/src/Tests/BookBreadcrumbTest.php
@@ -0,0 +1,207 @@
+    $config->set('hide', TRUE)->save();

This is not actually invalidating any cache tags.

However https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21R...

This should set max age to 0 given static config doesn't implement CacheableDependencyInterface, so you'd expect it to still pass.

But, we don't bubble max age yet per #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache.

So the $config cacheable dependency does nothing to the breadcrumbs.

Since this is only for a test, I've changed it to use state and a raw cache tag. Tests pass locally for me now.

static config objects don't have cache tags by default

Well, no, look at \Drupal\Core\Config\Config::save():

    $this->storage->write($this->name, $this->data);
    if (!$this->isNew) {
      Cache::invalidateTags($this->getCacheTags());
    }

So I don't think #28 is a good idea, it makes the test depend on state, which indeed by definition is uncacheable.

Doh completely forgot that then missed it when looking at this.

That snippet you posted explains the test fail though - because the isNew() check will fail there - the test is the first time it gets saved so the cache tag never gets invalidated.

This should pass, interdiff against #26.

Patch fail...

+++ b/core/modules/book/src/Tests/BookBreadcrumbTest.php
@@ -0,0 +1,210 @@
+    // Because the configuration isn't provided in config/install, save this
+    // twice to ensure the cache tag is invalidated.

OMG!!!! I don't yet understand how this can happen. But I think this should happen in setUp() then, because this is just extremely confusing.

It would probably be least confusing if we provided the default config in config/install with a value other than 'hide' - then it'll be installed when the module is, and the first save in the test will be an update instead of insert with no workaround in the test.

But this is also partly why I wanted to use state + cache tags because it's a fair bit of boilerplate just to get the access result to change between requests.

If no-one gets to it overnight I'll have a look tomorrow my time though.

It would probably be least confusing if we provided the default config in config/install with a value other than 'hide' - then it'll be installed when the module is, and the first save in the test will be an update instead of insert with no workaround in the test.

The value wouldn't even have to be something else than hide? Because Config isn't smart enough to not save anything if no actual changes are made?

So, yeah, +1 to putting default config in config/install. Not doing that is really the source of all confusion here AFAICT!

The value wouldn't even have to be something else than hide? Because Config isn't smart enough to not save anything if no actual changes are made?

The test relies on it not being 'hide' until it gets saved, but yeah otherwise this would be fine. Shifting to needs work for the default config change.

Okay, then yes, let the default installed config set it to something other than 'hide'.

Phew, glad that it was just that.

#2040135: Caches dependent on simple config are only invalidated on form submissions introduced the code quoted in #29. I thought it was to not do unnecessary invalidations while installing. But turns out it was @Berdir that suggested this in #2040135-69: Caches dependent on simple config are only invalidated on form submissions. Because it's consistent with how we do cache tag invalidation for content entities — see \Drupal\Core\Entity\Entity::invalidateTagsOnSave():

    if ($update) {
      // An existing entity was updated, also invalidate its unique cache tag.
      $tags = Cache::mergeTags($tags, $this->getCacheTagsToInvalidate());
    }

AFAICT that still makes sense.

Here's the default config and reverting #31.

Perfect.

Committed 70ca793 and pushed to 8.0.x and 8.1.x. Thanks!

At first I looked at the new config file added by the test module and wondered why it was not state but the test is neat as it uses the config to add a cacheable dependencies.

diff --git a/core/modules/book/src/Tests/BookBreadcrumbTest.php b/core/modules/book/src/Tests/BookBreadcrumbTest.php
index 2c126aa..a7b9684 100644
--- a/core/modules/book/src/Tests/BookBreadcrumbTest.php
+++ b/core/modules/book/src/Tests/BookBreadcrumbTest.php
@@ -62,7 +62,7 @@ protected function setUp() {
    *
    * @return \Drupal\node\NodeInterface[]
    */
-  function createBreadcrumbBook() {
+  protected function createBreadcrumbBook() {
     // Create new book.
     $this->drupalLogin($this->bookAuthor);
 
@@ -105,7 +105,7 @@ function createBreadcrumbBook() {
    * @return \Drupal\node\NodeInterface
    *   The created node.
    */
-  function createBookNode($book_nid, $parent = NULL) {
+  protected function createBookNode($book_nid, $parent = NULL) {
     // $number does not use drupal_static as it should not be reset
     // since it uniquely identifies each call to createBookNode().
     static $number = 0; // Used to ensure that when sorted nodes stay in same order.

Added visibility to a couple of functions where it was missing.

diff --git a/core/modules/book/src/Tests/BookBreadcrumbTest.php b/core/modules/book/src/Tests/BookBreadcrumbTest.php
index a7b9684..7080a27 100644
--- a/core/modules/book/src/Tests/BookBreadcrumbTest.php
+++ b/core/modules/book/src/Tests/BookBreadcrumbTest.php
@@ -61,6 +61,7 @@ protected function setUp() {
    * Creates a new book with a page hierarchy.
    *
    * @return \Drupal\node\NodeInterface[]
+   *   The created book nodes.
    */
   protected function createBreadcrumbBook() {
     // Create new book.
@@ -81,13 +82,13 @@ protected function createBreadcrumbBook() {
      *  |- Node 6
      */
     $nodes = array();
-    $nodes[0] = $this->createBookNode($book->id()); // Node 0.
-    $nodes[1] = $this->createBookNode($book->id(), $nodes[0]->id()); // Node 1.
-    $nodes[2] = $this->createBookNode($book->id(), $nodes[0]->id()); // Node 2.
-    $nodes[3] = $this->createBookNode($book->id(), $nodes[2]->id()); // Node 3.
-    $nodes[4] = $this->createBookNode($book->id(), $nodes[3]->id()); // Node 4.
-    $nodes[5] = $this->createBookNode($book->id(), $nodes[4]->id()); // Node 5.
-    $nodes[6] = $this->createBookNode($book->id()); // Node 6.
+    $nodes[0] = $this->createBookNode($book->id());
+    $nodes[1] = $this->createBookNode($book->id(), $nodes[0]->id());
+    $nodes[2] = $this->createBookNode($book->id(), $nodes[0]->id());
+    $nodes[3] = $this->createBookNode($book->id(), $nodes[2]->id());
+    $nodes[4] = $this->createBookNode($book->id(), $nodes[3]->id());
+    $nodes[5] = $this->createBookNode($book->id(), $nodes[4]->id());
+    $nodes[6] = $this->createBookNode($book->id());
 
     $this->drupalLogout();
 
@@ -106,9 +107,10 @@ protected function createBreadcrumbBook() {
    *   The created node.
    */
   protected function createBookNode($book_nid, $parent = NULL) {
-    // $number does not use drupal_static as it should not be reset
-    // since it uniquely identifies each call to createBookNode().
-    static $number = 0; // Used to ensure that when sorted nodes stay in same order.
+    // $number does not use drupal_static as it should not be reset since it
+    // uniquely identifies each call to createBookNode(). It is used to ensure
+    // that when sorted nodes stay in same order.
+    static $number = 0;
 
     $edit = array();
     $edit['title[0][value]'] = str_pad($number, 2, '0', STR_PAD_LEFT) . ' - SimpleTest test node ' . $this->randomMachineName(10);

Fixed some coding standards in the test. The standards are: Comments may not appear after statements, Return comment must be on the next line.