Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
STR:
- First create a new book add a series of child pages nested to as deep as 7 to 8 levels.
- Now create a second child page of the top-level book node and set of several child pages of that.
- 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
Comment | File | Size | Author |
---|---|---|---|
#12 | increment.txt | 1.44 KB | pwolanin |
#12 | 2662006-12.patch | 3.56 KB | pwolanin |
#12 | 2662006-12-test-only.patch | 2.58 KB | pwolanin |
#10 | 2662006-10-test-only.patch | 1.6 KB | pwolanin |
#10 | 2662006-10.patch | 2.58 KB | pwolanin |
Comments
Comment #2
mikeryanLet the testing begin.
Comment #4
davemybes CreditAttribution: davemybes commentedThis 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.
Comment #5
davemybes CreditAttribution: davemybes commentedHere's the same patch with the right comment number and submitted for testing.
Comment #6
davemybes CreditAttribution: davemybes commentedComment #7
pwolanin CreditAttribution: pwolanin as a volunteer commentedWill take a look at this
Comment #8
pwolanin CreditAttribution: pwolanin as a volunteer commentedCompare 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 areturn;
So, here's a slightly different version of the patch swapping that for a return, and avoiding doing the in_array() check 2x.
Comment #9
pwolanin CreditAttribution: pwolanin as a volunteer commentedSince this is a fatal error in normal use, marking as major.
Comment #10
pwolanin CreditAttribution: pwolanin as a volunteer commentedThe 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.
Comment #12
pwolanin CreditAttribution: pwolanin as a volunteer commentedPlus a little direct testing of the manager method
Comment #14
agentrickardTest looks fine. The single line comment
/** @var \Drupal\book\BookManagerInterface $manager */
look odd, though.Comment #15
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:
Comment #16
dawehnerI 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.
I see, we have checked
$exclude
already.Comment #17
catchChanged 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!