Problem/Motivation

The content element describes how and the order in which fields are displayed. Currently, fields are keyed by weight. This is a problem because when weights are changed, for example, the diff of yaml files will be large and more difficult to identify the actual change.

Proposed resolution

A better approach would be to key off field name, which would only change if entire fields are added or removed.

Remaining tasks

1. create the patch
2. write tests
3. review

User interface changes

none

API changes

none

Files: 
CommentFileSizeAuthor
#17 issue_2350537_comment_13.patch1.74 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,302 pass(es). View
#13 issue_2350537_comment_13.patch1.74 KBkatzilla
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,286 pass(es). View

Comments

webflo’s picture

Status: Active » Needs review
FileSize
643 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,908 pass(es). View
dawehner’s picture

I wonder whether we can have some test coverage for that.

In general I totally agree with the proposed solution. The common change is to change the priority not the machine name of a thing.

webflo’s picture

Issue tags: +Needs tests

Sure thats not a hard thing to do.

swentel’s picture

Yeah makes sense - I'm not sure if this has implications on Field UI - make sure we check this.

swentel’s picture

Also, I'm not sure it's useful to sort the hidden keys

dawehner’s picture

Also, I'm not sure it's useful to sort the hidden keys

Well they could also suffer from the reordering problem as well.

yched’s picture

Looking at the actual content of the yaml file, seeing components ordered by actual display order is much more readable.

Even when it comes to diffs, I'd say - if you move a component, then you see it removed somewhere (-) and added somewhere else (+), looks clear and reasonable ?

In fact, since Field UI's drag-n-drop renumbers weights anyway, the actual weights themselves are not relevant, and *that* leads to irrelevant diff hunks (moving one element around produces diff hunks for 'weight' changing in all the components that are after either its initial or final location).

If we stored the components in the right order but without a numeric weight, then the diffs would make more sense IMO.

Gábor Hojtsy’s picture

@yched: but what happens when you reorder AND change things? How can you tell that from the diff without looking very closely?

Gábor Hojtsy’s picture

yched’s picture

@Gabor:

what happens when you reorder AND change things? How can you tell that from the diff without looking very closely?

Like for code, yes.
Also, it's mentally closer to what you would see if you had a visual diff of a rendered node before/after the EntityDisplay change. Some parts have moved, some have changed, some did both. The parts that did neither do not appear in the diff.

I still think I'd prefer that, be for diffs or when looking at the content of one file, but well, i won't die if others feels strongly against it :-)

yched’s picture

Well, looks like @alexpott made "do not order by weight in exported YAML files" the official recommendation in http://chapterthree.com/blog/principles-configuration-management-part-one.

Still not really convinced personally, but I'll shut up now :-)

Berdir’s picture

I need this in #2248795: Entity displays fetch the bundle field definitions for deleted bundles, added it there as I also need to add it in a second place.

katzilla’s picture

FileSize
1.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,286 pass(es). View

Added the test for sorting

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: issue_2350537_comment_13.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,302 pass(es). View

No commit credit please - reuploading patch to preserve test result because it is unrelated and odd.

Berdir’s picture

Note that we have a slightly different patch in https://www.drupal.org/node/2248795

alexpott’s picture

@Berdir do you have a preference for doing the sort in the toArray() or preSave()?

yched’s picture

preSave() and toArray() are slightly different. preSave() changes the runtime object before it gets serialized to storage. toArray() is where the serialization happens. If we want to do stuff purely on the storage format, I'd say toArray() is the correct place.

Plus, doing the ksort in preSave() caused weird fails in #2248795: Entity displays fetch the bundle field definitions for deleted bundles that forced us to to the sort also in init(), where it doesn't belong. toArray() worked fine over there. See #37 / #38 / #43 over there.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

As per #18 and #20 then.

yched’s picture

Berdir’s picture

Sounds good to me. On mobile, so keeping it short.

alexpott’s picture

The thing is the tests break if you move the sort to toArray() because the when you save only what is written out is sorted so you have to reload the configuration entity from disk to get the new order.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.