Problem/Motivation
There is now a fatal error if a field_widget_third_party_settings_form hook doesn't return an array, such as entity_browser_entity_form_field_widget_third_party_settings_form(), which conditionally adds something only for a certain plugin.
Yes, it's documented as returning an array, but typically hooks don't require you to return something and it wasn't necessary until now. Breaks a fairly high profile contrib module, so creating a core issue instead of just fixing that. There are likely others.
Other exampes: sitestudio_default_helper, ai_translate_textfield, 5 pages of results, most do return an array.
Steps to reproduce
1. Install entity browser + entity_browser_entity_form
2. Edit form display of a node type, maybe need to open the widget settings of a field, maybe already fails on accessing the page.
Proposed resolution
Fallback to [] if empty
Remaining tasks
User interface changes
NA
Introduced terminology
NA
API changes
NA
Data model changes
NA
Release notes snippet
nA
Issue fork drupal-3514262
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
berdirMerge request created.
Comment #4
nicxvan commentedThis could probably use a test.
Comment #5
smustgrave commentedCould an additional assertion be added somewhere?
Comment #6
berdirA test would require another hook that returns nothing. I'm not sure that's really worth it.
I'm also not sure we actually want to support this. we could also explicitly deprecate a non-array return value.
either way, leaving at needs work to update the second place
Comment #7
berdirUpdated the second place. I think it would be nice to get this in before 11.2 and avoid fatal errors. I can fix entity_browser but others might be affected too.
Comment #8
smustgrave commentedIf we don't add tests could we add steps for manual testing to trigger this error?
Comment #9
berdirAdded STR, I don't remember 100% if it already happens when you visit the form display page or if you need to click the gear icon of a given field, but one or the other should reproduce the error.
Comment #10
smustgrave commentedJust visiting the Manage form display caused the fatal error. "TypeError: Unsupported operand types: array + null in Drupal\field_ui\Form\EntityFormDisplayEditForm->Drupal\field_ui\Form\{closure}() (line 131 of core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php)."
MR did solve the problem
Wasn't even using entity_browser just installed it.
Comment #11
smustgrave commentedComment #12
xjmComment #16
catchI think we need a follow-up to either add a test hook implementation that triggers the bug somewhere (probably an existing test would do fine - just enable the module so it explodes without the fix), or to properly deprecate a void return value from the hook and require an array.
This is a clear example where the new test guidelines from https://www.drupal.org/about/core/policies/core-change-policies/core-gat... apply for me - easy to reproduce, easy to fix, doesn't add any new untested code paths because that ternary executes all the time anyway. Maybe we should add a line like 'Fixes a recent regression which for any reason is not suitable to rollback in the target branch' too.
Crediting @xjm and @quietone for bringing this up in slack as one to focus on during the last hours of rc.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Once the follow-up is open would be good to link from here.
Comment #18
xjmJust so the followup doesn't get lost. Normally we would ask for that before commit but this was about hotfixing a regression in the minor before said minor was out. :)
Comment #19
quietone commentedFollow up created, #3566756: Test that third_party_settings_form hooks can return NULL or deprecate void return
Comment #20
smustgrave commentedSince follow up has been created, restoring fixed status. hope that's okay.