Problem/Motivation

The order of the views displays get messed up in the config entity.

Proposed resolution

Sort views display always by display name in the config entity to generate minimal diffs in CMI.

Remaining tasks

None

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo’s picture

webflo’s picture

Title: Sort views display by display name » Sort views displays by display name

Status: Needs review » Needs work

The last submitted patch, 1: views-display-order-2350821.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
3.69 KB
3.69 KB

The failures are a little bit more tricky than expected.

Status: Needs review » Needs work

The last submitted patch, 4: 2350821-4.patch, failed testing.

webflo’s picture

FileSize
3.61 KB
1.15 KB

Reroll

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: 2350821-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
1.6 KB

There we go.

damiankloip’s picture

  1. +++ b/core/modules/views/src/Entity/View.php
    @@ -301,6 +301,11 @@ public function calculateDependencies() {
    +    ksort($display);
    

    This I am ok with. The position is more a UI thing anyway.

  2. +++ b/core/modules/views_ui/src/Tests/DisplayCRUDTest.php
    @@ -33,7 +33,7 @@ class DisplayCRUDTest extends UITestBase {
    +  public function ptestAddDisplay() {
    
    @@ -62,7 +62,7 @@ public function testAddDisplay() {
    +  public function ptestRemoveDisplay() {
    
    @@ -105,7 +105,7 @@ public function testDefaultDisplay() {
    +  public function ptestDuplicateDisplay() {
    

    Oops

  3. +++ b/core/modules/views_ui/src/ViewFormBase.php
    @@ -47,6 +47,21 @@ protected function prepareEntity() {
           if (empty($this->displayID)) {
    +
    +        uksort($tabs, function($a, $b) {
    

    Isn't a #weight property already added in getDisplaytabs()? Also, the hardcoding of page, block seems a bit meh.

dawehner’s picture

FileSize
2.84 KB
1.15 KB

Isn't a #weight property already added in getDisplaytabs()? Also, the hardcoding of page, block seems a bit meh.

Well, this is sadly a bit tricky to be honest. We do sort the displays with the following code:

      $tabs[$id] = array(
        '#theme' => 'menu_local_task',
        '#weight' => $display['position'],
        '#link' => array(

which means that the order of the tabs is (as you would expect) caused by the priority managed in the UI.
Meh I don't have a better suggestion for that at the moment but hardcoding here feels better than the clusterfuck of moving displays around all the time.

Oops

Where is the problem ;)

Gábor Hojtsy’s picture

alexpott’s picture

Issue tags: +Needs tests

Unfortunately #2361539: Config export key order is not predictable for sequences, add orderby property to config schema probably won't help here since displays is a sequence. Unless decide to have a new rule that all sequences are key sorted. Which would not be so silly imho since what we could then say is mappings are sorting according to schema, associated arrays (sequences) are key sorted and indexed arrays (sequences too) are saved in the order provided.

+++ b/core/modules/views/src/Entity/View.php
@@ -301,6 +301,11 @@ public function calculateDependencies() {
+    // Sort the displays.
+    $display = $this->get('display');
+    ksort($display);
+    $this->set('display', array('default' => $display['default']) + $display);

The one issue with this idea is code like this where we want the default to always come first.

alexpott’s picture

Status: Needs review » Needs work

Setting back to needs work because we totally needs tests for this type of change cause it is so easy to break.

Gábor Hojtsy’s picture

#2361539: Config export key order is not predictable for sequences, add orderby property to config schema would only help if we decide to introduce something like an "order_by_value_of_key: id" or something along those lines, where we would provide the name of key to take the value to order the sequences by. That may or may not be a good idea or in scope there. I would not postpone this issue on that.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.29 KB
5.13 KB

Adding views UI test.

Status: Needs review » Needs work

The last submitted patch, 16: 2350821-views-display-order-16.patch, failed testing.

The last submitted patch, 16: 2350821-views-display-order-16-test-only.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
5.83 KB
+++ b/core/modules/views_ui/src/Tests/DisplayOrderTest.php
@@ -0,0 +1,69 @@
+    // Add a new block display.
+    $this->drupalPostForm(NULL, array(), 'Add Block');
+    $this->assertLinkByHref($path_prefix . '/block_1', 0, 'Make sure after adding a display the new display appears in the UI');
+
+    // Test that the new view displays are in the correct order.
+    $view = Views::getView($view['id']);
+    $displays = $view->storage->get('display');
+    $expected_displays = array('default', 'block_1', 'page_1');
+    $this->assertEqual(array_keys($displays), $expected_displays, 'The display names are in correct order.');

We should save the view after we added the block display. Otherwise the changes are not persistent right?

I could not gork the uksort callback. I think is does not work in all cases. I tried to refactored it into something easier.

@alexpott

$this->set('display', array('default' => $display['default']) + $display);

This line should prioritize default.

The last submitted patch, 19: 2350821-views-display-order-16-19.interdiff.patch, failed testing.

vijaycs85’s picture

Thanks @webflo. looks good. here is the test-only patch for #19 to prove it fails.

/me saved the patch as 2350821-view-display-order-19-test-only.php. May be bed time :)

Status: Needs review » Needs work

The last submitted patch, 21: 2350821-view-display-order-19-test-only.patch, failed testing.

dawehner’s picture

The other implementation indeed looks a bit nicer.

  1. +++ b/core/modules/views_ui/src/Tests/DisplayOrderTest.php
    @@ -0,0 +1,70 @@
    +
    +    $this->assertNoLink('Master*', 0, 'Make sure the master display is not marked as changed.');
    

    Huch, I am confused, don't we hide the master display by default, so that there is also no 'Master' on that page? The text feels a bit misleading.

  2. +++ b/core/modules/views_ui/src/Tests/DisplayOrderTest.php
    @@ -0,0 +1,70 @@
    +    // Add a new block display.
    +    $this->drupalPostForm(NULL, array(), 'Add Block');
    +    $this->assertLinkByHref($path_prefix . '/block_1', 0, 'Make sure after adding a display the new display appears in the UI');
    +    $this->drupalPostForm(NULL, array(), t('Save'));
    +
    +    // Test that the new view displays are in the correct order.
    +    $view = Views::getView($view['id']);
    +    $displays = $view->storage->get('display');
    +    $expected_displays = array('default', 'block_1', 'page_1');
    +    $this->assertEqual(array_keys($displays), $expected_displays, 'The display names are in correct order.');
    +  }
    

    Should we also explicit check the order in the UI instead of just the config?

alexpott’s picture

Category: Task » Bug report
Priority: Normal » Major
Issue tags: +Configurables

This is a bug as can be seen from the patch attached to #2316909-17: Revisit all built-in test/default views configuration in core - the orders of things shouldn't change just because we save them. Also this kind of blocks progress on that issue so bumping priority.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.53 KB
2.48 KB
  • Ignored point 1), this was BS
  • Added a dedicated UI test for point 2)
alexpott’s picture

How come we're not sorting by weight in the UI?

alexpott’s picture

I've just postponed #2316909: Revisit all built-in test/default views configuration in core on this - so this is now critical

dawehner’s picture

@alexpott

Well, you know, people came up with that behaviour in order to improve UX in d7.
The page display is considered as more important than a block display, especially because on admin/structure/views/add
you see the same kind of order.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views_ui/src/ViewFormBase.php
    @@ -77,6 +78,36 @@ protected function prepareEntity() {
    +    $default = array_filter(array_keys($tabs), function ($value) {
    +      return (strpos($value, 'default') === 0) ? TRUE : FALSE;
    +    });
    

    This is unnecessary since the default display is only shown when it is the only display.

  2. +++ b/core/modules/views_ui/src/ViewFormBase.php
    @@ -77,6 +78,36 @@ protected function prepareEntity() {
    +    $page = array_filter(array_keys($tabs), function ($value) {
    +      return (strpos($value, 'page') === 0) ? TRUE : FALSE;
    +    });
    +
    +    $block = array_filter(array_keys($tabs), function ($value) {
    +      return (strpos($value, 'block') === 0) ? TRUE : FALSE;
    +    });
    

    This functionality appears untested since we add page_1 and block_1 in the test. If a view did have view displays with these ID then it would break the ability to reorder displays in the UI.

Basically I don't see the point of ViewFormBase::sortDisplayTabs() or the DisplayOrderTest test since views display re-ordering is tested in DisplayTest::testReorderDisplay()

alexpott’s picture

So #29.1 is wrong since this is configurable. but there is already code preventing you from re-ordering that so I still stand behind my statement that I don't see the need for the test or the code.

alexpott’s picture

Issue tags: +D8 upgrade path
olli’s picture

What is the problem with storing the displays in the same order as they appear in UI?

alexpott’s picture

@olli because then when all you do is re-order them you get a massive diff when all that has changed is the weight.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.34 KB
3.4 KB

So manual testing has proved that the master display always comes first in the UI and users can re-order to whatever they like and this is tested. So this patch should just concentrate on the order that displays are saved.

Patch attached does that.

Status: Needs review » Needs work

The last submitted patch, 34: 2350821.34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
899 bytes
4.28 KB

Ok we need to sort the tabs by weight to choose the correct one when no display is set.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @alexpott!!

Alright, Page still appears before block +1

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 186a80c on 8.0.x
    Issue #2350821 by dawehner, webflo, alexpott, vijaycs85: Sort views...
olli’s picture

#33: Ok, thanks!

I think it might be a good idea to add a method somewhere which you can use to get a list of view displays ordered by position (the same order as display tabs in UI). That could be used in views ui for "attach to", "link display" and in view area plugin.

Status: Fixed » Closed (fixed)

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