Support multiple implementations for hook_field_widget_third_party_settings_form

Created on 25 November 2024, 26 days ago

Problem/Motivation

Since the flood gates are open for multiple implementations of the same hook in the same modules, the hook field_widget_third_party_settings_form doesn't support that since it 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.

Proposed resolution

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

Remaining tasks

Code solutions
Add tests

User interface changes

None

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component

field system

Created by

πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @rodrigoaguilera
  • Pipeline finished with Failed
    26 days ago
    Total: 96s
    #349732
  • Pipeline finished with Failed
    26 days ago
    Total: 104s
    #349735
  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

    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(

  • Pipeline finished with Success
    26 days ago
    Total: 2851s
    #349739
  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    don't forget field_formatter_third_party_settings_form as well

  • Pipeline finished with Failed
    26 days ago
    Total: 179s
    #350239
  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona
  • Pipeline finished with Failed
    26 days ago
    Total: 1069s
    #350252
  • Pipeline finished with Failed
    26 days ago
    Total: 2052s
    #350311
  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

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

  • Pipeline finished with Failed
    26 days ago
    Total: 547s
    #350359
  • Pipeline finished with Success
    26 days ago
    Total: 524s
    #350467
  • Pipeline finished with Success
    26 days ago
    Total: 620s
    #350545
  • Pipeline finished with Failed
    26 days ago
    Total: 1671s
    #350716
  • Pipeline finished with Failed
    26 days ago
    Total: 533s
    #350802
  • Pipeline finished with Failed
    26 days ago
    Total: 605s
    #350850
  • Pipeline finished with Failed
    25 days ago
    Total: 575s
    #350997
  • Pipeline finished with Failed
    25 days ago
    Total: 506s
    #351017
  • Pipeline finished with Success
    25 days ago
    Total: 504s
    #351029
  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

    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 the the simple fix.

    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.

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • Pipeline finished with Failed
    25 days ago
    Total: 700s
    #351889
  • Pipeline finished with Success
    25 days ago
    Total: 574s
    #351911
  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

    Sure. Added tests for the formatters as well

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Looks great now thanks!

Production build 0.71.5 2024