Problem/Motivation

The logic which decides whether to display the field plugin (formatter/widget) settings edit button currently tests whether there is any settings summary text to display. If so, the summary text and the edit button are displayed.

However, if there is no summary text, the edit button will not be shown, even if settings are available for the plugin. This is a bug; the decision to show the edit-button should depend on whether settings are available.

This matters because hook_field_formatter_settings_form_alter() allows modules to add extra settings to formatters which don't already have settings. Meanwhile, the summary text for the extra settings needs to be set in hook_field_formatter_settings_summary_alter(), but there is no way to guarantee that summary text is being provided by that hook. (Likewise for the widget alter hooks.)

When a contrib module extends a formatter's settings, it doesn't always make sense to add any summary text, because when several modules extend formatter settings, the settings summaries can become overcrowded.

Steps to reproduce this bug

The problem isn't obvious when looking just at core; most (all?) of the core field formatters which have settings produce a summary message in all cases.

To reproduce this, you'll need a module that provides extra field plugin settings. I suggest using Field formatter class, which has a D8 version. It provides an extra setting to all field formatters, to add HTML classes to the display output. If an extra class is specified, it will be indicated in the settings summary message. (I've deliberately disabled the "no class" summary message in the latest 8.x-1.x-dev.)

Proposed Resolution

Refactor Drupal\field_ui\DisplayOverviewBase::buildFieldRow() so that the edit-button is shown whenever there are plugin settings available.

API changes

None.

Files: 
CommentFileSizeAuthor
#42 2029857-41.patch3.11 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,242 pass(es).
[ View ]
#42 2029857-41-test-only.patch2.21 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 59,213 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#37 2029857-37.patch3.5 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 59,158 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#37 2029857-37-plugin-definition.patch3.35 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 59,341 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#37 2029857-37-test-only.patch2.52 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 59,233 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#33 formatter_settings.png22.58 KBplopesc
#33 formatter_unwanted_settings.png13.66 KBplopesc
#33 2029857-33-test-only.patch2.04 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 59,389 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#33 2029857-33.patch3.02 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 59,047 pass(es).
[ View ]
#31 2029857-31.patch5.88 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 58,416 pass(es).
[ View ]
#31 interdiff.txt1.16 KBswentel
#28 2029857-28.patch5.9 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 58,586 pass(es), 9 fail(s), and 11 exception(s).
[ View ]
#28 interdiff.txt846 bytesswentel
#25 field_plugin_settings_edit-button-2029857-25.patch5.94 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 58,904 pass(es), 9 fail(s), and 11 exception(s).
[ View ]
#24 field_plugin_settings_edit-button-2029857-24.patch5.94 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#23 field_plugin_settings_edit-button-2029857-23.patch5.93 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#20 field_plugin_settings_edit-button-2029857-20.patch5.92 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,030 pass(es).
[ View ]
#20 interdiff.txt913 bytesswentel
#18 field_plugin_settings_edit-button-2029857-18.patch5.93 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 58,958 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#18 interdiff.txt3.33 KBswentel
#16 field_plugin_settings_edit-button-2029857-16.patch5.94 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#16 interdiff.txt3.34 KBswentel
#11 field_plugin_settings_edit-button-2029857-11.patch2.6 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 58,224 pass(es).
[ View ]
#11 interdiff.txt1.78 KBswentel
#9 field_plugin_settings_edit-button-2029857-9.patch837 bytesandrewmacpherson
PASSED: [[SimpleTest]]: [MySQL] 56,927 pass(es).
[ View ]
#7 field_plugin_settings_edit-button-2029857-7.patch848 bytesandrewmacpherson
PASSED: [[SimpleTest]]: [MySQL] 56,845 pass(es).
[ View ]
#1 field_formatter_settings_edit-button-2029857-1.patch2.48 KBandrewmacpherson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_formatter_settings_edit-button-2029857-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field_formatter_settings_edit-button-2029857-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I had a stab at fixing this. The attached patch seems to work okay, lets see if the tests are green.

The patch moves some code which generates the formatter settings form, outside of the if/else block, so that it gets generated whether we are in edit mode or not. Then separate checks are done to determine whether to display the summary and edit button respectivley.

Note that in order to test this, you'll need a module that provides extra field formatter settings. I suggest using the Field formatter class, which has an 8.x version. (I've deliberately disabled the "no class" summary message in the latest 8.x-1.x-dev.)

Makes sense to me. Tagging for a usability review.

Adding the usability tag ;-)

Status:Needs review» Needs work
Issue tags:+Needs usability review, +Field API

The last submitted patch, field_formatter_settings_edit-button-2029857-1.patch, failed testing.

Issue summary:View changes

adding the screenshot

Title:Field formatter settings edit button doesn't show unless there is a settings summaryField plugin settings edit button doesn't show unless there is a settings summary

Also applies to widget settings, since form modes were committed in #2014821: Introduce form modes UI and their configuration entity.

Updated title and issue summary.

Status:Needs work» Needs review
StatusFileSize
new848 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,845 pass(es).
[ View ]

This is a far simpler patch :-)

Issue summary:View changes

Bug affects field widgets as well as plugins since form modes were introduced. Added steps to reproduce.

You could simply check if ($plugin->getSettings())

Other than that, why not. Please check whether the phpdoc for the summary() methods in the widgets an formatters interfacesneed to be updated ?

StatusFileSize
new837 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,927 pass(es).
[ View ]

Okay, this patch uses ($plugin->getSettings())

Can we leave checking documentation to a separate issue?

Can we leave checking documentation to a separate issue?

Not really :-). If the changes here make existing docs incorrect/obsolete, they should be updated accordingly in the patch too.
I.m only talking about checking the existing docs for settingsSummary() and settingsForm() in FormatterInterface because they might mention the current behavior (button only if summary not empty). Maybe they don't and everything's fine, it's just that I'm away from my coding env and checking the issue queues with my phone, so it's not easy for me to check the code myself :-) - but TBH I'd be surprised if the current behavior isn't docimented anywhere.

StatusFileSize
new1.78 KB
new2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,224 pass(es).
[ View ]

Yeah, there was a mention in the interface.

Status:Needs review» Reviewed & tested by the community

Thanks @swentel, forgot about this one.
Right, should be good then :-)

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

We should have a test for this so we don't break this during future refactorings.

Title:Field plugin settings edit button doesn't show unless there is a settings summaryField plugin settings edit button doesn't show unless there is a settings summary or when moving a field from hidden to content
Priority:Normal» Major

Going to raise this to major. The settings form also doesn't work anymore if you move a field from hidden to content, you can't toggle the settings button as long as you haven't saved the the display.

Title:Field plugin settings edit button doesn't show unless there is a settings summary or when moving a field from hidden to contentField plugin settings edit button doesn't show unless there is a settings summary
Priority:Major» Normal

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new3.34 KB
new5.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Now with tests

Status:Needs review» Needs work

Hmm the test is actually, wrong, one second.

Status:Needs work» Needs review
StatusFileSize
new3.33 KB
new5.93 KB
FAILED: [[SimpleTest]]: [MySQL] 58,958 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Fix

Status:Needs review» Needs work

The last submitted patch, field_plugin_settings_edit-button-2029857-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new913 bytes
new5.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,030 pass(es).
[ View ]

This is green.

Status:Needs review» Reviewed & tested by the community

Looks good.

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

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.93 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

rerolled

StatusFileSize
new5.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Ok, this should be better, too lazy for interdiff though.

StatusFileSize
new5.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,904 pass(es), 9 fail(s), and 11 exception(s).
[ View ]

Dear me

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs usability review, +Field API, +Needs reroll

The last submitted patch, field_plugin_settings_edit-button-2029857-25.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new846 bytes
new5.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,586 pass(es), 9 fail(s), and 11 exception(s).
[ View ]

The function signature for viewElements was changed.

#28: 2029857-28.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs usability review, +Field API

The last submitted patch, 2029857-28.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.16 KB
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,416 pass(es).
[ View ]

After the big Field renames ...

Status:Reviewed & tested by the community» Fixed

Committed 069902e and pushed to 8.x. Thanks!

Added missing @file doc block during commit

--- a/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/formatter/TestFieldEmptySettingFormatter.php
+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/formatter/TestFieldEmptySettingFormatter.php
@@ -1,5 +1,10 @@
<?php
+/**
+ * @file
+ * Contains \Drupal\field_test\Plugin\field\formatter\TestFieldEmptySettingFormatter
+ */
+
namespace Drupal\field_test\Plugin\field\formatter;

Status:Fixed» Needs work
StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,047 pass(es).
[ View ]
new2.04 KB
FAILED: [[SimpleTest]]: [MySQL] 59,389 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new13.66 KB
new22.58 KB

Hello

I've been testing this and I found an annoying behavior with patch committed in #31.

Problem is that it is checking this:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
@@ -355,6 +355,8 @@ protected function buildFieldRow($field_id, FieldInstanceInterface $instance, En
+        }
+        if ($plugin->getSettings()) {
           $field_row['settings_edit'] = $base_button + array(

Plugin can have settings, but not form settings. In that case, the settings button is shown, but once clicked, nothing happens.

It should check that there are an available setting form for that plugin instead of the existence of a settings array.

Steps to reproduce:

  1. Create a field which haa more than one available formatter, one of them ithout settings form (IE: Number float field)
  2. Select the formatter which has setings form (IE: Default formatter) and configure its settings
    formatter_settings.png
  3. Select the formatter which does not have configuration form (IE: Unformatted). After the AJAX, you'll see something like this:
    formatter_unwanted_settings.png
  4. Click on the configuration button, and check that nothing happens

Attaching patch that checks if the plugin has a config form instead of settings array.

Regards.

That's weird. When you select a different formatter, the whole row should be updated, and the presence of the "settings" button re-evaluated against the new formatter.

Status:Needs work» Needs review

Changing status. Let's see testbot...

This seems to be just a bug.

Issue summary:View changes

minor edit, plugin instead of formatter.

Issue summary:View changes
StatusFileSize
new2.52 KB
FAILED: [[SimpleTest]]: [MySQL] 59,233 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.35 KB
FAILED: [[SimpleTest]]: [MySQL] 59,341 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.5 KB
FAILED: [[SimpleTest]]: [MySQL] 59,158 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Hello

I was talking yesterday with yched about this issue, and he suggested me to display or not edit plugin link depending on getPluginDefinition() provides plugin settings or not. It works unless hook_field_formatter_settings_form_alter() adds some form elements to a plugin that does not provide settings. In that case, edit plugin link is not displayed and you can't edit settings provided by hook_field_formatter_settings_form_alter().

Attaching 3 files, test-only, plugin-definition approach and settings form approach.

Any thoughts?

Regards.

It works unless
hook_field_formatter_settings_form_alter() adds some form elements to a
plugin that does not provide settings

Currently, a module that implements hook_field_formatter_settings_form_alter() typically also alters the formatter definition to add the corresponding settings - or else the form inputs wouldn't get saved.

This might change after #2005434: Let 3rd party modules store extra configuration in EntityDisplay / #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition, but let's see when those happen.

2029857-37-plugin-definition.patch simply fixes the implementation of the approach committed earlier, I 'd rather stick with that for now ?

We should add a comment explaining why $plugin->getSettings() isn't appropriate here.

The last submitted patch, 37: 2029857-37-test-only.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 37: 2029857-37.patch, failed testing.

The last submitted patch, 37: 2029857-37-plugin-definition.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
FAILED: [[SimpleTest]]: [MySQL] 59,213 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.11 KB
PASSED: [[SimpleTest]]: [MySQL] 59,242 pass(es).
[ View ]

OK, let's see if now we have it :)

Now checks getPluginDefinition(), created a new field_test formatter without any setting to gives test support.

Regards.

The last submitted patch, 42: 2029857-41-test-only.patch, failed testing.

Status:Needs review» Reviewed & tested by the community

Looks good. Thanks !

Status:Reviewed & tested by the community» Fixed

Committed bf3ac2f and pushed to 8.x. Thanks!

Fixed the following during commit.

git diff ./core/
diff --git a/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldFormatter/TestFieldNoSettingsFormatter.php b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/F
index 2eb76e0..1d3d150 100644
--- a/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldFormatter/TestFieldNoSettingsFormatter.php
+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldFormatter/TestFieldNoSettingsFormatter.php
@@ -3,7 +3,7 @@
/**
  * @file
  *
- * Contains \Drupal\field_test\Plugin\field\formatter\TestFieldNoSettingsFormatter.
+ * Contains \Drupal\field_test\Plugin\Field\FieldFormatter\TestFieldNoSettingsFormatter.
  */
namespace Drupal\field_test\Plugin\Field\FieldFormatter;

Status:Fixed» Closed (fixed)

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