Problem/Motivation

When applying one or multiple sorts to config entity queries the results are not necessarily correct for a single sort, and are wrong if multiple sorts are applied becuase of the PHP-based sorting in:

In \Drupal\Core\Config\Entity\Query\Query::execute()

Proposed resolution

Correctly implement the sort comparator to return 0 if two values are equal and either implement multiple key sorts correctly or at least document the fact that they do not work.

Remaining tasks

test-only patch
fix patch

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Issue fork drupal-2862699

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

Issue summary: View changes
Status: Active » Needs review
Related issues: +#2757207: Introduce "stable sort by weight" functions to sort arrays (and objects)
StatusFileSize
new1.04 KB

Here's an addition to the existing test to show a failure. The failure message basically shows that only the last sort applied matters. There's nothing in the interface the suggests that's the expected behavior as compared to a SQL query.

Failed asserting that Array &0 (
    0 => '6'
    1 => '4'
    2 => '1'
    3 => '2'
    4 => '5'
    5 => '3'
) is identical to Array &0 (
    0 => '1'
    1 => '2'
    2 => '3'
    3 => '4'
    4 => '5'
    5 => '6'
).

Status: Needs review » Needs work

The last submitted patch, 2: 2862699-2.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new2.78 KB

Here's with a recursive sorting function that should work for any number of sorts.

But wow - the tablesort behavior for config is weirdly broken also.

The current test assumes the added sort() call actually overrides the sort specified from the tablesort headers. With the fix, it will only add an additional sort on items that sort the same in the tablesort.

Status: Needs review » Needs work

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

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.

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.

m4olivei’s picture

Ran into another issue with the default sort wherein we expected it to sort ignoring case, but it doesn't do that, so an ASC sort on label returns:

Peach_Candor
Peach_Herald_Beauty
Peach_Red_Haven
Peach_loring

expected:

Peach_Candor
Peach_Herald_Beauty
Peach_loring
Peach_Red_Haven

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.

dpolant’s picture

To fix the issue m4olivei has described we'll need to use something other than the php comparator to sort the strings. strcasecomp() might work.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

bkline’s picture

Just as an aside, it seems that it would have been much less likely for this bug to have gotten this far if the sort() method had actually had some real documentation, explaining that it can be invoked more than once for a query. That would have encouraged people to actually use that capability at the outset, and maybe some unit tests would have even been created from the start to verify that the array in which the sort fields were stored was used correctly.

bkline’s picture

... we expected it to sort ignoring case, ...

Would it be possible to provide a pointer to that requirement? I don't doubt that it exists, but it's certainly not in the documentation for the sort() method. Also, wouldn't this be a separate bug? If discovering this second bug has everyone so stumped that it has blocked fixing the original, more serious bug for over two years, perhaps applying the principle of one bug, one ticket might get this one moving again. Just a thought.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

Issue tags: +Bug Smash Initiative, +Needs reroll, +Pittsburgh 2023
chrisdarke’s picture

Issue tags: -Pittsburgh 2023 +Pittsburgh2023

Migrating Pittsburgh 2023 to Pittsburgh2023 tag for cleanup

prem suthar’s picture

StatusFileSize
new3.06 KB

I have added the Re-rolled patch for 11.x.

welly made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

wim leers’s picture

Title: Sorts are broken for \Drupal\Core\Config\Entity\Query\Query » Multiple sorts are broken for config entity queries (\Drupal\Core\Config\Entity\Query\Query)
Issue tags: +Experience Builder, +Contributed project soft blocker
StatusFileSize
new842.5 KB

I just ran into this too for Canvas — quoting \Drupal\canvas\Controller\ApiConfigControllers::list():

    // If the Canvas config entity type has a weight, sort by it.
    if ($canvas_config_entity_type->hasKey('weight')) {
      $query->sort('weight');
    }

    // Always sort by ID as a secondary sort to ensure deterministic ordering
    // across databases.
    $query->sort($canvas_config_entity_type->getKey('id'));

… but due to the bug reported here, it is indeed impossible.

Illustrated:

👆 Note the 2 sorts correctly appear, after the first sort, the order is correct, but then just as we're about to execute the second sort, the original sort is overwritten.