Improve error handling/messages when exporting books

Is that the entire exception message?

The comment for this generic exception is: "Thrown when the node was not attached to a book."

Been bitten by this too.

I wonder why we throw such a hard exception here, can't we just simply return an empty array (or whatever we need there), it's so harsh for users.

I would be nice if the exception would say which node is throwing the exception too, or logging it at least.
Another option would be to harden the access check to check on $node->book and if it doesn't exist return access denied for example.

The way I could reproduce it was because I was printing the export link manually without checking if $node->book existed. The user was then presented with a 500, but this could be nicer.

Added steps to reproduce which requires knowing the route book/export/html. I do not believe this is accessible via any UI links.

Tests only. (there is some excess lines in there, ignore the last two new lines)

Add assertion to confirm the warning message.

Applied patch #12 successfully

Before applying the patch, when we create any content type other than Book (Book module is enabled) and try to export that node by going to url like /book/export/html/[node-ID] the website will encounter an error. We can check the error by looking in the log messages.

Exception error before patch

After the patch, we won't get an error like "Website encountered an error". The exception is handled and we will get a "Page not found" warning message.

Page not found warning after the patch

Logged warning message after the patch

After adding patch #12, its showing me that the content cannot be exported.
'New Updated Test article title is not in a book and cannot be exported. '

This is looking good

I looked into whether outputting the node title is a security issue here, but the route already does access checking for the node.

I'm wondering if we should instead be adding an access callback that checks the node is book-enabled, and let it be handled higher up the stack?

1. +++ b/core/modules/book/tests/src/Functional/BookTest.php
   @@ -242,6 +242,16 @@ public function testBookExport() {
   +    // Make sure we get a 404 on a nodes not in any book.
   

   should this be 'any nodes' or 'a node'?

2. +++ b/core/modules/book/tests/src/Functional/BookTest.php
   @@ -242,6 +242,16 @@ public function testBookExport() {
   +    $node = $this->drupalCreateNode(['type' => 'article', 'title' => 'Book-not-in-book']);
   +    $this->drupalGet('book/export/html/' . $node->id());
   +    $this->assertSession()->statusCodeEquals(404);
   +    $this->assertSession()->pageTextContains('Book-not-in-book is not in a book and cannot be exported');
   

   this looks to duplicate the previous test - if so - we don't need two tests for the same thing

The attached patch makes tiny changes to address 17.1 and 17.2

+    // Make sure we get a 404 on nodes not in any book.

+    $node = $this->drupalCreateNode(['type' => 'book', 'title' => 'Book-not-in-book']);

To try to answer the thought:
I'm wondering if we should instead be adding an access callback...

For simplicity's sake, I think it is better to do the node type check here in the book module
rather trying to insert it into the routing system.

/book/export/html/[nid]
calls BookController::bookExport
calls the book.export service
calls BookExport

The current patch does the check in BookController.

change to 'needs review' for testing

Fixed minor CS error message.

Applied patch #21 and it works fine.

Before patch:

After patch:

RTBC +1

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Verified this issue exists on D10

On Drupal 10.1.x with a standard install
Follow the steps in the IS and confirm I get the fatal error
Apply fix and follow steps again and I get a 404

Code looks fine. Triggered for D10 tests.

Patch #21 passed 10.1 tests and review was done in #27

Good job!

This Issue Summary does not have a proposed resolution. How is the error handling being improved? It really helps the reviewer/committer to know what it expected.

I see that patch is being re-tested on a unsupported version of Drupal. Let's get that updated.

Finally, I looked at the patch I don't think that #17.2 has been addressed. We are testing twice that a non book node results in a 404. It would also be easier to read if the arrays were on multiple lines.

This extension is being deprecated, see #3376070: [Meta] Tasks to deprecate Book module. It will be removed from core and moved to a contrib project, #3376101: [11.x] [Meta] Tasks to remove Book.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

This issue may be re-opened if it can be considered critical, If unsure, re-open the issue and ask in a comment.