[regression] Menu List Builder getEntityIds() no longer calls getEntityListQuery

I see why this was done, now. You can't unset previously defined sorts.

\Drupal\Core\Entity\EntityListBuilder::getEntityListQuery does:

->sort($this->entityType->getKey('id'));

Any other sort calls would sort secondary to that one.

Opening an MR that uses a query tag to alter the query and fix the sorts. It's not great but the only option.

Something we could add test coverage for?

It feels more of a coding standard / PHPStan rule than test coverage. After reading 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC it seems like this could be a bugfix without a test, but should have a follow up for preventing this kinds of changes in entity list builders.

Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?

Also, this is true:

Is the fix achieved without adding new, untested, code paths?

Mind updating proposed solution

Fine with moving generic test coverage to a follow up.

Went ahead and opened the follow up.

Tweaked summary some.

But change didn't break anything and does seem like the policy could apply.

Thanks for opening, added notes there. Sorry, it's been on my list but kept getting buried.

Given this is not going to make 10.2.x can I suggest that we fix the original problem in a more generic and easier to override way - see https://git.drupalcode.org/project/drupal/-/merge_requests/8388

Oh nice so all you have to do is set a key. +1 from me but will let #mglaman mark it since he opened the issue.

Even better

alexpott → changed the visibility of the branch 3410091-regression-menu-list to hidden.

I read the issue summary, comments and the MR. I don't see any unanswered questions or missing core gate requirements. However, the proposed resolution is now incorrect.

Thanks to alexpott for the simple, flexible solution.

I have update the proposed resolution but I do have one question, about naming. Would it be better if the constant was something like 'ENTITY_SORT_KEY' or even 'ENTITY_QUERY_SORT_KEY' so that it is even more explicit when this key can be used.

I think the constant name is fine because it is protected (so not part of the public API) and inline with \Drupal\Core\Entity\EntityListBuilder::$limit - the whole class is about listing entities so adding entity to it's name doesn't give us much.

Committed/pushed to 11.x and cherry-picked to 10.4.x and 11.0.x. I didn't backport to 10.3.x because it's a new constant in classes that contrib can subclass.

xjm → closed merge request !8388

xjm → closed merge request !7890

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