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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.51 KB
820 bytes

Initial patch...

dawehner’s picture

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.

Berdir’s picture

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.

larowlan’s picture

Issue tags: +Novice

Adding tag

vijaycs85’s picture

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.

vijaycs85’s picture

Issue tags: -Regression +Novice

Restoring tag.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

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.

Berdir’s picture

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

tim.plunkett’s picture

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

neclimdul’s picture

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:

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

  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.

  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.

neclimdul’s picture

Updating documentation on interface.

jibran’s picture

+++ 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

tim.plunkett’s picture

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

Berdir’s picture

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

damiankloip’s picture

FileSize
549 bytes
2.86 KB

I would say, yes.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I think this is good to go.

jibran’s picture

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.

vijaycs85’s picture

Here it is

damiankloip’s picture

The status can stay the same for that change.

alexpott’s picture

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

Patch no longer applies.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

Quick re-roll...

vijaycs85’s picture

Issue tags: -Needs reroll +Regression

Updating tag...

catch’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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