Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
ConfigEntityListTest fails currently with PostgreSQL as database backend.
This is related to #2235111: EntityQueryTest is fragile. EntityQuery does not do any sorting at the query-level so the results are arbitrary. It also limits to 50 records. EntityQuery should do a sort by default if we're going to rely on a specific sort in PHP as well. Config entities use weight. Or for change the implementation for Config entities only. Or change how ListBuilder uses EntityQuery.
Proposed resolution
Explicitly sort by entity id in EntityListBuilder::getEntityIds because that is what the Drupal application expects in tests and in practice. The list will be sorted by property at a later time by EntityListBuilder although this deserves a follow-up because a pager limiting entities returned might conflict with a sort by a different property later. A follow-up issue was created for discussing changing the SQL and PHP sort, #2447805: EntityListBuilder::getEntityIds sort by label.
- This passes EntityListBuilderTest(s) and ConfigEntityListTest on SQLite.
- The patch to use is #4: drupal-2443636-get-entity-ids-missing-sort-4.patch
Remaining tasks
None.
User interface changes
None.
API changes
This changes the behavior of getEntityIds method to return a sorted list where before the list was returned unsorted. On MySQL and MySQL-like databases systems "unsorted" actually meant "sorted by primary key" so that there is no difference on those platforms.
Beta phase evaluation
| Issue category | Bug |
|---|---|
| Issue priority | Critical: PostgreSQL Support |
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | drupal-2443636-get-entity-ids-missing-sort-4.patch | 710 bytes | mradcliffe |
Comments
Comment #1
bzrudi71 CreditAttribution: bzrudi71 commentedComment #2
mradcliffeComment #3
mradcliffeThis is going with fixing getEntityIds to have a default sort. It makes sense to me that if I want entity ids via a pager, then the method should query by the default sort expected by the application and tests. The list builder classes will sort by an entity property later.
I think this deserves a follow-up DrupalWTF issue because as a developer expecting a truncated list to be sorted by weight doesn't make sense. I would want to sort either before the pager or return all entity ids and do the expected sort by weight.
This passes tests locally and may also solve other entity order by tests.
Comment #4
mradcliffeUrk. I forgot to upload the patch. :(
Comment #5
bzrudi71 CreditAttribution: bzrudi71 commentedHa... nice find! I spend a complete hour this morning to get this *$%"$" list builder doing what I expect from it - and yes, this may also help with other fails around.
Good work @mradcliffe!
Comment #6
BerdirFrom a user perspective, wouldn't it be more useful to sort on the label instead of the ID?
Comment #7
daffie CreditAttribution: daffie commentedIt all looks good to me.
I can confirm that the test fails for postgreSQL and with the patch the test passes for postgreSQL.
So for me it is RTBC.
@Berdir: If you think that an other solution is better please say so.
Comment #8
amateescu CreditAttribution: amateescu commented@daffie, that's exactly what he did?
I agree with #6, we should sort on label and fallback to ID if there's no 'label' entity key.
Comment #9
grom358 CreditAttribution: grom358 commentedAs per comment #6
Comment #10
grom358 CreditAttribution: grom358 commentedComment #12
grom358 CreditAttribution: grom358 commentedSorting by label doesn't work because the sort is applied by Query line 79
This does not work as expected for text labels.
Comment #13
grom358 CreditAttribution: grom358 commented@mradcliffe patch resolves the issue in regards to resolving the critical. Core makes assumptions in numerous places that using entity id sort order. So I have moved that as followup issue to here #2447805: EntityListBuilder::getEntityIds sort by label and it goes beyond the scope of the this issue.
Comment #14
grom358 CreditAttribution: grom358 commentedComment #15
mradcliffeThanks for working on this and creating follow-up, @grom358. I fell ill again earlier this week, and have been out of it and not been able to respond or do much of anything at all.
I agree. The scope of this issue should be limited to fixing the critical since there may have been some decisions made with regard to sort order.
This also may help SQLite for which the specification says that "If a SELECT statement that returns more than one row does not have an ORDER BY clause, the order in which the rows are returned is undefined." I'll assign this to me to look run the list builder tests on SQLite in vagrant. If it passes, then I'll move back to RTBC.
Comment #16
mradcliffePasses the same tests in SQLite.
Comment #17
mradcliffeForgot to change priority. I think that's correct based on the meta issue.
Comment #18
alexpottCommitted fd350fb and pushed to 8.0.x. Thanks!
Comment #20
amateescu CreditAttribution: amateescu commentedAll the other child issues of the critical parent meta issue are major, this one should be the same.