There is a fatal error on book outline page (node/[nid]/outline) because of reference to EntityFormControllerNG in core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php form.

Files: 
CommentFileSizeAuthor
#22 2112237-book-outline-fatal-22.patch2.86 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,251 pass(es).
[ View ]
#19 2112237-block-19.patch2.86 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,843 pass(es).
[ View ]
#19 2112237-diff-16-19.txt472 bytesvijaycs85
#16 2112237-16.patch2.86 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,852 pass(es).
[ View ]
#16 interdiff-2112237-16.txt549 bytesdamiankloip
#12 drupal8.book-module.2112237-12.patch2.33 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 59,156 pass(es).
[ View ]
#12 interdiff.txt1.6 KBneclimdul
#8 drupal8.book-module.2112237-8.patch1.74 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 58,856 pass(es).
[ View ]
#1 2112237-book-outline-fatal-1-test-only.patch820 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 59,292 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 2112237-book-outline-fatal-1.patch1.51 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 59,519 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.51 KB
FAILED: [[SimpleTest]]: [MySQL] 59,519 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
new820 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,292 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Initial patch...

I guess we should test the actual output of this page as well? Just the title seems not enough.

Status:Needs review» Needs work

The last submitted patch, 2112237-book-outline-fatal-1.patch, failed testing.

Priority:Normal» Critical

Interesting, looks like even that simple test shows another bug?

Noticed this too a while ago, I'd suggest to not delay a fatal error fix with additional tests.

AFAIK easy to reproduce fatals/regressions are critical.

Issue tags:+Novice

Adding tag

Issue tags:-Novice

Yes @Berdir, those notices are on drupalLogin(). Not sure what's wrong as the same user login in other cases in same class works fine.

Issue tags:-regression+Novice

Restoring tag.

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,856 pass(es).
[ View ]

Cause: #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface

Patch from #2112619: WSOD Book Outline Page with cleaned up test from #1

Also, those warnings where miserable to trace down. took me over an hour in xdebug... yikes... Its the FALSE that was being returned from getBaseFormID(); false passes isset but then fails when it gets passed as the key to array_key_exists() in the theme registry. Lots of rickety code interacting.

Issue tags:-Novice

Yeah, I'm aware that I broke this one :)

I guess it got in during one of the re-rolls. I did update a few of those forms that resulted in test fails but must have missed this one.

+++ b/core/modules/book/lib/Drupal/book/Tests/BookTest.php
@@ -509,4 +509,17 @@ public function testBookOrdering() {
+  /**
+   * Test outline of a book.
+   */
+  public function testBookOutline() {

Super nitpick: TestS...

Also, spending 1h in xdebug doesn't sound like novice to me :)

+++ b/core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php
@@ -51,7 +51,7 @@ public static function create(ContainerInterface $container) {
-    return FALSE;
+    return NULL;

I'm not sure why this change is needed, AFAIK the base_form_id is always checked with isset().

But if we do choose to make this change, please update BaseFormIdInterface accordingly.

You are correct, base_form_id is checked with isset before adding it to the theme suggestions. https://github.com/drupal/drupal/blob/8.x/core/includes/form.inc#L1154
However:

<?php
$var
= false;
var_dump(isset($var)); // true.
?>
http://3v4l.org/WTtIe

So that passes and we get false added to the list of theme suggestions. Then, later this code gets run:

<?php
 
if (is_array($hook)) {
    foreach (
$hook as $candidate) {
      if (isset(
$hooks[$candidate])) {
        break;
      }
    }
?>

https://github.com/drupal/drupal/blob/8.x/core/includes/theme.inc#L925

When that 'false' comes up, we hit isset($hooks[false]), we go into the offsetExists on ThemeRegistry which runs this with $offset = false which triggers the warning.

<?php
 
return array_key_exists($offset, $this->storage);
?>

https://github.com/drupal/drupal/blob/8.x/core/lib/Drupal/Core/Utility/T...

Arguably that's a bug, but returning NULL worked around it.

StatusFileSize
new1.6 KB
new2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 59,156 pass(es).
[ View ]

Updating documentation on interface.

+++ b/core/lib/Drupal/Core/Form/BaseFormIdInterface.php
@@ -18,8 +18,8 @@
+   * @return string|null

NULL
Let's fix #9 and this is RTBC. Nice interdiff :P

null is correct. We *just* fixed all of these: #2105065: Use correct capitalization of NULL for @param/@return/@var

Looking at EntityFormController::getBaseFormId(), it returns '' in case there is no base form id, should this be updated to NULL too?

StatusFileSize
new549 bytes
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,852 pass(es).
[ View ]

I would say, yes.

Status:Needs review» Reviewed & tested by the community

Ok, I think this is good to go.

Issue tags:-Needs tests

+++ b/core/modules/book/lib/Drupal/book/Tests/BookTest.php
@@ -509,4 +509,17 @@ public function testBookOrdering() {
+   * Test outline of a book.

Tests :) as pointed in #9.

StatusFileSize
new472 bytes
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,843 pass(es).
[ View ]

Here it is

The status can stay the same for that change.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,251 pass(es).
[ View ]

Quick re-roll...

Issue tags:-Needs reroll+regression

Updating tag...

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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