"Manage display" : Formatter change not reflected on settings [#940668]

Split out from #934234: AJAX requests during drag-and-drop on the "Manage display" tabs in the Field UI breaks the page, which got a little confused.

When changing the selected formatter, the 'formatter settings' summary is not updated to match settings for the new formatter.

Patch + tests coming.

Comment	File	Size	Author
#10	field_ui_formatter_settings_fix-940668-10.patch	18.42 KB	yched
#10
#7	field_ui_formatter_settings_fix-940668-7.patch	18.42 KB	yched
#7
#5	field_ui_formatter_settings_fix-940668-5.patch	18.92 KB	yched
#5
#3	field_ui_formatter_settings_fix-940668-3.patch	18.42 KB	yched
#3
#2	field_ui_formatter_settings_fix-940668-2.patch	18.92 KB	yched
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

LaurentAjdnik CreditAttribution: LaurentAjdnik commented 14 October 2010 at 16:20

Tagging

Comment #2

yched CreditAttribution: yched commented 14 October 2010 at 16:43

Status:

Active

» Needs review

File	Size
field_ui_formatter_settings_fix-940668-2.patch	18.92 KB

Here's a patch + test (the test part is 90% of the patch size)

Bug was apparently caused by some recent changes around #limit_validation_errors (couldn't track a specific patch, through).
Until recently, form elements not present in #limit_validation_errors were hidden from $form_state['values'] in submit callbacks, but where still present in $form_state['values'] in the 'rebuilding' form callback. In current HEAD, they're hidden all along - which probably makes sense.

As a result, #limit_validation_errors cannot be used for the 'formatter change' event. It wipes too much from $form_state['values']. Manually specifying all the elements we do want in $form_state['values'] is too tedious, and not friendly for modules that form_alter() the form anyway.

I opened #941620: #skip_validation_errors : complement of #limit_validation_errors (D8 feature request) about that limitation. Meanwhile, we just remove #limit_validation_errors here. No biggie, will mainly reflect in that edge case:
- Edit settings for a formatter
- Leave a required field empty (or other invalid input)
- While still in edit mode, drag the row to 'Hidden' --> validation error (which should logically not matter, the field is being hidden anyway)

Patch comes with a minimal test for the very issue being fixed. I am moving forward on a much more extended test suite for the "Manage display" screen interactions (I'm as much tired of fixing bugs on this screen than Dries and webchick are of reviewing / committing them...), but it is blocked on a tricky behavior of 'image_button' FAPI elements within tests, which I'll describe in a separate thread.

Patch also :
- fixes a colspan bug when a row is in 'edit settings' mode
- takes care of the @todos regarding "drupal_add_js() vs $form['#attached']['js']", that were pending after #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework
I can obviously separate this in 3 patches, but the last two would require the 1st one to get in in order to be tested / reviewed, so I'd really rather not.

Comment #3

yched CreditAttribution: yched commented 14 October 2010 at 16:53

File	Size
field_ui_formatter_settings_fix-940668-3.patch	18.42 KB

Sorry, a cruft comment line slipped through.

Comment #4

effulgentsia CreditAttribution: effulgentsia commented 15 October 2010 at 18:29

Status:

Needs review

» Reviewed & tested by the community

Lovely, lovely patch. For anyone wanting to test manually: go to admin/structure/types/manage/article/display, and play with the "Body" format. You should be able to switch from "Default" to "Trimmed". When "Default", there should be no settings summary in the column to the right. When "Trimmed", there should be a settings summary that says "Trim length: 600". In HEAD, the settings summary is only updated when you click "Save". With patch, the settings summary is updated as you change the format. Note, with HEAD and patch, the changes aren't actually saved until you click "Save", and that is by design. The only difference is that with the patch, you can see the state of the settings that will be saved, and thereby, know to click the Gear icon if you want different settings. As mentioned in the OD, this was the original Field UI behavior when formatter settings were introduced, but regressed with HEAD changes. The patch includes very nice tests to prevent future regression of this.

Minor docs nits, which could be done in a non-critical follow-up, so RTBC. Though, yched, if you're around and want to re-roll with these changes, that's cool too.

+++ modules/field/tests/field_test.field.inc
@@ -258,6 +258,61 @@ function field_test_field_formatter_info() {
+  // The name of the setting depends on the field type.

Depends on the formatter, right? Not the field type.

+++ modules/field/tests/field_test.field.inc
@@ -258,6 +258,61 @@ function field_test_field_formatter_info() {
+  // The name of the setting depends on the field type.

See above.

+++ modules/field_ui/field_ui.test
@@ -31,6 +25,119 @@ class FieldUITestCase extends DrupalWebTestCase {
+   * @param $initial_edit
+   *   $edit parameter for drupalPost() on the first step ('Manage fields'
+   *   screen).
+   * @param $field_edit
+   *   $edit parameter for drupalPost() on the first step ('Field settings'
+   *   form).
+   * @param $instance_edit
+   *   $edit parameter for drupalPost() on the second step ('Instance settings'
+   *   form).
+   */

First step, second step, third step. Metaphysical question: can one take two first steps?

+++ modules/field_ui/field_ui.test
@@ -31,6 +25,119 @@ class FieldUITestCase extends DrupalWebTestCase {
+    // Check that the field doesn not appear in the overview form

s/doesn/does/, and end with a period.

Powered by Dreditor.

Comment #5

yched CreditAttribution: yched commented 15 October 2010 at 21:24

File	Size
field_ui_formatter_settings_fix-940668-5.patch	18.92 KB

"The name of the setting depends on the field type"
Doh - of course.
The other two are just moved around from existing code, but true, needs fixing.

New patch for those comment fixes, keeping RTBC.

Comment #6

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 17 October 2010 at 02:35

Status:

Reviewed & tested by the community

» Needs work

+          //'#value' => t('Edit @label settings', array('@label' => $instance['label'])),

Is that commented out because you were debugging something, or because we don't need it anymore? If the former, it needs to be uncommented, if the latter we should remove the line.