Tried on fresh 8.4.x and also 8.3.5, to reproduce:
1. Enable 'Field Layout' module.
2. Edit some 'Content type', under 'Manage display' change 'Layout settings' to 'Two column', save.
3. Change formatter or its settings and save changes.
4. Try moving field to another region, observe the formatter/settings getting reset to defaults.

Also checked widgets under 'Manage form display' but they seem to work fine.

CommentFileSizeAuthor
#23 2896215-field_ui-23-PASS.patch4.21 KBtim.plunkett
#23 2896215-field_ui-23-FAIL.patch2.16 KBtim.plunkett
#18 2896215-18.patch2.3 KBanybody
#9 2896215-9.patch1.95 KBborisson_
#9 interdiff-6-9.txt558 bytesborisson_
#6 2896215-6.patch1.94 KBtacituseu

Comments

tacituseu created an issue. See original summary.

tacituseu’s picture

tacituseu commented

Tracked down to core/modules/field_ui/field_ui.es6.js:

    regionChange(region) {
      ...
      // Restore the formatter back to the default formatter. Pseudo-fields
      // do not have default formatters, we just return to 'visible' for
      // those.
      const value = (typeof this.defaultPlugin !== 'undefined') ? this.defaultPlugin : this.$pluginSelect.find('option').val();

      if (typeof value !== 'undefined') {
        this.$pluginSelect.val(value);
      }
      ...
    },

Looks like it was changed in #2796581-130: Fields must store their region in entity displays, and is an unintended consequence of removing - Hidden - formatter, the previous behaviour was to only reset it when the field was un-hidden (see: 962eddec).

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Can you reproduce without Field Layout? If you tracked it to that file, it could be a wider bug in Field UI.

tacituseu’s picture

tacituseu commented

Without Field Layout there are only 2 regions 'Content' and 'Disabled', and then it sort of matches the previous behavior, with an added quirk though: when you move field 'Tags' from 'Content' to 'Disabled', formatter is reset to 'Label', but when you save it it changes to 'Rendered entity'.

Edit:
By previous behavior I mean, I recall it used to be that moving to 'Disabled' changed formatter to '- Hidden -', and moving out of 'Disabled' it was setting it to default, but I would not expect this when moving it between 'normal' regions.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tacituseu’s picture

tacituseu commented
Status: Active » Needs review
StatusFileSize
new 2896215-6.patch1.94 KB

Patch resets to default formatter only when field is moved out of 'Disabled' region, when moving between visible/displayed regions it preserves it.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Component: layout.module » field_layout.module
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
StatusFileSize
new interdiff-6-9.txt558 bytes
new 2896215-9.patch1.95 KB
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

This is a really nasty bug which already cost us a lot of time. We're happy to confirm this is RTBC. We tested it in several environments. It would be great to have this in the next core release!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Component: field_layout.module » field_ui.module
Issue tags: -Blocks-Layouts

Good catch, this was broken in #2796581: Fields must store their region in entity displays.
And yes, it's only reproducible when there are multiple regions, hence the need for Field Layout.
But the bug is in Field UI itself.

Ideally this would get functional JS test coverage, but I'm not sure that it's required.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2896215-9.patch, failed testing. View results

tacituseu’s picture

tacituseu commented
Status: Needs work » Reviewed & tested by the community

Bot acting up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

if we've broken it once we should definitely add test coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs tests
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

We're using the patch successfully since month now. It works great and fixes the problems.
So this should definitely become part of 8.6.x.

If I know how to write the tests I'd do it, but I fear it would be trial and error. :( Could someone perhaps point to a similar / reference implementation or mentor me for that?

All in all RTBC but missing tests.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs work » Needs review
StatusFileSize
new 2896215-18.patch2.3 KB

Rerolled patch against 8.7.x

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

The last submitted patch, 9: 2896215-9.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work

Still needs tests

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@tim.plunkett, sorry. If someone could give me a first code example how to write these tests correctly, I'd like to write them / add further tests. I'm not experienced in that yet.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2896215-field_ui-23-FAIL.patch2.16 KB
new 2896215-field_ui-23-PASS.patch4.21 KB

Here's a test (the FAIL patch is the interdiff)

The last submitted patch, 23: 2896215-field_ui-23-FAIL.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

The test seems to cover everything and the patch I uploaded was absolutely useless - so I think I can RTBC this.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Whao thank you so much for mentoring, tim.plunkett, I've learned a lot in the last issues you commented and the next time I'll be able to write the given tests on my own. I'll have a look if I see demand for further tests in this case, otherwise confirming RTBC. THANK YOU!!

Works on 8.6.x and 8.7.x

  • larowlan committed 1f03271 on 8.7.x 
    Issue #2896215 by tim.plunkett, borisson_, Anybody, tacituseu: Formatter...

  • larowlan committed 27229b5 on 8.6.x 
    Issue #2896215 by tim.plunkett, borisson_, Anybody, tacituseu: Formatter...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 1f03271 and pushed to 8.7.x. Thanks!

Cherry picked as 27229b5a37 to 8.6.x

Status: Fixed » Closed (fixed)

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