Result of book_node_load() should not vary depending on user permissions

While it is problematic, the description here is not quite correct.

persistent entity caching does include hook_node_load(), only hook_node_storage_load(), hook_node_load() is still called once per request. But of course, if the user switches, then it is broken as well.

Started to work on this issue: Added some multiloading for nodes in the book, and ported over some of the menu link improvements.

persistent entity caching does include hook_node_load(), only hook_node_storage_load(), hook_node_load() is still called once per request. But of course, if the user switches, then it is broken as well.

Well, in general I think switching users, at least manually is an edge case.
In general I think its a sign that we really can't just authenticate after routing, i'm still convinced that lazy-authentication ensures that you never deal with the wrong user.

Some work.

Now, some propery work.

New, much more simpler approach.

This time with a test as well.

+++ b/core/modules/book/src/BookOutlineStorage.php
@@ -47,11 +47,15 @@ public function hasBooks() {
+    if ($access) {
+      $query->addTag('node_access');
+    }
+
     $query->addMetaData('base_table', 'book');

Would make sense to add the metadata inside the if as well, as that belongs together?

Valid point, thank you!

Doh, I was sure I fixed that already.

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -660,4 +668,35 @@ public function testAdminBookListing() {
+    // Ensure that the loaded book in hook_node_lode() does NOT depend on the

typo: hook_node_lode()

Ha, a lode though seems to be a real existing thing: http://en.wikipedia.org/wiki/Lode

- * @return array - * Array of loaded book items. + * @return array Array of loaded book items. + * Array of loaded book items. */
Other Than this, rtbc

Looks good, tests fine

+++ b/core/modules/book/src/BookManager.php
@@ -695,7 +695,7 @@ public function loadBookLink($nid, $translate = TRUE) {
+    $result = $this->bookOutlineStorage->loadMultiple($nids, $translate);

+++ b/core/modules/book/src/BookOutlineStorage.php
@@ -47,12 +47,16 @@ public function hasBooks() {
+  public function loadMultiple($nids, $access = TRUE) {

This looks super odd - why are we passing boolean about translation into a variable about access?

Back to needs review for #24 - if this is correct perhaps a comment in the code would be nice.

This looks super odd - why are we passing boolean about translation into a variable about access?

Coming from the good old days of from menu.inc we have this odd thing called translating.
Translating is pretty much access checking though these days (

* Provides book loading, access control and translation.

See the following code:

  public function bookLinkTranslate(&$link) {
    $node = NULL;
    // Access will already be set in the tree functions.
    if (!isset($link['access'])) {
      $node = $this->entityManager->getStorage('node')->load($link['nid']);
      $link['access'] = $node && $node->access('view');
    }
    // For performance, don't localize a link the user can't access.
    if ($link['access']) {
      // @todo - load the nodes en-mass rather than individually.
      if (!$node) {
        $node = $this->entityManager->getStorage('node')
          ->load($link['nid']);
      }
      // The node label will be the value for the current user's language.
      $link['title'] = $node->label();
      $link['options'] = array();
    }
    return $link;
  }

Having a better name would require API changes, let's avoid this here. So back to RTBC

Back to Alex.

  /**
   * Loads multiple book entries.
   *
   * @param int[] $nids
   *   An array of nids to load.
   *
   * @param bool $translate
   *   If TRUE, set access, title, and other elements.
   *
   * @return array[]
   *   The book data of each node keyed by NID.
   */
  public function loadBookLinks($nids, $translate = TRUE);

The documentation for loadBookLinks looks like it could do with improving considering how this issue changes things. Since this is now about respecting access and therefore has security implications.

There we go.

+++ b/core/modules/book/src/BookManagerInterface.php
@@ -57,6 +57,14 @@ public function getActiveTrailIds($bid, $link);
+   *
+   * On top of it, loadBookLinks

This looks orphaned - should it be a @see?

This looks orphaned - should it be a @see?

Yeah sure, let's add a @see

Back we go

Let's do a better rebase

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 2874862 and pushed to 8.0.x. Thanks!