Custom Display Settings - Order the view modes based on name rather than key

It wasn't immediately clear to me what was meant, based on the issue description, but I figured it out and have verified that your patch works beautifully. You are right that the display modes should be sorted by the label, not by the machine name. I imagine that if I changed the language, I would be able to see that the display modes are not in alphabetical order, but in English, the machine names of the default display modes and the labels correspond, so the alphabetical order is correct. To illustrate the problem, I created a new display mode, naming it "Apple" (just to make it appear at the top of the list). I then changed the name to "Wapple", which should have moved it to the bottom of the list, but it still appeared at the top. After applying your patch, it appears at the bottom. So I was able to observe the issue (on this test 8.4.x-dev install) without switching to a language where the display mode labels might not correspond to the machine names.

I love the simplicity of this one-line patch; tests aren't broken by it, and it does something that really makes sense, so I hope this can just be committed without further ado. :-)

I think this can be marked RTBC.

Often times in core we use natcasesort() for that instead of asort(), so I think that should be use here, as well. What do you think?

Description updated based on suggestion in #5. I think this makes the issue clearer to quickly understand.

I agree that natcasesort() is better. And it works just as well (better, I guess, since case-sensitivity won't affect the sort order). I've tested this locally with the same results as reported in #3. I think we can put this back to RTBC. :-)

Perfect, thanks for the updated patch @mfernea and thanks for testing again @LoMo!

Huh? In #5, the message shows that the patch passed, but #9, the system says it failed??? Did it fail in some other combination of PHP/DB versions? If so, why don't we get more information? In any case, I suspect a glitch.

Retesting...

Moving back to RTBC. The patch applies, the tests in #5 indicate pass, and it's such a simple patch I really cannot imagine it failed in some combination of environments (except due to a glitch), since any other reason would produce consistent failure (e.g. a test that expects the display modes to be displayed in order of machine name would consistently fail if the machine names and label names were not similar). Why is the system giving mixed messages here?

Now the page does correctly say that #5 failed. But having looked at the results, it seems some tests just aren't as stable as they could be. No single test is consistently failing, and the tests which have failed do not seem likely to be related to this issue, so we'll re-run... again.

Setting back to RTBC. Hopefully the tests won't glitch this time, but really they seem highly unlikely to be affected by this patch.

Tests seem to have passed this time (all of them), just like the first time. Only the 2nd and 3rd runs failed. I think this is safe to commit and does what it should.

This is a usability bug and should have an automated test to make sure we don't break it in the future. Discoverability and sensible orders of things help people use Drupal.

I'm working on building a test for this. Writing such tests falls into my area of expertise, but this will be my first "BrowserTest" for D8 core, so I'll be happy to have any feedback when I'm done.

I've created a browser test which I added to core/modules/field_ui/src/Form/EntityDisplayFormBase.php, where it seemed similar actions were already being tested (and where an environment is set up with the Article content type and an appropriate user). The test fails at the expected point if the original patch is not applied. And it passes if I apply the patch. My test uses patterns seen in other Functional (BrowserTest) tests already in core, which were useful examples.

Note that this patch includes the original single-line change to core/modules/field_ui/src/Form/EntityDisplayFormBase.php (no interdiff created since the original patch was so simple and mine only adds a function to one test suite (and a necessary permission to the test user setup).

Thanks, @mfernea. I figured there were probably some things that needed tweaks in there. Re the test in one patch, it's clear that this should fail, since the fix is not in the same patch, so this looks good.

I didn't realize $this->assertText() was deprecated. I guess that only applies to newer "Functional" tests and the assertText and assertUrl, etc, functions that are deprecated are still perfectly valid if extending one of the legacy Simpletest-based tests (which are slowly being replaced (?). Anyway, I see lots of uses of assertText(), so I guess the process of replacing it would be done in smaller chunks, replacing all such deprecated functions with their recommended replacements (e.g. one core module at a time)... and I assume there must be a roadmap for that.

Valid point made in #21, @mfernea. I'll need to come back to this. For functional tests like this, it's helpful to have a setup that supports the test without the need for great time overhead of creating a new environment/users, etc. That test class already had a test function that opened the same path (admin/structure/types/manage/article/display). Looking at the field_ui functional tests, perhaps it better fits in the EntityDisplayModeTest.php file/test class. I'll try to get back to that a bit later today.

Okay... that wasn't as simple as I hoped it would be. But it's working and in a more appropriate test class now (within the field_ui module, where it should be). We should see the test-only patch end with one failure and the "complete" patch should pass.

And the tests-only patch fails (as hoped) and the patch with tests passes (ditto). Looking good, I think... :-)

+++ b/core/modules/field_ui/tests/src/Functional/EntityDisplayModeTest.php
@@ -16,7 +16,7 @@ class EntityDisplayModeTest extends BrowserTestBase {
-  public static $modules = ['block', 'entity_test', 'field_ui'];
+  public static $modules = ['block', 'entity_test', 'field_ui', 'node'];

It is a bit unfortunate to be testing this with nodes. I think it should be possible to test this with the entity test entity type that is already being enabled. Did you try that? Or did you encounter any issues with that maybe?

Re #26 In some test classes it's appropriate to create an admin user (or whatever) and even log in as such during the setUp function. This test class had existing functions that did the on-the-fly create/login of user after attempting access (anonymous), so logging in during the setUp didn't seem appropriate. Nor did creating another user in the setUp, if it's only used in this test function and not the others. But creating a whole new test class seemed even less appropriate. If you look at the whole file and what other tests are doing, I think you'll understand why I just stuck to the same pattern in use here. But I agree and in the other test class (in the field_layout module) it was done that way. Extending existing test classes is often requires making compromises from what I'd like to do, but I prefer to find a reasonable "home" for a new test function in an existing class to creating a whole new test file. In this case, it's not a beautiful compromise, but I have spent way too much time on the test for this issue, especially given that the test is for a regression that probably will never happen (in this place) ever again, once this patch is committed.

Re #27. @tstoeckler: Yeah, I know. But I'm not making the test more complicated than it has to be, I promise. ;-) This bug only applies to the list of display modes seen on a content type display tab. The sort order bug actually doesn't affect the list of display modes (on admin/structure/display-modes/view, the sort order is already correct). For instance, on that page, if I rename "Teaser" to "Breezer", as this test does, it already appears at the top without this patch. The bug only comes into play when viewing the list of view modes on a content type (see screenshot for the issue). The following is a screenshot from admin/structure/display-modes/view, as seen after making the change the test does, and shows that the bug doesn't appear when just looking at the display-modes separate from a content type:

This looks really good. It had a failing test in #23 that proves that this fixes the problem.

+++ b/core/modules/field_ui/tests/src/Functional/EntityDisplayModeTest.php
@@ -140,4 +146,62 @@ public function testEntityFormModeUI() {
+    $this->drupalPostForm('admin/structure/display-modes/view/manage/node.teaser', $edit, t('Save'));
+    $this->assertSession()->pageTextContains(t('Saved the Breezer view mode.'));

I have a really small nit to pick, otherwise this is RTBC for me.

The patch doesn't need to have the t() here, as it's only installed in one language.

This is a really, really small nitpick, sorry.

The interdiff is the wrong way around, but in the patch it's fixed. Thanks!

Committed and pushed 93bf4cb540 to 8.6.x and 043563222a to 8.5.x. Thanks!

I'd love to see these actually sortable, as in my case I want to have a non-alpha order, ideally setting the order with crosshairs at /admin/structure/display-modes/view

Eg; my views modes are: 'Inline, Small, Medium, Large, Extra large' , but the alphabetical order gives them 'Extra large, Inline, Large, Medium, Small' - which is not a a tad frustrating for the user on embeds via WYSIYWG.

Any suggestions?