Problem/Motivation

Follow-up of #3489707: Support multiple implementations for hook_field_widget_third_party_settings_form and field_formatter_third_party_settings_form in the same module.

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

Command icon 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

berdir created an issue. See original summary.

berdir’s picture

Status: Active » Needs review

Merge request created.

nicxvan’s picture

This could probably use a test.

smustgrave’s picture

Status: Needs review » Needs work

Could an additional assertion be added somewhere?

berdir’s picture

A 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

berdir’s picture

Status: Needs work » Needs review

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

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs steps to reproduce

If we don't add tests could we add steps for manual testing to trigger this error?

berdir’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs steps to reproduce

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

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

smustgrave’s picture

Issue summary: View changes
xjm’s picture

 

  • catch committed c2f61df0 on 11.2.x
    Issue #3514262 by berdir, smustgrave, xjm, quietone:...

  • catch committed 2cecb7b3 on 11.x
    Issue #3514262 by berdir, smustgrave, xjm, quietone:...

catch credited quietone.

catch’s picture

Version: 11.x-dev » 11.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

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

xjm’s picture

Title: entity_browser_entity_form_field_widget_third_party_settings_form hooks can no longer return NULL » [already committed, NW for followup] entity_browser_entity_form_field_widget_third_party_settings_form hooks can no longer return NULL
Status: Fixed » Needs work

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

quietone’s picture

Title: [already committed, NW for followup] entity_browser_entity_form_field_widget_third_party_settings_form hooks can no longer return NULL » entity_browser_entity_form_field_widget_third_party_settings_form hooks can no longer return NULL
Status: Needs work » Needs review
Issue tags: -Needs followup
smustgrave’s picture

Status: Needs review » Fixed

Since follow up has been created, restoring fixed status. hope that's okay.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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