Problem/Motivation

STR:

  1. First create a new book add a series of child pages nested to as deep as 7 to 8 levels.
  2. Now create a second child page of the top-level book node and set of several child pages of that.
  3. When attempting to edit a node that has children that go deep enough, there is a fatal error:
    [Mon Jan 25 17:23:18.977405 2016] [:error] [pid 27538] [client 206.192.168.17:5717] PHP Fatal error:  Call to a member function label() on null in /var/www/html/core/modules/book/src/BookManager.php on line 414</li>
  <li>

the recurseTableOfContents function doesn't properly handle the case when it's pruned every node in a particular branch. E.g. it runs through this foreach loop, and every node is below the depth_limit. The break exits the loop, and proceeds through the function.

    foreach ($tree as $data) {
      if ($data['link']['depth'] > $depth_limit) {
        // Don't iterate through any links on this level.
        break;
      }
      if (!in_array($data['link']['nid'], $exclude)) {
        $nids[] = $data['link']['nid'];
      }
    }

It then crashes within this foreach loop, as it attempts to call Unicode::truncate without verifying if there are any nodes left.

   foreach ($tree as $data) {
      $nid = $data['link']['nid'];
      if (in_array($nid, $exclude)) {
        continue;
      }
      $toc[$nid] = $indent . ' ' . Unicode::truncate($nodes[$nid]->label(), 30, TRUE, TRUE);
      if ($data['below']) {
        $this->recurseTableOfContents($data['below'], $indent . '--', $toc, $exclude, $depth_limit);
      }
    }

Proposed resolution

add a check on or after the first foreach loop that returns from the function if there aren't any nodes.

    // Return to calling function if all links have been excluded due to depth
    if (empty($nids)) {
      return;
    }

Remaining tasks

Add tests

User interface changes

n/a

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#12 increment.txt1.44 KBpwolanin
#12 2662006-12.patch3.56 KBpwolanin
#12 2662006-12-test-only.patch2.58 KBpwolanin
#10 2662006-10-test-only.patch1.6 KBpwolanin
#10 2662006-10.patch2.58 KBpwolanin
#8 2662006-8.patch1004 bytespwolanin
#5 drupal-fix-fatal-error-for-large-book-hierarchies-2662006-05-8.0.x.patch570 bytesdavemybes
#4 drupal-fix-fatal-error-for-large-book-hierarchies-2662006-02-8.0.x.patch570 bytesdavemybes
BookManager.patch439 bytesjtyocum
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jtyocum created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Version: 8.0.1 » 8.0.x-dev
Status: Active » Needs review
Issue tags: +Needs tests

Let the testing begin.

Status: Needs review » Needs work

The last submitted patch, BookManager.patch, failed testing.

davemybes’s picture

davemybes CreditAttribution: davemybes commented
FileSize
drupal-fix-fatal-error-for-large-book-hierarchies-2662006-02-8.0.x.patch570 bytes

This happens in 8.0.3 as well. I literally just figured out the issue a few minutes ago and then found this post. Sweet! In my case, it was throwing the fatal for every 4th level book item. The patch works great for me. Apparently there should also be a unit test or simpletest written to cover this sort of failure, but that's beyond my current abilities, I'm afraid. I'm attaching a properly formatted (hopefully) patch, though.

davemybes’s picture

davemybes CreditAttribution: davemybes commented
FileSize
drupal-fix-fatal-error-for-large-book-hierarchies-2662006-05-8.0.x.patch570 bytes

Here's the same patch with the right comment number and submitted for testing.

davemybes’s picture

davemybes CreditAttribution: davemybes commented
Status: Needs work » Needs review
pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
Assigned: Unassigned » pwolanin

Will take a look at this

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
FileSize
2662006-8.patch1004 bytes

Compare to the Drupal 7 version:
https://api.drupal.org/api/drupal/modules%21book%21book.module/function/...

There was only one foreach loop, so the break; actually skipped everything - it's essentially the same as a return;

So, here's a slightly different version of the patch swapping that for a return, and avoiding doing the in_array() check 2x.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
Priority: Normal » Major
Issue summary: View changes

Since this is a fatal error in normal use, marking as major.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
Issue summary: View changes
FileSize
2662006-10.patch2.58 KB
2662006-10-test-only.patch1.6 KB

The test-only patch here is also the interdiff

Not sure about the test method naming.

Probably good to add some direct API calls to validate the return data also, as well as more web/UI testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2662006-10-test-only.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
Status: Needs work » Needs review
FileSize
2662006-12-test-only.patch2.58 KB
2662006-12.patch3.56 KB
increment.txt1.44 KB

Plus a little direct testing of the manager method

The last submitted patch, 12: 2662006-12-test-only.patch, failed testing.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard at Palantir.net commented

Test looks fine. The single line comment /** @var \Drupal\book\BookManagerInterface $manager */ look odd, though.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented

@agentrickard the @var comment is for hinting the variable type to an IDE.

There are a couple other uses already in that test file like:

      /** @var \Drupal\Core\Url $url */
      $url = $next->urlInfo();
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests 
  1. +++ b/core/modules/book/src/BookManager.php
@@ -397,7 +397,7 @@ protected function recurseTableOfContents(array $tree, $indent, array &$toc, arr
         // Don't iterate through any links on this level.
-        break;
+        return;
       }
       if (!in_array($data['link']['nid'], $exclude)) {

    I see, when we hit any link which is too depth in the tree, we don't need to iterate sibiling entries nor do anything with it. Maybe we could expand the doc a bit, to make this clear. 

  2. +++ b/core/modules/book/src/BookManager.php
@@ -408,7 +408,8 @@ protected function recurseTableOfContents(array $tree, $indent, array &$toc, arr
-      if (in_array($nid, $exclude)) {
+      // Check for excluded or missing node.
+      if (empty($nodes[$nid])) {

    I see, we have checked $exclude already.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Changed array() in the new test to short array syntax prior to commit. Otherwise looked great.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 43ea3d9 on 
    Issue #2662006 by pwolanin, incrn8, jtyocum: Fatal error when trying to...

  • catch committed 68bde24 on 
    Issue #2662006 by pwolanin, incrn8, jtyocum: Fatal error when trying to...

Status: Fixed » Closed (fixed)

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