Problem/Motivation

While testing #1968982: Convert book theme tables to table #type, found a bug.

Steps to reproduce

  1. Install Drupal 8 using the Standard profile.
  2. Enable the Book module.
  3. Create a book page (/node/add/book), setting the value in Book outline > Book to '- Create a new book -'.
  4. Save the form at /admin/structure/book/1.

Current behavior:

Notice: Undefined index: table in Drupal\book\Form\BookAdminEditForm->submitForm() (line 100 of core/modules/book/src/Form/BookAdminEditForm.php).
Warning: array_keys() expects parameter 1 to be array, null given in Drupal\book\Form\BookAdminEditForm->submitForm() (line 100 of core/modules/book/src/Form/BookAdminEditForm.php).
Warning: array_flip() expects parameter 1 to be array, null given in Drupal\book\Form\BookAdminEditForm->submitForm() (line 100 of core/modules/book/src/Form/BookAdminEditForm.php).
Warning: array_merge(): Argument #1 is not an array in Drupal\book\Form\BookAdminEditForm->submitForm() (line 101 of core/modules/book/src/Form/BookAdminEditForm.php).
Recoverable fatal error: Argument 1 passed to Drupal\Core\Render\Element::children() must be of the type array, null given, called in /Users/sreeves/Sites/d8.dev/core/modules/book/src/Form/BookAdminEditForm.php on line 103 and defined in Drupal\Core\Render\Element::children() (line 75 of core/lib/Drupal/Core/Render/Element.php).

Proposed resolution

Seems like the keys expected by the submit handler are incorrect.

Remaining tasks

  1. Write patch
  2. Will need to write a webtest to prevent this from happening again.

User interface changes

n/a

API changes

TBD

CommentFileSizeAuthor
#7 interdiff.txt1.11 KBstar-szr
#7 2346313-7.patch4.23 KBstar-szr
#7 2346313-7-testonly.patch924 bytesstar-szr
#3 book-admin-form-broken-2346313-3.patch4.04 KBandrei.dincu

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Category: Task » Bug report
Priority: Normal » Major
Issue summary: View changes

Updating STR and confirming the bug.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Active
andrei.dincu’s picture

andrei.dincu commented
Status: Active » Needs review
StatusFileSize
new book-admin-form-broken-2346313-3.patch4.04 KB

Check if $form_state['table'] is set in BookAdminEditForm and add test function testEmptyBook() in BookTest.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Title: Book admin form broken » Fatal error when submitting the book admin form with an empty book
Status: Needs review » Needs work

Thanks @andrei.dincu! Can we get a test-only patch uploaded please so we can see that the test catches the bug? Similar to what you see on #2348413: Fatal error at /admin/structure/book.

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -95,6 +95,17 @@ function createBook() {
+  function testEmptyBook() {
+      // Create new book.
+    $nodes = $this->createBook();

This function needs a docblock (all functions do), and the indentation of the inline comment is too much. Please see https://www.drupal.org/node/1354#drupal and https://www.drupal.org/node/1354#inline for more info. :)

I would also suggest that the $book variable is not necessary, just use $this->book, but that's debatable.

The last submitted patch, 3: book-admin-form-broken-2346313-3.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: Unassigned » star-szr

Working on some updates to the test, hope you don't mind @andrei.dincu!

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: star-szr » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2346313-7-testonly.patch924 bytes
new 2346313-7.patch4.23 KB
new interdiff.txt1.11 KB

A few updates to the test besides the docs, these are the important changes:

  • Create an empty book by calling createBookNode('new') directly
  • Log in as admin_user to be able to access admin/structure/book/ID
  • Submit the form via drupalPostForm
  • Remove the HTML from the assertText parameter (use @ instead of %) since assertText checks the "text version" of the page

The last submitted patch, 7: 2346313-7-testonly.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +Needs manual testing, +Novice, +Amsterdam2014

Awesome, tagging for the sprint tomorrow:
Someone should run through this patch on simplytest.me with the issue summary steps and if all is good, RTBC!

tomefa’s picture

tomefa commented

I will test this patch now.

tomefa’s picture

tomefa commented
Status: Needs review » Reviewed & tested by the community

I applied the patch and followed the steps on simplytest.me
No error on the php_error log file or in the drupal log and nothing on the screen too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 2e60dbb and pushed to 8.0.x. Thanks!

  • alexpott committed 2e60dbb on 8.0.x 
    Issue #2346313 by Cottser, andrei.dincu | joelpittet: Fixed Fatal error...

Status: Fixed » Closed (fixed)

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