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
When attempting to delete a book parent with children through the UI:
Recoverable fatal error: Argument 1 passed to Drupal\book\BookManager::updateOutline() must implement interface Drupal\node\NodeInterface, array given, called in /path/core/modules/book/src/BookManager.php on line 442 and defined in Drupal\book\BookManager->updateOutline() (line 250 of core/modules/book/src/BookManager.php).
on second attempt, this error isn't thrown, but then when viewing a child page:
Fatal error: Call to a member function access() on a non-object in /path/core/modules/book/book.module on line 239
To reproduce:
- Enable the book module
- Add a top-level book node
- Add one or more child book nodes
- Attempt to delete the parent node, note the exception. Try again and it succeeds.
- Visit one of the child nodes and note the fatal error.
Proposed resolution
- Addressing the first issue resolves the second
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Bug because fatal errors and exceptions |
---|---|
Issue priority | Major because removing a parent book node leaves the child nodes inaccessible. Logic is in place to ensure this doesn't happen, but it got left behind due to a lack of tests. |
Comment | File | Size | Author |
---|---|---|---|
#51 | Child node created before applying patch was deleted without erroe in first attempt.PNG | 61.55 KB | swati_qa |
#51 | Parent Node was deleted in first attempt without error.PNG | 56.27 KB | swati_qa |
#51 | Patch applied.PNG | 43.93 KB | swati_qa |
#51 | Retest patches created.PNG | 55.69 KB | swati_qa |
#51 | Created Nodes before applying patch.PNG | 69.95 KB | swati_qa |
Comments
Comment #1
jhedstromNot a complete fix yet, but here's a test that illustrates the issue, and the beginnings of a fix.
Comment #4
jhedstromThis gets things back to working as expected.
Comment #5
jhedstromComment #7
jhedstromAdding a beta phase evaluation.
Comment #8
jhedstromComment #9
pwolanin CreditAttribution: pwolanin at Acquia commentedminor, please use route names instead of paths when adding test code:
I think you might be able also to use the base for ID like: book_form_node_form_alter ?
So at least you won't run this code in every form
Comment #10
jhedstromI didn't know about the ability to pass a route to
drupalGet()
, that makes much more sense. Also changing to the base form works, and greatly simplifies the function.Comment #11
pwolanin CreditAttribution: pwolanin at Acquia commentedThese 2 lines should also use the routes via a Url object
like:
Comment #12
jhedstromHere's a patch that fixes the overlooked drupalGet calls mentioned in #11.
Comment #15
pwolanin CreditAttribution: pwolanin at Acquia commentedChanges look good, assuming the tests are still green.
Comment #16
alexpottThis loss of specificity in the form alter is worrying. I think we need to include the operation in the base form id.
Comment #17
pwolanin CreditAttribution: pwolanin at Acquia commented@alexpottt - hmm, good point. But I don't think there is an option to include the operation? The form ID includes the bundle name which is too specific.
I had assumed that was delete specific, but I see it's more generic:
I'm not seeing an obvious way to get the operation other than looking at the form ID?
Comment #18
alexpottLet's add the p tags with
#prefix
and#suffix
.Comment #19
pwolanin CreditAttribution: pwolanin at Acquia commentedPer discussion with alexpott, fixing the base for ID to include the operation.
Comment #22
pwolanin CreditAttribution: pwolanin at Acquia commentedtest failed on settings.php, so I think it was a bot issue
Comment #24
jhedstromI think the patch in #19 has addressed Alex's feedback.
This seems like a beneficial change. Does it need a CR?
Comment #27
lokapujyaSimple reroll.
Comment #28
lokapujyaJust want to see the test-only again.
Comment #30
jhedstromI think this is good now. If we need a CR, I can add one as mentioned in #24.
Comment #38
lokapujyaSo if the book is deleted the direct child pages become a book (rather than parent of "none") which is the same as D7.
Note that node/{nid}/outline/remove is still not allowed (Access Denied) and has a test.
I don't see any reason for a CR.
RTBC from my testing.
Comment #40
Truptti CreditAttribution: Truptti at Axelerant commentedVerified the patch "2482857-27.patch" mentioned in #27, the child nodes are now accessible after applying the patch
Steps performed to verify the patch:
1.Created a Book page "Test Page"
2.Created child nodes "Test page 1" and Test page 2".
3.Deleted the "Test Page" , while deleting got an error "The website encountered an unexpected error. Please try again later." on a second attempt it got deleted
4.Accessed "Test page 1" and "Test page 2" and got error "( ! ) Fatal error: Call to a member function access() on a non-object in C:\wamp\www\drupal\core\modules\book\book.module on line 233"
5.Applied the patch and repeated steps 1 to 4 with book names as "New Page" and child nodes as "Child page 1" and "Child page 2"
6."New Page" got deleted without any exception error and I am able to access "Child page 1" and "Child page2" without any fatal error.
7.However still the fatal error is displayed for "Test page 1" and "Test page 2" (the child pages created before applying the patch)
So if the point no 7 is meant to be like that then the issue can be marked as RTBC from the testing.
Comment #41
jhedstromComment #42
pwolanin CreditAttribution: pwolanin at Acquia commenteddoesn't apply
Comment #43
lindzeng CreditAttribution: lindzeng commentedPatch was applied with fuzz.
Comment #44
lindzeng CreditAttribution: lindzeng commentedComment #45
jhedstromBack to RTBC.
Comment #46
alexpottThis is an API change. I don't think we can do the fix this way now. I think we'll need to just change the book_form_node_delete_confirm_alter() to book_form_node_confirm_alter() and test the operation.
Instead use book_type_is_allowed()
Why is this change necessary to fix the bug?
Is this change of the child bid explicitly tested?
Not in scope.
Comment #47
jhedstromThis patch addresses #46:
Comment #48
alexpottRe #47.3 I don't think we should be making UI changes in a bugfix issue. It was this way in Drupal 7 when book deletion worked. I'm not going to push on #47.5 other than to say that on bigger issues it is these type of out of scope changes that makes large patches hard to review because you have to go back and think so why is that change relevant to the scope of this issue.
Is this used?
Comment #50
jhedstromThis removes the warning class from the message.
I'd happily do DX docs follow-ups, but test-only patches are very hard to get reviewed (historically anyway).
Comment #51
swati_qa CreditAttribution: swati_qa at Axelerant commentedVerified the patch "2482857-50.patch" mentioned in #50.
Steps performed to verify the patch:
1. Created a Book page "Parent Node"
2. Created child nodes "Child Node 1" and "Child Node 2".
3. Deleted the "Parent Node" , while deleting got an error "The website encountered an unexpected error. Please try again later." on a second attempt it got deleted
4. Accessed "Child Node 1" and "Child Node 2" and didn't get any error.
5. Applied the patch and repeated steps 1 to 4 with book names as "Patch_retest_Parent Node" and child nodes as "Patch retest_Child Node" and "Patch retest_Child Node 2"
6. "Patch_retest_Parent Node" got deleted without any exception error and I am able to access "Patch retest_Child Node" and "Patch retest_Child Node 2" without any fatal error.
7. No error is displayed for "Child Node 1" (the child page created before applying the patch).
So, issue can be marked as "PASSED" for errors and warnings.
Comment #52
swati_qa CreditAttribution: swati_qa at Axelerant commentedComment #53
alexpottCommitted a224bb8 and pushed to 8.0.x and 8.1.x. Thanks!