Problem/Motivation

Since the flood gates are open for multiple implementations of the same hook in the same modules, the hooks field_widget_third_party_settings_form and field_formatter_third_party_settings_form don't support that since they expects each module to have only one implementation

Steps to reproduce

Use the Hook attribute twice in the same module

#[Hook('field_widget_third_party_settings_form')]                                                                                                                                                             
  public function fieldWidgetThirdPartySettingsForm(WidgetInterface $plugin, FieldDefinitionInterface $field_definition, $form_mode, $form, FormStateInterface $form_state) {

adding different form elements.
Both form elements are expected but only one of them show up, the last one to be processed.

Proposed resolution

Merge all elements of the same module in the same form array

Remaining tasks

Code solution.
Add tests.

User interface changes

None

Issue fork drupal-3489707

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:

About issue forks

Comments

rodrigoaguilera created an issue. See original summary.

rodrigoaguilera opened merge request !10332

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Status: Active » Needs work

Weird. The tests are not failing where they are supposed to.

The solution to fixing the test should be in core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php protected function thirdPartySettingsForm().

Adding:
$settings_form[$module] = ($settings_form[$module] ?? []) + $hook(

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

don't forget field_formatter_third_party_settings_form as well

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Title: Support multiple implementations for hook_field_widget_third_party_settings_form » Support multiple implementations for hook_field_widget_third_party_settings_form and field_formatter_third_party_settings_form in the same module
Issue summary: View changes
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented

Seems like trying to fail the test on purpose, changing assertNotEmpty for assertEmpty, also passes the test so I'm clueless about what is happening with that test :(

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Status: Needs work » Needs review

Seems like it was an issue with not declaring the schema for the new third party setting field.
I also squashed the experiments that I did while I didn't have a proper dev enviroment.

In the last two commits I add just the test, failing in the correct place and then the simple fix that fixes those tests.

The test only checks widgets, not formatters but since it was the same fix for both I didn't find it very useful to duplicate testing there.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Thanks for the rapid fix, I like it but I can't RTBC it because I suggested the heart of the change so it would be somewhat RTBC'ing my own code. Maybe others don't quite like the succinct version and want core to elaborate on it? I dunno.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Sorry since this is one of two exceptions I think we need a test for both to prevent a regression more than prove the fix works.

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Status: Needs work » Needs review

Sure. Added tests for the formatters as well

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Looks great now thanks!

acbramley’s picture

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

PHPStan basline has conflicts

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Status: Needs work » Reviewed & tested by the community

The phpstan issues were solved in other commits so no need to modify the baseline anymore. Back to RTBC

quietone’s picture

quietone commented
Issue tags: -Needs reroll

I read the IS, comments and the MR. There are no unanswered questions and I updated credit.

I did leave suggestions in the MR for to improve the comments. But I think they are minor and should not block commit, unless, of course, another committer wants those comments changed.

Leaving at RTBC

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented

Thanks. I applied the suggestions from the MR

catch made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

phpcs fails after a rebase.

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Status: Needs work » Needs review

Thanks for the rebase. I added some $this->t() to other string to be consistent.

Plase review again

nicxvan’s picture

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

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

  • catch committed 18a2a3b4 on 11.x 
    Issue #3489707 by rodrigoaguilera, nicxvan, ghost of drupal past,...

catch closed merge request !10332

catch’s picture

catch
he/him
Primary language English
commented

Status: Fixed » Closed (fixed)

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This results in 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.

created a follow-up: #3514262: entity_browser_entity_form_field_widget_third_party_settings_form hooks can no longer return NULL