Problem/Motivation

The admin menu list (/admin/structure/menu) appears to sort alphabetically by menu title until the list exceeds one page. When the number of menus exceeds 50, the list is paginated and menus are then sorted 1) by machine name across all menus, then 2) alphabetically by menu title per listing page. If menu titles are changed such that they no longer align with the machine name, they can be difficult to find across a multipage list.

Steps to Reproduce

  1. Create/generate more than 50 menus (e.g. with devel)
  2. Change an early menu title on page 1 such that it should sort to the end of the entire menu list

You'll find that it only moves to the bottom of page 1, not to the end of the last page

Proposed resolution

Implement/override getEntityIds() in MenuListBuilder.php to explicitly sort the list query by menu title ('label').

Remaining tasks

Write a patch
Tests
Review
Commit

User interface changes

Menu list will be sorted alphabetically by menu title across the entire menu list.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

CommentFileSizeAuthor
#48 interdiff_20-48.txt5.68 KBranjith_kumar_k_u
#48 3070721-48.patch3.28 KBranjith_kumar_k_u
#23 AfterPatch#20.png25.96 KBasha nair
#23 BeforePatch#20.png27.72 KBasha nair
#20 interdiff_17-20.txt1.91 KBranjith_kumar_k_u
#20 3070721-20.patch3.86 KBranjith_kumar_k_u
#17 interdiff-16-17.txt945 bytesdanielveza
#17 3070721-17-test-only.patch2.97 KBdanielveza
#17 3070721-17.patch3.66 KBdanielveza
#16 3070721-16.patch3.63 KBdanielveza
#16 3070721-16-test-only.patch2.96 KBdanielveza
#9 sort_menu_list-3070721-9.patch686 bytesanmolgoyal74
#6 3070721-after_patch.png12.48 KBabhijith s
#6 3070721-before_patch.png17.01 KBabhijith s
#2 sort_menu_list-3070721-2.patch687 bytesjustcaldwell

Comments

justcaldwell created an issue. See original summary.

justcaldwell’s picture

justcaldwell
he/him
Primary language English
Location Austin, Texas
commented
Status: Active » Needs review
StatusFileSize
new sort_menu_list-3070721-2.patch687 bytes

Patch attached.

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.

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.

abhijith s’s picture

abhijith s commented
StatusFileSize
new 3070721-before_patch.png17.01 KB
new 3070721-after_patch.png12.48 KB

Applied patch #2 on 9.2.x and it works fine.The menu items are sorted by title in entire page after this patch.Adding screenshots

Before(Menu starting with 'z' on first page ):
before

After(Now the same menu items appers correctly as last item):
after

abhijith s’s picture

abhijith s commented
Status: Needs review » Reviewed & tested by the community
quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Just a brief look.

I see that the patch also has a coding standard error and there is no test for this. Adding tag for the test.

anmolgoyal74’s picture

anmolgoyal74 commented
StatusFileSize
new sort_menu_list-3070721-9.patch686 bytes

Updated CS.

anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This was tagged for tests in #8 that still need to happen.

Did not review the code.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs work » Needs review
StatusFileSize
new 3070721-16-test-only.patch2.96 KB
new 3070721-16.patch3.63 KB

Confirmed the issue exists in manual testing. Added a test.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
StatusFileSize
new 3070721-17.patch3.66 KB
new 3070721-17-test-only.patch2.97 KB
new interdiff-16-17.txt945 bytes

Fixing the code standards.

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Failure in #17 seems legit

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 3070721-20.patch3.86 KB
new interdiff_17-20.txt1.91 KB

Fixed test failures.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
Status: Needs work » Needs review
ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
asha nair’s picture

asha nair commented
StatusFileSize
new BeforePatch#20.png27.72 KB
new AfterPatch#20.png25.96 KB

Applied patch #21 in 10.x successfully and it fixes the issue. Now it sorts the entire menu list after updating. Adding screenshots for reference.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Not 100% sold on the test fix. As they use to pass before and this change shouldn't have caused those to fail. But will let the committer decide.

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

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.

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

The last submitted patch, 17: 3070721-17.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/menu_ui/src/MenuListBuilder.php
@@ -17,6 +17,23 @@ class MenuListBuilder extends ConfigEntityListBuilder {
+      ->accessCheck(TRUE)

    I don't think config entities can have query access, so this is redundant and can be removed. phpstan should no longer warn about this too, but it was at one point. 

  2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuListBuilderTest.php
@@ -0,0 +1,92 @@
+   * Tests the menus are listed alphabetically.
+   */
+  public function testMenusListedAlphabetically(): void {
+    // Test alphabetical order without pager.

    Is there an existing test that we could add an assertion to?

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
Status: Needs work » Needs review
StatusFileSize
new 3070721-48.patch3.28 KB
new interdiff_20-48.txt5.68 KB

Addressed #47, please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Points in #47 appear to be addressed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed 6ec16df and pushed to 11.x. Thanks!

  • longwave committed 6ec16df9 on 11.x 
    Issue #3070721 by DanielVeza, ranjith_kumar_k_u, justcaldwell,...

Status: Fixed » Closed (fixed)

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