Fatal error in Book navigation block for unpublished parent book item

Let's ensure we add the test coverage to ensure to not break it again.

@catch, @effulgentsia, @Cottser, @alexpott, and I discussed this issue today and agreed to handle it as a major issue per https://www.drupal.org/core/issue-priority#major-bug per these points specifically:

• Interfere with normal site visitors' use of the site (for example, content in the wrong language, or validation errors for regular form submissions), even if there is a workaround.
• Trigger a PHP error through the user interface, but only under rare circumstances or affecting only a small percentage of all users, even if there is a workaround.
• Render one feature unusable with no workaround.

This issue fits in those categories. If, however, it is possible for the site to end up in a completely unrecoverable state, then this issue should be repromoted to critical. Thanks!

Needs work because we need some tests :(

@szato
Feel free to ping me, if you need some help with that.

I just got hit by this one... does it look like the patch will make it into the next 8.1 release, or will we have to wait for Drupal 8.2?

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -752,4 +752,32 @@ public function testHookNodeLoadAccess() {
+
+    $this->drupalLogout();

Note: We don't need to logout at the end here, the site is destroyed afterwords anyway ... Let's skip that.

Hi @szato.

Thanks for your patch, A few nits:

1. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -752,4 +752,30 @@ public function testHookNodeLoadAccess() {
   +  /**
   +   * Tests the book navigation block when book is unpublished.
   +   *
   +   * There was a fatal error with "Show block only on book pages" block mode:
   +   * @see https://www.drupal.org/node/2743183
   +   */
   

   It is not necessary pointing to a specific issue (unless there is a TODO there), you just need to describe what you are testing.

2. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -752,4 +752,30 @@ public function testHookNodeLoadAccess() {
   +    $administratorUser = $this->drupalCreateUser(array('edit any book content', 'administer blocks', 'administer nodes', 'bypass node access'));
   

   There is already an adminUser check: $this->adminUser in that class.

3. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -752,4 +752,30 @@ public function testHookNodeLoadAccess() {
   +    $edit = [];
   

   We are mixing the old array syntax with the short syntax, usually in new methods we use the short syntax.

   So, can you please change all the array() to []?

4. I think this is not the best approach to fix this, with this patch if the top node of the book is unpublished we hide the entire Book Navigation block but if we are a user with the right permission (like an admin or if the node belongs to the user) we should be able to see this block even if the node is unpublished. I talked with Catch via IRC and he thinks to your approach is correct, so ignore this last point :-) Thanks!

Adding a condition for published nodes also fixes the issue for me on 8.1.x.

I have tested patch applied by @nielsvandermolen in #20, and it fixes the issue on 8.1.x.

looks good to me.

Thanks!

This error occurs because \Drupal\book\BookManager::doBookTreeCheckAccess() will set a tree to [] if the access is false.

One of the key parts of how we check book access is in \Drupal\book\BookManager::bookTreeCheckAccess()

      // @todo This should be actually filtering on the desired node status
      //   field language and just fall back to the default language.
      $nids = \Drupal::entityQuery('node')
        ->condition('nid', $nids, 'IN')
        ->condition('status', 1)
        ->execute();

So an alternative and perhaps better fix for this is to add something similar to the query in \Drupal\book\Plugin\Block\BookNavigationBlock::build(). What I'm saying is change:

$nid = $query->condition('nid', $node->book['bid'], '=')->execute();

to

$nid = $query
  ->condition('nid', $node->book['bid'], '=')
  ->condition('status', 1)
  ->execute();

Because whilst this query does access checks, block navigation always only display published content (never unpublished). Whether or not the users can view unpublished nodes is irrelevant. The advantage of doing the fix this way is that when this occurs we'll do way less because we wouldn't ever call the book tree building code.

The fix in #22 will work and maybe the sub-system maintainer will feel it is a better approach but I think it is worth considering the change outlined above.

In reviewing the patch I ran the test without the fix and it fails as expected - nice work on the test.

Works well and I think we all can agree on this solution. It seems the patch can still go in the 8.2.x branch because it is not a new feature.

All the fixes land first on 8.3.x and then we can backported them to 8.2.

But Yes I agree this should be on 8.2 as well.

Thanks!

Committed and pushed bcee34d to 8.3.x and 63b661d to 8.2.x. Thanks!

Eligible for 8.2.x because this is a straight up bug-fix with no new methods or possibly breaking changes. I spoke to @pwolanin about this change - he want to rewrite book module :) - he seemed okay with the change.