Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2112237-book-outline-fatal-22.patch | 2.86 KB | vijaycs85 |
#19 | 2112237-block-19.patch | 2.86 KB | vijaycs85 |
#19 | 2112237-diff-16-19.txt | 472 bytes | vijaycs85 |
#16 | 2112237-16.patch | 2.86 KB | damiankloip |
#16 | interdiff-2112237-16.txt | 549 bytes | damiankloip |
Comments
Comment #1
vijaycs85Initial patch...
Comment #2
dawehnerI guess we should test the actual output of this page as well? Just the title seems not enough.
Comment #4
BerdirInteresting, 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.
Comment #5
larowlanAdding tag
Comment #6
vijaycs85Yes @Berdir, those notices are on drupalLogin(). Not sure what's wrong as the same user login in other cases in same class works fine.
Comment #7
vijaycs85Restoring tag.
Comment #8
neclimdulCause: #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.
Comment #9
BerdirYeah, 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.
Super nitpick: TestS...
Also, spending 1h in xdebug doesn't sound like novice to me :)
Comment #10
tim.plunkettI'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.
Comment #11
neclimdulYou 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:
http://3v4l.org/WTtIe
So that passes and we get false added to the list of theme suggestions. Then, later this code gets run:
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.
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.
Comment #12
neclimdulUpdating documentation on interface.
Comment #13
jibranNULL
Let's fix #9 and this is RTBC. Nice interdiff :P
Comment #14
tim.plunkettnull is correct. We *just* fixed all of these: #2105065: Use correct capitalization of NULL for @param/@return/@var
Comment #15
BerdirLooking at EntityFormController::getBaseFormId(), it returns '' in case there is no base form id, should this be updated to NULL too?
Comment #16
damiankloip CreditAttribution: damiankloip commentedI would say, yes.
Comment #17
BerdirOk, I think this is good to go.
Comment #18
jibranTests :) as pointed in #9.
Comment #19
vijaycs85Here it is
Comment #20
damiankloip CreditAttribution: damiankloip commentedThe status can stay the same for that change.
Comment #21
alexpottPatch no longer applies.
Comment #22
vijaycs85Quick re-roll...
Comment #23
vijaycs85Updating tag...
Comment #24
catchComment #25
catchCommitted/pushed to 8.x, thanks!