Cannot delete a book parent

minor, please use route names instead of paths when adding test code:

+     $this->drupalGet('node/' . $this->book->id() . '/delete');

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

These 2 lines should also use the routes via a Url object

+     $this->drupalPostForm('node/' . $this->book->id() . '/delete', [], t('Delete'));
+     $this->drupalGet('node/' . $nodes[0]->id());

like:

    $url = Url::fromRoute('entity.node.delete_form', ['node' =>$this->book->id()])
    $this->drupalPostForm($url, [], t('Delete'));
   $url = Url::fromRoute('entity.node.canonical', ['node' =>$nodes[0]->id()])
    $this->drupalGet($url);

Changes look good, assuming the tests are still green.

+++ b/core/modules/book/book.module
@@ -335,18 +335,22 @@ function book_node_prepare_form(NodeInterface $node, $operation, FormStateInterf
-function book_form_node_delete_confirm_alter(&$form, FormStateInterface $form_state) {
-  $node = Node::load($form['nid']['#value']);
+function book_form_node_confirm_form_alter(&$form, FormStateInterface $form_state) {

This loss of specificity in the form alter is worrying. I think we need to include the operation in the base form id.

@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:

abstract class EntityConfirmFormBase extends EntityForm implements ConfirmFormInterface {

  /**
   * {@inheritdoc}
   */
  public function getBaseFormId() {
    return $this->entity->getEntityTypeId() . '_confirm_form';
  }

I'm not seeing an obvious way to get the operation other than looking at the form ID?

+++ b/core/modules/book/book.module
@@ -335,18 +335,22 @@ function book_node_prepare_form(NodeInterface $node, $operation, FormStateInterf
-      '#markup' => '<p>' . t('%title is part of a book outline, and has associated child pages. If you proceed with deletion, the child pages will be relocated automatically.', array('%title' => $node->label())) . '</p>',
+      '#markup' => '<p class="messages messages--warning">' . t('%title is part of a book outline, and has associated child pages. If you proceed with deletion, the child pages will be relocated automatically.', array('%title' => $node->label())) . '</p>',

Let's add the p tags with #prefix and #suffix.

Per discussion with alexpott, fixing the base for ID to include the operation.

test failed on settings.php, so I think it was a bot issue

Simple reroll.

Just want to see the test-only again.

So 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.

Verified 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.

doesn't apply

Patch was applied with fuzz.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
   @@ -20,7 +20,7 @@
   -    return $this->entity->getEntityTypeId() . '_confirm_form';
   +    return $this->entity->getEntityTypeId() . '_' . $this->getOperation() . '_confirm';
   

   This 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.

2. +++ b/core/modules/book/book.module
   @@ -323,18 +323,24 @@ function book_node_prepare_form(NodeInterface $node, $operation, FormStateInterf
   +  $types = \Drupal::config('book.settings')->get('allowed_types');
   ...
   +  if (!in_array($node->getType(), $types)) {
   +    // Not a book node.
   +    return;
   +  }
   

   Instead use book_type_is_allowed()

3. +++ b/core/modules/book/book.module
   @@ -323,18 +323,24 @@ function book_node_prepare_form(NodeInterface $node, $operation, FormStateInterf
   -      '#markup' => '<p>' . t('%title is part of a book outline, and has associated child pages. If you proceed with deletion, the child pages will be relocated automatically.', array('%title' => $node->label())) . '</p>',
   +      '#prefix' => '<p class="messages messages--warning">',
   +      '#markup' => t('%title is part of a book outline, and has associated child pages. If you proceed with deletion, the child pages will be relocated automatically.', array('%title' => $node->label())),
   +      '#suffix' => '</p>',
   

   Why is this change necessary to fix the bug?

4. +++ b/core/modules/book/src/BookManager.php
   @@ -439,9 +439,9 @@ public function deleteFromBook($nid) {
   -      foreach ($result as $child) {
   -        $child['bid'] = $child['nid'];
   +      $children = $this->entityManager->getStorage('node')->loadMultiple(array_keys($result));
   +      foreach ($children as $child) {
   +        $child->book['bid'] = $child->id();
            $this->updateOutline($child);
          }
   

   Is this change of the child bid explicitly tested?

5. +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -30,7 +31,7 @@ class BookTest extends WebTestBase {
   -   * @var object
   +   * @var \Drupal\node\NodeInterface
   
   @@ -77,11 +78,13 @@ protected function setUp() {
   -    $this->adminUser = $this->drupalCreateUser(array('create new books', 'create book content', 'edit own book content', 'add content to books', 'administer blocks', 'administer permissions', 'administer book outlines', 'node test view', 'administer content types', 'administer site configuration'));
   +    $this->adminUser = $this->drupalCreateUser(['create new books', 'create book content', 'edit any book content', 'delete any book content', 'add content to books', 'administer blocks', 'administer permissions', 'administer book outlines', 'node test view', 'administer content types', 'administer site configuration']);
   ...
   +   *
   +   * @return \Drupal\node\NodeInterface[]
   

   Not in scope.

Re #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.

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -10,6 +10,8 @@
+use Drupal\node\Entity\Node;

Is this used?

Verified 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.

Committed a224bb8 and pushed to 8.0.x and 8.1.x. Thanks!