Original report by @swentel

Problem

A number of D7 contrib modules use hook_field_(formatter|widget)_info_alter() to add extra "settings" to some widgets / formatters.
They then use form alters on Field UI forms to let admins enter values for those extra settings (and let Field UI store those values along with the "regular" settings), and either entity_view_alter or entity_form_alter to modify the render arrays generated by the render arrays according to those "extra settings".

Typically, this is done to add functionnality not provided by core (e.g: display only the 1st value; use a custom separator between values...). See all modules depending on field_formatter_settings.

The problem is we don't know which module altered those extra settings in, and therefore cannot provide config schema for them (I initially opened #1975112: Config schema mechanisms cannot reflect alterable config trees for this). Thus, those extra settings can't be localized.

Proposed solution

We discussed that with @gabor and @alexpott in Portland, and agreed that putting those 3rd party extra-configuration as part of the formatter/widget "settings" makes no sense. Formatters are classes, this is closed code, you cannot make them use "extra settings" they don't know about.
Those extra properties are not settings for the formatter, they are settings for some 3rd party code that runs after the formatter and alters the result. Storing them along with the formatter settings is just a convenient way to have them automatically saved and loaded back (in D8, in the EntityDisplay config entities).

The typical answer for this in D8 is "store this yourself in your own CMI records / ConfigEntities". If a 3rd party module has params to store about itw own config for a node type, this doesn't go in node.type.[node_type].yml.

In this specific case, however, the logic to get from a view mode to the actual settings to use for this field in this entity bundle in this view mode is a bit complex - see EntityViewDisplay::collectRenderDisplays(). Requiring each 3rd party module that wants to add "extra properties" to store those themselves means they need to reproduce that same logic, and chances are high that they will do it wrong. Plus, the methaphor of EntityDisplay objects is that they contain the whole "recipe" to display an entity, for instance so that they can be altered as a whole.

So the plan would be to add an 'extra' entry, next to 'settings', in each component in EntityDisplay objects. This would act as a free for all dumping ground for 3rd party "settings". The content of this entry would need to be keyed by module name, so that we can build config schemas.

Remaining tasks

  • Key settings by module
  • Add dependencies
  • Add configuration schema

API Changes

- EntityDisplay::getComponent() / setComponent() accept an additional 'extra' entry.
- The config schema for EntityDisplay objects needs to be adjusted accordingly (but HEAD currently provides no config schema for EntityDisplay anyway)
- Field UI probably needs to provide an 'extra' form container in formatter & widget configuration forms, so that the elements altered in there end up saved in the 'extra' entries of each component. It's the responsibility of each module that alters stuff in to key its additions by module name.
- More importantly, this means that widgets / formatter settings do not need to be alterable anymore, and can get out of the plugin definitions and move to a class method. Memory gains here, and also allows us to be more in line with what other plugin types do. Actually doing this affects all widgets / formatters though. Probably for a followup, if at all.

Postponed until

CommentFileSizeAuthor
#100 2005434.100.patch59.35 KBalexpott
#100 98-100-interdiff.txt1.31 KBalexpott
#98 2005434.98.patch57.63 KBalexpott
#98 95-98-interdiff.txt2.02 KBalexpott
#95 2005434.95.patch55.66 KBalexpott
#95 93-95-interdiff.txt7.07 KBalexpott
#93 2005434.93.patch50.21 KBalexpott
#91 2005434.91.patch50.96 KBalexpott
#91 89-91-interdiff.txt15.82 KBalexpott
#89 2005434.89.patch44.14 KBalexpott
#89 87-89-interdiff.txt2.57 KBalexpott
#87 2005434.87.patch44.66 KBalexpott
#87 85-87-interdiff.txt6.36 KBalexpott
#85 2005434.85.patch38.4 KBalexpott
#85 79-85-interdiff.txt20.68 KBalexpott
#80 2005434.79.patch26.38 KBalexpott
#80 78-79-interdiff.txt17.01 KBalexpott
#78 2005434.78.patch18.94 KBalexpott
#78 74-78-interdiff.txt8.28 KBalexpott
#74 interdiff-2005434-72-74.txt973 bytesandrewmacpherson
#74 let_3rd_party_modules-2005434-74.patch17.79 KBandrewmacpherson
#72 let_3rd_party_modules-2005434-72.patch17.84 KBandrewmacpherson
#72 interdiff-2005434-71-72.txt618 bytesandrewmacpherson
#71 interdiff-2005434-66-71.txt2.35 KBandrewmacpherson
#71 let_3rd_party_modules-2005434-71.patch18.62 KBandrewmacpherson
#66 let_3rd_party_modules-2005434-66.patch16.03 KBandrewmacpherson
#66 interdiff-2005434-63-66.txt9.99 KBandrewmacpherson
#66 interdiff-2005434-61-66.txt14.37 KBandrewmacpherson
#66 interdiff-2005434-56-66.txt15 KBandrewmacpherson
#64 interdiff.txt8.22 KBheddn
#64 let_3rd_party_modules-2005434-63.patch10.01 KBheddn
#61 interdiff.txt6.82 KBandrewmacpherson
#61 let_3rd_party_modules-2005434-61.patch12.51 KBandrewmacpherson
#56 interdiff.txt6.83 KBheddn
#56 let_3rd_party_modules-2005434-56.patch8.03 KBheddn
#53 interdiff.txt4.34 KBheddn
#53 let_3rd_party_modules-2005434-53.patch7.7 KBheddn
#49 interdiff.txt868 bytesheddn
#49 let_3rd_party_modules-2005434-49.patch5.5 KBheddn
#46 interdiff.txt5.38 KBheddn
#46 let_3rd_party_modules-2005434-45.patch5.47 KBheddn
#44 interdiff.txt1.99 KBheddn
#44 2005434-3rd_party_modules-44.patch5.31 KBheddn
#41 2005434-3rd_party_modules-41.patch2.2 KBheddn
#32 2005434-32.patch4.69 KBheddn
#26 2005434-26.patch4.81 KBswentel
#24 2005434-24.patch1.93 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Not sure what this issue is about.
Supporting field_groups / display suite sounds like #1875974: Abstract 'component type' specific code out of EntityDisplay ?

So is this the "add a free-for-all dumping ground property in each EntityDisplay components" issue we talked about in Portland about not being able to support extra formatter / widget settings added by 3rd party modules ? (the problem being that we currently cannot provide config schema for those)

swentel’s picture

Yes, this is the dumping ground one, never got to start a patch for this for now, will be august, if we get that even in anyway ?!

yched’s picture

OK, expanded the issue summary then.

yched’s picture

Title: Let other modules store configuration in entity display » Let 3rd party modules store extra configuration in entity display

Adjusted title

yched’s picture

Title: Let 3rd party modules store extra configuration in entity display » Let 3rd party modules store extra configuration in EntityDisplay

again

yched’s picture

Issue summary: View changes

Expanded issue summary

webchick’s picture

Based on the fact that this already discussed w/ Alex Pott and (assuming the summary is correct) the API impact here is minimal, we can probably go ahead and tag this as "approved API change."

xjm’s picture

Priority: Normal » Major

Based on the discussion in Portland, I had the impression that this was at least major. (Please re-demote if that's not the case.)

yched’s picture

@webchick / xjm :
Does the "Approved" tag extend to the last item in the "API change" section ?

More importantly, this means that widgets / formatter settings do not need to be alterable anymore, and can get out of the plugin definitions and move to a class method. Memory gains here, and also allows us to be more in line with what other plugin types do. Actually doing this affects all widgets / formatters though.

effulgentsia’s picture

this means that widgets / formatter settings do not need to be alterable anymore, and can get out of the plugin definitions and move to a class method

This just refers to *default* settings, correct? Can't we do this part in a BC way by making WidgetBase and FormatterBase implement the class method as returning what's in the plugin definition, if it's there, but mark the $settings annotation property within FieldWidget and FieldFormatter as @deprecated?

yched’s picture

re #9: that's about defining the collection of settings names that the widget knows about, and their default values.

The BC you propose would work as a temporary measure I guess, but I don't think we'd want to leave it in the release, because it would result in "the settings for WidgetA are alterable with hook_field_widget_settings() because they are still written in WidgetA class annotations, but not the settings for WidgetB", which would be really weird and inconsistent.

yched’s picture

Issue summary: View changes

followup

alexpott’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +beta blocker

If we inject random settings into the settings part of of field.instance config entity then we can not create a full config schema. This is a problem with the content_translation module currently adds a translation_sync setting for every field instance. This caused interesting problems with the module install uninstall test in #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct as this is the only test that has both the content_translation and options module installed.

yched’s picture

@alexpott : actually I was wrong on IRC by pointing you to this issue :-/.
This issue is about EntityDisplay objects, and the approach described here would only let us solve the issue of 3rd party settings altered in into widgets and formatters settings. content_translation adds a field/instance setting, so, albeit related/similar, it's out of scope here.

[edit: considerations moved to a separate issue, linked below]

yched’s picture

xjm’s picture

Issue tags: +Entity Field API
yched’s picture

Issue summary: View changes
yched’s picture

Wim Leers’s picture

Is this blocked by #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition? This issue's summary is *very* similar to the one of #2136197, which is pretty confusing. It's not clear to me what the goal of this issue versus #2136197 is.

yched’s picture

@Wim : the two are definitely related, (and yeah, both issue summaries kind of re-explain the issue at hand) but the code scopes are independant.

- #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition moves the declaration of "settings" out of the plugin annotations, and into the classes themselves, like most other plugin types do, which de facto makes the settings non-alterable (out of reach of hook_field_*_info_alter())
- This issue then intends to provide a replacement mechanism for storing "3rd party extra properties" for each component in EntityDisplay config entities.

The split can be debated / reconsidered, of course.

Wim Leers’s picture

No, that makes perfect sense, that split just wasn't clear to me. Thanks for explaining! :)

Maybe because I wasn't part of the in-person discussions and had only the issue summaries to provide context.

effulgentsia’s picture

A number of D7 contrib modules use hook_field_(formatter|widget)_info_alter() to add extra "settings" to some widgets / formatters.

Core does not have any examples of this, not even in a test module, which makes the implementation of this issue completely independent from #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition. What we need in this issue is a test module to be a use case for adding extra settings in the way proposed by this issue, and then make that test work. Per the issue summary, the field_formatter_settings module and its dependents can be used as inspiration for crafting the test module.

xjm’s picture

jessebeach’s picture

Issue summary: View changes
jessebeach’s picture

Status: Postponed » Active

Unpostponed!

swentel’s picture

Status: Active » Needs review
FileSize
1.93 KB

We actually have field_test module that altered the settings for widgets and formatters. However, the tests were actually not testing fully, even before the removal of annotation settings (though the functionality works). Adjusted the patch, will have two fails now. No other changes so far though. Will look a bit further this week(end).

Status: Needs review » Needs work

The last submitted patch, 24: 2005434-24.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.81 KB

Updated patch, quickly hacked together. Works functionality wise, but misses dependencies (it's also currently not keyed by module) and configuration schema.

effulgentsia’s picture

Status: Needs review » Needs work

Per #26.

swentel’s picture

Additionally, the patch now stores the 'extra' key in the settings key, that should be moved.
I'm offline though for at least a week, so if anyone feels like picking up, go ahead.

mikemiles86’s picture

Assigned: Unassigned » mikemiles86
mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
catch’s picture

Not clear what the current status of this issue is, but just cross-posting #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration since I keep getting confused which issue is which.

heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.69 KB

Reroll.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

@catch The difference with #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration that just adds field translation settings only.

This one just an ability to store some "extra" (outbound) settings within formatter and widget configuration in entity display

Also there's #1875974: Abstract 'component type' specific code out of EntityDisplay but mostly about adding sane api to add field grouping and other cases

jibran’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Missing change record

This needs change record.

alexpott’s picture

Also we need tests and we're still not keying the settings by module and ensuring config schemas work

xjm’s picture

Issue tags: -Missing change record

@jibran, the missing change record tag is for issues that have already been committed. This one isn't ready yet. We will need a draft change record when the issue is complete and only if it includes public API changes that introduce BC breaks.

jibran’s picture

@xjm thank you very much for explaining that. So is it "Needs change record" then?
It is allowing 3rd party modules(core+contrib) to store extra configuration in EntityDisplay. IMHO it is a public API change. And from issue summary

Actually doing this affects all widgets / formatters though. Probably for a followup, if at all.

so it introduce BC breaks. And issue was RTBC so I assumed issue was complete.

heddn’s picture

This adds keying by module. Not sure how to handle adding dependencies and configuration schema. For the field display schema, I started adding some stuff to field.schema.yml but wasn't sure how to structure the schema for unstructured settings.

heddn’s picture

Status: Needs work » Needs review
swentel’s picture

Actually doing this affects all widgets / formatters though. Probably for a followup, if at all.
so it introduce BC breaks. And issue was RTBC so I assumed issue was complete.

No, this was done already in #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition.

What's left in HEAD is the regression that it's not possible currently to store extra settings - unless you want to handle the storage, field ui form altering and saving yourself.

What's basically left here is #24 (get those tests in), #28 and #35

heddn’s picture

Missed adding the patch.

Status: Needs review » Needs work

The last submitted patch, 41: 2005434-3rd_party_modules-41.patch, failed testing.

yched’s picture

Copy / pasted from IRC :

<yched> the "extra settings" patch stores the settings as an 'extra' key in the same array than the rest of the settings ?
<swentel> ah yes, I made a comment somewhere that we should move that
<yched> yeah, I'd rather avoid polluting the existing 'settings' entry, we try to control what's in there
<yched> if we add a "dumping ground" entry for "stuff we don't really know about, it would be better off in a dedicated entry
<yched> I think that's required anyway if we want to have config schemas for those extras, since the config schema for 'settings' is in the hands of the module providing the widget/formatter
<swentel> also true
<swentel> updated my last comment - added #28 on "things left to do"
<swentel> So, an extra property in entity display then, but what name ... 'extra' seems so euh 'vague' :)
<yched> 'settings' / 'extra_settings' ? or 'third_party_settings' ?
<swentel> Hmmm, I would go for 'third_party_settings' , it's the best description
<yched> me too I guess
<yched> we better be clear what this is about, coz 'settings' / 'extra_settings' reads "uh wtf ?"
heddn’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
1.99 KB

Status: Needs review » Needs work

The last submitted patch, 44: 2005434-3rd_party_modules-44.patch, failed testing.

heddn’s picture

Here's a rename to third party.

heddn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: let_3rd_party_modules-2005434-45.patch, failed testing.

heddn’s picture

heddn’s picture

Status: Needs work » Needs review
yched’s picture

+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -348,7 +348,14 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
+          $settings_form['third_party_settings'][$field_definition->getProvider()] += $third_party_settings_form;

Not sure why we're keying by $field_definition->getProvider() (i.e always 'field' for configurable fields) ?

The goal is to have the 'third_party_settings' entry in the config file be keyed by the names of the various 3rd party modules that provide each "extra setting". So it looks like we can't rely on ModuleHandler::alter() anymore, and need to loop on ModuleHandler::getImplementations() and do individual calls to ModuleHandler::invoke() instead, keying the result with the module name.

Meaning, if we're not doing an alter(), we should rename hook_field_formatter_settings_form_alter() to hook_field_formatter_third_party_settings_form() ?

Status: Needs review » Needs work

The last submitted patch, 49: let_3rd_party_modules-2005434-49.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
7.7 KB
4.34 KB

Status: Needs review » Needs work

The last submitted patch, 53: let_3rd_party_modules-2005434-53.patch, failed testing.

Dave Reid’s picture

heddn’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
6.83 KB

Indexing by module.

andypost’s picture

This's awesome issue, let's get the same easy naming for this extra settings.
Currently node type already use this approach to store settings, so probably
it makes sense to introduce some interface for this kind of behavior.
At the same time entity reference field uses selection plugin for this extra settings

  1. +++ b/core/modules/field/tests/modules/field_test/field_test.module
    @@ -175,14 +175,16 @@ function field_test_field_widget_settings_summary_alter(&$summary, $context) {
    +function field_test_field_formatter_third_party_settings_form($element, $form_state, $context) {
    +  $third_party = $context['formatter']->getSetting('third_party_settings');
    

    why not getExtraSettings to allow store this data indexed by module(extension) name?

  2. +++ b/core/modules/field/tests/modules/field_test/field_test.module
    @@ -175,14 +175,16 @@ function field_test_field_widget_settings_summary_alter(&$summary, $context) {
    +    '#title' => t('3rd party formatter settings form'),
    

    Extra settings - much easy to get

  3. +++ b/core/modules/field_ui/src/DisplayOverview.php
    @@ -237,7 +237,18 @@ protected function alterSettingsForm(array &$settings_form, $plugin, FieldDefini
    +    $settings_form['third_party'] = array();
    ...
    +      $settings_form['third_party'][$module] = $third_party_settings;
    

    s/third_party/extra

  4. +++ b/core/modules/field_ui/src/DisplayOverview.php
    @@ -237,7 +237,18 @@ protected function alterSettingsForm(array &$settings_form, $plugin, FieldDefini
    +    foreach ($this->moduleHandler->getImplementations('field_formatter_third_party_settings_form') as $module) {
    +      $third_party_settings = $this->moduleHandler->invoke($module, 'field_formatter_third_party_settings_form', array(
    

    field_formatter_extra_settings_form

  5. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -348,7 +348,14 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
    +        $this->alterSettingsForm($additional_settings_form, $plugin, $field_definition, $form, $form_state);
    

    extra_settings_form

  6. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -348,7 +348,14 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
    +        if (!empty($additional_settings_form)) {
    +          $settings_form['wrapper'] = array(
    +            '#type' => 'container',
    ...
    +          $settings_form['wrapper'] += $additional_settings_form;
    

    Suppose "extra" better then some wrapper

  7. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -524,7 +531,11 @@ public function submitForm(array &$form, array &$form_state) {
    +        $third_party = !empty($settings['third_party_settings']) ? $settings['third_party_settings'] : array();
    ...
    +        if (!empty($third_party)) {
    +          $settings['third_party_settings'] = $third_party;
    

    IMO "extra_settings" more readable

yched’s picture

  1. +++ b/core/modules/field_ui/src/DisplayOverview.php
    @@ -237,7 +237,18 @@ protected function alterSettingsForm(array &$settings_form, $plugin, FieldDefini
    +      $third_party_settings = $this->moduleHandler->invoke($module, 'field_formatter_third_party_settings_form', array(
    +        $settings_form,
    +        $form_state,
    +        $context,
    +      ));
    +      // Index the settings form by module name.
    +      $settings_form['third_party'][$module] = $third_party_settings;
    

    For clarity, var name should somehow include "form". $sub_form ?

    (well, actually, that variable is not really needed, we could do the call right in the same line that assigns the result in $settings_form).

    Also, the hook is still shaped as if it was an alter hook, which it is not :
    - no need to squash parameters in a $context array, that's specific to alter hooks, we usually don't do that for other hooks
    - no need to pass $settings_form, the hook implementations are not supposed to mess with it. They do need $plugin, $field_definition, $view_mode, $form and $form_state though.
    - the method name itself is inaccurate now :-)

  2. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -348,7 +348,14 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
    +        $additional_settings_form = array();
    +        $this->alterSettingsForm($additional_settings_form, $plugin, $field_definition, $form, $form_state);
    +        if (!empty($additional_settings_form)) {
    

    Again, not an alter anymore. So this should be something like
    if ($third_party_settings_form = $this->thirdPartySettingsForm(...)) {
    // Add the wrapper.
    }

    (+ is 'wrapper' really the right name here ? Isn't this what structures the submitted values in $form_state['values'] ?)

  3. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -524,7 +531,11 @@ public function submitForm(array &$form, array &$form_state) {
    +        $third_party = !empty($settings['third_party_settings']) ? $settings['third_party_settings'] : array();
             $settings = array_intersect_key($settings, $default_settings);
    +        if (!empty($third_party)) {
    +          $settings['third_party_settings'] = $third_party;
    +        }
    

    Looks like this still places the "third party settings" within the 'settings' entry ? As per #43, we can't do that.

    We need to end up with saving the following :

    $component_values = array(
      'type' => $values['type'],
      'weight' => $values['weight'],
      'settings' => $settings,
      'third_party_settings' => $third_party_settings
    );
    
  4. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
    @@ -194,12 +194,12 @@ public function testWidgetUI() {
    -    $fieldname = 'fields[field_test][settings_edit_form][settings][field_test_widget_settings_form_alter]';
    +    $fieldname = 'fields[field_test][settings_edit_form][settings][wrapper][widget][field_test_widget_settings_form_alter]';
    

    There lies the issue IMO :
    The structure of form values we should aim is :
    - fields[$field_name][settings_edit_form][settings][some_official_setting]
    for "official settings of the formatter plugin being used"
    - fields[$field_name][settings_edit_form][third_party_settings][$module_name][some_third_party_setting]
    for "some 3rd party setting added by some 3rd party module"

yched’s picture

#58 crossposted with @andypost's #57.

re @andypost "extra settings / 3rd party settings" : see #43. I still favor an explicit 'third_party_settings'.

andrewmacpherson’s picture

I'm the maintainer of some D7 contrib modules which provide extra formatter settings. D8 ports are in progress, but are currently broken following #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition. Additionally, I have a D8 sandbox module experimenting with extra settings for widgets.

Each of these modules has a branch called 8.x-1.x-reimplement-extra-settings, where I will be keeping them up to date with the patch being developed here. I'll provide feedback as we go along.

andrewmacpherson’s picture

FileSize
12.51 KB
6.82 KB

The patch in #56 doesn't add the 3rd party settings to the widget settings form. This patch adds the equivalent code to invoke the 3rd-party settings hook for widgets too, in FormDisplayOverview::alterSettingsForm(). The equivalent code in the tests is updated too.

The documentation and example code for the widget hooks in field_ui.api.php is also updated, to be consistent with the formatter hooks.

This patch doesn't attempt to address any of the stuff in comments 57-59; those were posted while I was working on this one.

andrewmacpherson’s picture

Here's something I don't understand: why are the tests for the hooks in field_ui.module API kept in field_test.module along with field.module? Shouldn't they be in a field_ui_test.module? It seems a bit contrived that a module would alter it's own formatter/widget.

Status: Needs review » Needs work

The last submitted patch, 61: let_3rd_party_modules-2005434-61.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
10.01 KB
8.22 KB

To make things more confusing, this is a cross post of #2005434-61: Let 3rd party modules store extra configuration in EntityDisplay

I know it will fail. However, it incorporates feedback to remove part of the form alter logic. It doesn't remove the call to alterSettingsForm, because that is needed for other alters i.e. FormDisplayOverview.

  • Removes the context from alterSettingsForm in DisplayOverview
  • Removes the 'wrapper'.
  • Stores the third_party_settings on the same level as settings.
  • Adds dependencies for modules that provide third party settings.

It stops short is being able to retrieve the third_party_settings. This is because there is no getter for third_party_settings yet. The next step is to build that. After that, it still needs to add the configuration schema.

I'll be traveling tomorrow, so maybe someone else can continue move this forward and merge with #61.

Status: Needs review » Needs work

The last submitted patch, 64: let_3rd_party_modules-2005434-63.patch, failed testing.

andrewmacpherson’s picture

This patch combines the work from patches in #61 and #64

andrewmacpherson’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 66: let_3rd_party_modules-2005434-66.patch, failed testing.

heddn’s picture

I'm not seeing where alterSettingsForm is used any more. I'd suggest renaming that to thirdPartySettingsForm.

re: #2005434-57: Let 3rd party modules store extra configuration in EntityDisplay, can we call it additional_settings_form as opposed to extra_settings_form or third party? I don't want to bikeshed over third party, but I'm not really happy with the name either.

For the getThirdPartySetting function, I propose we add it to FormatterInterface with a signature of:

  /**
   * Returns the value of a setting, or its default value if absent.
   *
   * @param string $module
   *   The module providing the third party setting.
   * @param string $key
   *   The setting name.
   *
   * @return mixed
   *   The setting value.
   */
public function getThirdPartySetting($module, $key)

We'd also need associated setters and plural getter/setters (setThirdPartySetting, getThirdPartySettings, setThirdPartySettings.

andrewmacpherson’s picture

getThirdPartySetting (and related methods) need to be available for field widgets too, so I think this means they needs to go into Drupal\Core\Field\PluginSettingsInterface and Drupal\Core\Field\PluginSettingsBase.

andrewmacpherson’s picture

This patch adds the getter/setter method signatures to Drupal\Core\Field\PluginSettingsInterface.
It does not yet provide implementations in PluginSettingsBase.

I'm unclear how setThirdPartySettings() will be used. Is it intended to set the entire array of ALL third party settings at once, or should it also have a $module parameter (i.e. it sets all settings for one particular module)? I've assumed the former in this patch.

andrewmacpherson’s picture

FileSize
618 bytes
17.84 KB

I made a silly mistake in last night's patch; I'd added the getThirdPartySettings() signature to FormatterInterface and PluginSettingsInterface.

This patch corrects that. The third party getters/setters are declared in PluginSettingsInterface only.

Still need implementations in PluginSettingsBase.

yched’s picture

I'm unclear how setThirdPartySettings() will be used. Is it intended to set
the entire array of ALL third party settings at once, or should it also have
a $module parameter (i.e. it sets all settings for one particular module)?
I've assumed the former in this patch

Good question. I'd tend think the most frequent API use will be "sets all settings for one particular module", I'm not sure we have a case for some code to "set all for all modules".

andrewmacpherson’s picture

OK, updated the signature for setThirdPartySettings()

xjm’s picture

Status: Needs work » Needs review

@andrewmacpherson, be sure to set the issue NR when posting new patches so that they are picked up by testbot. Thanks for your work!

The last submitted patch, 72: let_3rd_party_modules-2005434-72.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 74: let_3rd_party_modules-2005434-74.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.28 KB
18.94 KB

Let's get Drupal installing and make Drupal\field_ui\Tests\ManageDisplayTest pass.

heddn’s picture

+++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
@@ -84,4 +91,40 @@ public function setSetting($key, $value) {
+  public function getThirdPartySettings() {
+    if (isset($this->settings['third_party'])) {
+      return $this->settings['third_party'];
+    }
+    return array();
...
+  public function getThirdPartySetting($module, $key) {
+    if (isset($this->settings['third_party'][$module][$key])) {
+      return $this->settings['third_party'][$module][$key];
+    }
+    return NULL;
...
+  public function setThirdPartySetting($module, $key, $value) {
+    $this->settings['third_party'][$module][$key] = $value;
+    return $this;
+  }
+
...
+  public function setThirdPartySettings($module, array $settings) {
+    $this->settings['third_party'][$module] = $settings;
+    return $this;
+  }

Shouldn't these all use $third_party_settings and not $settings? Or is this just so tests will start to pass again.

alexpott’s picture

FileSize
17.01 KB
26.38 KB

New patch:

  • ensures that third_party settings are stored in the settings array - not entirely sure this is correct but we have a lot of code manipulating the settings array so it might be easiest. I've move the test code from field_test to field_third_party_test so everything is a bit more "third party" (I'm not a huge fan of this description as I'm unsure of who the second party is). @heddn if we're going to move the third party settings out of the settings array we're going to have to make changes to all the field formatter and widget constructors.
  • Added schema and tests for it
  • Removes setThirdPartySettings and getThirdPartySettings as these general functions are unused and I can't see why we need them.

Noticed that the ajax forms on manage display and manage form display are interesting since you click on the cog and update the settings but nothing is saved until you press save on the main form - however unlike views or when you re-order the fields there is no you've made changes warning.

Status: Needs review » Needs work

The last submitted patch, 80: 2005434.79.patch, failed testing.

yched’s picture

I'm really -1 on placing '3rd_party_settings' inside 'settings'.

'settings' is entirely the playground of each individual widget / formatter, that are also the only ones to be asked for the config schema of entries underneath. The general widget / formatter API should not put stuff in there, nor assume some specific entry is present and has some specific system-wide semantics.

E.g 'weight' is a toplevel property, not a 'setting'. The new 'third_party_settings' property has exactly the same status.

That split between generic API features and plugin specific settings has been very structuring and very helpful since the early days of CCK, I'd really like to preserve it.

yched’s picture

Regarding terminology: I'm not married to 'third_party_settings', but the idea in #43 above was that having 'settings' & 'extra_settings' was a bit obscure ("why do some stuff lie in 'settings' and others in 'extra_settings' ? what's the difference ?)

"Third party setting" is clearer IMO as "coming from the outside".
The first two parties here are the Field system and the widget / formatter plugin, talking to each other.
3rd parties are other modules that have additional stuff to add to the conversation, whose semantics is unknown, but that we're kind enough to load and save alongside ;-)

IMO clarity wins over brevity and terminology shortcuts. Up for debate, of course - we should indeed come to an agreement before applying the same pattern and terminology in #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration :-)

swentel’s picture

Noticed that the ajax forms on manage display and manage form display are interesting since you click on the cog and update the settings but nothing is saved until you press save on the main form - however unlike views or when you re-order the fields there is no you've made changes warning.

We have #857312: Add a "changes not applied until saved" warning when changing widget/formatter settings for that - really old :)

Also agree with yched that these settings should not mingle with the existing 'settings' key.

+++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
@@ -121,9 +133,15 @@ function testFormatterUI() {
+    // Test the settings form behavior. We should need have an edit button since
+    // there are thrid party settings to configure.
     $edit = array('fields[field_test][type]' => 'field_no_settings', 'refresh_rows' => 'field_test');

'we should need have' sounds weird. Also, 'thrid'.

alexpott’s picture

Status: Needs work » Needs review
FileSize
20.68 KB
38.4 KB

Moved third party settings to their own array.

Fixed schema - this became quite painful since schema testing is broken. The patch attached includes #2287193: checkConfigSchema() can fail to report errors since it was impossible to validate the schema changes without it and in fact it identified errors in the current schema in HEAD.

Also fixed failing tests.

Status: Needs review » Needs work

The last submitted patch, 85: 2005434.85.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.36 KB
44.66 KB

Fixed some formatter and widget constructors.

Status: Needs review » Needs work

The last submitted patch, 87: 2005434.87.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.57 KB
44.14 KB

Fixed the tests, removed #2287193: checkConfigSchema() can fail to report errors from the patch and fixed a spelling mistake swentel spotted.

yched’s picture

Thanks folks ! This is getting close :-)

  1. +++ b/core/modules/entity/src/EntityDisplayBase.php
    @@ -297,6 +303,11 @@ public function setComponent($name, array $options = array()) {
    +    // Remove unnecessary empty third_party_settings.
    +    if (empty($options['third_party_settings'])) {
    +      unset($options['third_party_settings']);
    +    }
    

    Why is this needed ?
    In order to avoid isset() checks in the rest of the code stack, and for consistency of what gets ultimately saved in the config record, I'd think we should leave the entry even if it's an empty array() ?

    Also, if we do anything regarding 'third_party_settings' here, it should be done within the "if ($field_definition = $this->getFieldDefinition($name)) {" branch above - this is specific to Display conponents that are "Field API fields".

  2. +++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
    +++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
    @@ -108,6 +108,7 @@ public function getInstance(array $options) {
    

    The 'third_party_settings' key needs to be added :
    - to the phpdoc of
    FormatterPluginManager/WidgetPluginmanager::getInstance()
    - to the phpdoc of FieldDefinitionInterface::getDisplayOptions().

  3. +++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
    @@ -108,6 +108,7 @@ public function getInstance(array $options) {
         $configuration += array(
           'field_definition' => $field_definition,
    +      'third_party_settings' => array(),
         );
    

    We shouldn't have to fill in this entry at this point (we don't fill a default for 'setttings' either here). If there are cases where it's missing, then it's a sign something else is wrong upstream - might be related to the previous point.

    Same in FormatterPluginManager::getInstance()

  4. +++ b/core/modules/field_ui/field_ui.api.php
    @@ -11,65 +11,76 @@
    + * @param object $plugin
    + *   The field formatter.
    

    Nitpick : "The instantiated formatter plugin" ? and we could typehint on FormatterInterface

    (same for widget / hook_field_widget_third_party_settings_form())

  5. +++ b/core/modules/field_ui/field_ui.api.php
    @@ -11,65 +11,76 @@
    + *   The The (entire) configuration form array.
    ...
    + *   The The (entire) configuration form array.
    

    Double "The"

  6. +++ b/core/modules/field_ui/field_ui.api.php
    @@ -11,65 +11,76 @@
    +      '#default_value' => $plugin->getThirdPartySetting('my_module', 'my_setting'),
    

    We should add a note to the phpdoc of getThirdPartySetting() that, unlike 'settings' for which defaults are known and filled in by the Field API upstream, there is nothing that takes care of filling defaults for "third party settings".

    So if an extra setting should have a default that is not NULL, it is the responsibility of consuming code to handle the case of getThirdPartySetting() returning NULL.

    Alternatively, we could add a third param for the $default in getThirdpartySetting() (a bit like variable_get() ...)

  7. +++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
    @@ -84,4 +93,22 @@ public function setSetting($key, $value) {
    +  public function getThirdPartySetting($module, $key) {
    +    if (isset($this->thirdPartySettings[$module][$key])) {
    +      return $this->thirdPartySettings[$module][$key];
    +    }
    +    return NULL;
    

    Nitpick : could be done with a ternary (like getSetting() does).

    Also, see previous point about maybe adding a $default = NULL third param.

  8. +++ b/core/modules/field_ui/src/DisplayOverview.php
    @@ -231,13 +231,18 @@ protected function getFieldLabelOptions() {
       protected function alterSettingsForm(array &$settings_form, $plugin, FieldDefinitionInterface $field_definition, array $form, array &$form_state) {
    

    Method name is outdated, this is not an alter() anymore.

    --> Rename to thirdPartySettingsForm(), and let it return the generated subform ?

    (same in FormDisplayOverview)

  9. +++ b/core/modules/field_ui/src/DisplayOverview.php
    @@ -231,13 +231,18 @@ protected function getFieldLabelOptions() {
    +    // Iterate through the modules calling their
    +    // field_formatter_third_party_settings_form handlers (if any):
    +    foreach ($this->moduleHandler->getImplementations('field_formatter_third_party_settings_form') as $module) {
    +      // Index the settings form by module name.
    +      $settings_form[$module] = $this->moduleHandler->invoke($module, 'field_formatter_third_party_settings_form', array(
    

    Nitpick : those are not "handlers".

    --> maybe just :
    "Invoke hook_field_formatter_third_party_settings_form()", keying resulting subforms by module name."
    ?

    (similar in FormDisplayOverview)

alexpott’s picture

FileSize
15.82 KB
50.96 KB
  1. This just removes it from config object - if you're using the plugin it will always have an empty array. Therefore we don't have unnecessary isset() checks everywhere
  2. Fixed
  3. This means we don't have to have it always in the default configuration see #1
  4. I like it - nice.
  5. Fixed
  6. Fixed - went for a $default param
  7. see #6
  8. Fixed
  9. Fixed

If we want all entity display configuration to have empty third_party_settings keys then I'm happy to come back and change 1 and 3.

Status: Needs review » Needs work

The last submitted patch, 91: 2005434.91.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
541 bytes
50.21 KB

Lol - borked rebase. Patch reverts unrelated change in SchemaCheckTrait

yched’s picture

Sorry for being a pain :-) - I'd vote to always have 'third_party_settings' entries, even empty, in the config records. That's what we do for 'settings' already, introducing special / different treatment for 'third_party_settings' specifically seems like it would add more confusion / maintainability burden in the long run ?

Formatter/WidgetPluginManager::prepareConfiguration() takes care of filling default empty array entries for 'settings' & 'third_party_settings', I'd rather keep it here and limit the entry points where we need to do the "filling of defaults".

For data in Display config records, this prepareConfiguration() is run before save, when components get set in Display::setComponent() - and the patch partially undoes it with an unset(empty third_party_settings), just to fill it again in PluginManager::getInstance() when the config is used at runtime.

alexpott’s picture

FileSize
7.07 KB
55.66 KB

Not a pain - happy to discuss - and code. I'm not fussed to be honest. I've added third_party_settings to all the default config and now we save it even it is empty. I've also standardised all the default config to make the order in which it would be if the UI had created it. Side note: this revealed that the current entity.view_display.taxonomy_term.forums we completely borked,

yched’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks :-)

Works for me if green !

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: 2005434.95.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
57.63 KB

Fixing up the tests :)

Status: Needs review » Needs work

The last submitted patch, 98: 2005434.98.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
59.35 KB

Migration whack-a-mole!

yched’s picture

Status: Needs review » Reviewed & tested by the community

Weehee!

  • Commit 48bf025 on 8.x by catch:
    Issue #2005434 by alexpott, heddn, andrewmacpherson, swentel: Let 3rd...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed this. Any niggles I had were already discussed in depth above and the outcome looks right to me. Committed/pushed to 8.x, thanks!

andypost’s picture

is there any change notice?

xjm’s picture

@andypost, I don't think there's any real BC break?

xjm’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
    @@ -55,14 +55,17 @@
    -  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode) {
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings) {
    

    This additional constructor parameter isn't much of a BC break.

  2. +++ b/core/modules/field_ui/field_ui.api.php
    @@ -11,65 +11,76 @@
    -function hook_field_formatter_settings_form_alter(&$element, &$form_state, $context) {
    ...
    +function hook_field_formatter_third_party_settings_form(\Drupal\Core\Field\FormatterInterface $plugin, \Drupal\Core\Field\FieldDefinitionInterface $field_definition, $view_mode, $form, $form_state) {
    ...
    -function hook_field_widget_settings_form_alter(&$element, &$form_state, $context) {
    ...
    +function hook_field_widget_third_party_settings_form(\Drupal\Core\Field\WidgetInterface $plugin, \Drupal\Core\Field\FieldDefinitionInterface $field_definition, $form_mode, $form, $form_state) {
    
    +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -850,31 +872,32 @@ protected function saveDisplayStatuses($display_statuses) {
    -  abstract protected function alterSettingsForm(array &$settings_form, $plugin, FieldDefinitionInterface $field_definition, array $form, array &$form_state);
    +  abstract protected function thirdPartySettingsForm(PluginSettingsInterface $plugin, FieldDefinitionInterface $field_definition, array $form, array &$form_state);
    

    Oh, we did rename these hooks. That's happened since the last time I looked this over.

xjm’s picture

Assigned: Unassigned » xjm
Status: Fixed » Active

I'll start one.

xjm’s picture

Actually I was mistaken about the D7 code; both these hooks are new in D8. The ones I'm thinking of were already replaced in the plugin conversion. ;) So https://drupal.org/node/2130757 needs an update.

andrewmacpherson’s picture

Introduced hooks to alter formatter settings element in Field UI

This earlier change notice is now outdated. What happens now? Do we update that to describe the new third-party settings hooks, or does that notice get superceded by a new one?

andrewmacpherson’s picture

Oh, snap! I wrote the earlier chenge record. I'm happy to help with updating it to use the new third-party settings form hook, should have time this evening. I'm updating my contrib modules too.

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Fixed
Issue tags: -Needs change record updates

I've updated https://drupal.org/node/2130757. Checked with @alexpott and @swentel and we agreed that we don't need an 8.x-to-8.x CR here.

andypost’s picture

Updated change notice is great also each entity display needs to be re-saved to get new settings

+++ b/core/modules/forum/config/install/entity.form_display.taxonomy_term.forums.default.yml
@@ -10,10 +10,12 @@ content:
+    third_party_settings: {  }

+++ b/core/modules/forum/config/install/entity.view_display.taxonomy_term.forums.default.yml
@@ -5,7 +5,11 @@ mode: default
+    third_party_settings: {  }

I missed that all entity displays should be updated...

andrewmacpherson’s picture

The change record updates look good, but I think we need a better title. It still has "alter", and only talks about formatters. How does this sound?

"Introduced hooks to extend field formatters and widgets with third-party settings".

I am SO excited to see how third-party widget settings get used in contrib.

andrewmacpherson’s picture

Status: Fixed » Active
xjm’s picture

Status: Active » Fixed

I updated the title. I think any further improvements to that CR don't need to hold this issue open -- really it's a little weird to have a CR for contrib D7 code in the first place. :) Thanks @andrewmacpherson for reviewing it!

Gábor Hojtsy’s picture

So there is no mention of removing/deleting those settings on the issue or the patch. In #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields I tried to design the API to resemble this but needed to account for removing settings, especially since it may result in the removal of an entire config entity (since we have a config entity only for wrapping these additional settings as well). Eg. when you uninstall a module that used to provide those settings or when a module updates itself and needs to remove / rename settings or migrate a setting to another, etc. Not sure why is it not accounted for here?

yched’s picture

Yes, was thinking about that too (but i'm in a train...)
We have no code to remove the "third party settings" for $module when $module gets uninstalled.

Gábor Hojtsy’s picture

Well, not just for module uninstall but if module v1 has a setting names 'boo' and in v2 it needs to remove that and add a setting 'foo' instead then no way. It can add a new setting but not remove an old one.

alexpott’s picture

Gábor Hojtsy’s picture

Not a solution for #118 though.

xjm’s picture

yched’s picture

@Gabor #118 : how is that different from a formatter plugin adding / renaming some of its 'settings' in a new version ?
We don't have code to support that either ? (and didn't in D7 either)

andrewmacpherson’s picture

Made some more updates to the change record (https://drupal.org/node/2130757)
- updates in the D8 specimen code
- updated links to API docs

Status: Fixed » Closed (fixed)

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