Make WordPress Core

Opened 5 years ago

Last modified 4 weeks ago

#52035 new defect (bug)

The `add_submenu_page()` position is ignored.

Reported by: howdy_mcgee's profile Howdy_McGee Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: administration Cc:

Description

Hello,

I'm not sure that the add_submenu_page() position argument is being applied properly. See the below example:

<?php
add_action( 'admin_menu', function() {
        
        add_submenu_page(
                'edit.php',
                'Foo',
                'Foo',
                'list_users',
                'foo',
                function() {
                        echo 'Hello Foo';
                },
                200,
        );
        
        add_submenu_page(
                'edit.php',
                'Bar',
                'Bar',
                'list_users',
                'bar',
                function() {
                        echo 'Hello Bar';
                },
                100,
        );
        
} );

I would expect "Bar" to appear before "Foo" since it has a lower position number. What ends up happening is whichever add_submenu_page() was called first, gets position priority.

Attachments (1)

52035.diff (988 bytes) - added by im_niloy 22 months ago.

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
5 years ago

  • Component changed from Menus to Administration

Thanks for the report!

Moving to the Administration component, as Menus is specifically for the Menus screen and nav menu functions.

This ticket was mentioned in PR #816 on WordPress/wordpress-develop by mukeshpanchal27.


5 years ago
#2

  • Keywords has-patch added

#3 @mukesh27
5 years ago

Hi there!

Above attached PR #816 fixes the menu position issue.

#4 @leogermani
4 years ago

Hi @mukesh27 ,

I just stumbled upon this bug and had a look at your patch.

It doesn't solve the problem if there are multiple submenus being registered because apparently array keys get reset and the last menu registered will always end up in the last position.

I suggest start by writing some tests so we can cover many different situations. I'm happy to help testing and giving feedback to move this ticket forward.

And in the meantime I'm going to look for a workaround to my problem :)

#5 @leogermani
4 years ago

Also, it just occurred to me that there would be a problem if 2 submenus are registered using the same position.

#6 @leogermani
4 years ago

Introduced in #39776

#7 @oglekler
2 years ago

  • Keywords needs-patch added; has-patch removed

@im_niloy
22 months ago

This ticket was mentioned in Slack in #core by im_niloy. View the logs.


21 months ago

This ticket was mentioned in PR #6971 on WordPress/wordpress-develop by @rajinsharwar.


20 months ago
#9

  • Keywords has-patch added; needs-patch removed

Fix menu position for the add_submenu_page()

Trac ticket: https://core.trac.wordpress.org/ticket/52035

#10 @rajinsharwar
20 months ago

  • Keywords needs-testing needs-unit-tests added

I changed the position of the code from @im_niloy, and removed the count() check.

#11 @rajinsharwar
20 months ago

  • Keywords needs-unit-tests removed

So according to https://core.trac.wordpress.org/ticket/39776#comment:18, it says "If a number larger than the number of items is passed, it is appended to the end". So, if the number passed is greater than the number of sub-menus, it will be auto appended in the end.

I have modified my PR to change this. Now the Menu items will be added in the array pretty much with the key of it's position passed. So now the array with the menu items would look like:

   [0] => Array
        (
            [0] => My Custom Page
            [1] => manage_options
            [2] => my_top_level_slug
            [3] => My Custom Page
        )
    [200] => Array
        (
            [0] => My Custom Page - 1
            [1] => manage_options
            [2] => my_top_level_slug
            [3] => My Custom Page - 1
        )
    [100] => Array
        (
            [0] => My Custom Page - 2
            [1] => manage_options
            [2] => my_top_level_slug
            [3] => My Custom Page - 2
        )

Of course, after the ksort, the arrays are arranged in ascending. this solves the issue that whenever any higher value is passed, it still follows the same rule of priority. Modified the unit test as well, so I do not think any new unit tests are necessary.

Feel free to add your thoughts!

#12 @rajinsharwar
20 months ago

@leogermani I have added a case for the one you mentioned in https://core.trac.wordpress.org/ticket/52035#comment:5.

Now if the menus have larger $position passed, then it adds float values to arrange accordingly. Like for below:

add_submenu_page( 'my_top_level_slug', 'My Custom Submenu Page - 1', 'My Custom Submenu Page - 1',
                'manage_options', 'my-secondary-slug', '', 200 );
add_submenu_page( 'my_top_level_slug', 'My Custom Submenu Page - 2', 'My Custom Submenu Page - 2',
        'manage_options', 'my-secondary-slug', '', 200 );

The output will be:

    [0] => Array
        (
            [0] => My Custom Page
            [1] => manage_options
            [2] => my_top_level_slug
            [3] => My Custom Page
        )

    [200] => Array
        (
            [0] => My Custom Submenu Page - 1
            [1] => manage_options
            [2] => my-secondary-slug
            [3] => My Custom Submenu Page - 1
        )

    [200.1] => Array
        (
            [0] => My Custom Submenu Page - 2
            [1] => manage_options
            [2] => my-secondary-slug
            [3] => My Custom Submenu Page - 2
        )

Let me know if we should add a unit test for this case.

#13 @leogermani
20 months ago

Hi @rajinsharwar, thank you so much for working on this!

Your patch works fine in my tests and solves all the issues.

I do think we could take this opportunity to add a few more tests though, it should be simple.

There are 2 things we are not testing:

  1. All the tests that use that data provider add only one submenu with a custom position, after adding several submenus with no position informed. One of the things we are fixing here is the addition of several submenus with custom positions, I think we should test that.
  1. Now that you have changed how the array keys are handled, I think we could have a better test to check if the menu fell in the right position.

The data provider you are passing on array( 123456, 123456 ) for example looks like a weak test to me. We are only testing that the key was preserved, but we are not testing whether ksort produced the expected outcome.

Maybe we could add a test that resets the array keys, or loops through the submenus, and test the actual position of each item.

Lastly, I'm not sure if this deserves a Dev Note, since we are changing how menus are stored, no longer with sequential numeric keys.

(let me know and I can help writing tests if you want)

Last edited 20 months ago by leogermani (previous) (diff)

#14 @Tarun.Parswani
13 months ago

#58062 was marked as a duplicate.

#15 @ozgursar
4 weeks ago

  • Keywords needs-testing removed

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6971

Environment

  • WordPress: 7.0-alpha-61215-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 143.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.4
  • MU Plugins: None activated
  • Plugins:
    • Code Snippets 3.9.4
    • Test Reports 1.2.1

Steps to Reproduce

  1. Install Code Snippets plugin to add the following snippet. (Alternatively add it into the functions.php of the theme)
add_action( 'admin_menu', function() {
        
        add_submenu_page(
                'edit.php',
                'Foo',
                'Foo',
                'list_users',
                'foo',
                function() {
                        echo 'Hello Foo';
                },
                200,
        );
        
        add_submenu_page(
                'edit.php',
                'Bar',
                'Bar',
                'list_users',
                'bar',
                function() {
                        echo 'Hello Bar';
                },
                100,
        );
        
} );
  1. In admin dashboard, view the Posts menu.
  2. Without patch Foo is displayed above Bar

Actual Results

  1. ✅ Issue resolved with patch.

Additional Notes

  • Tested submenu position by changing the last argument of add_submenu_page() function. Zero places the submenu item at the top and larger value places it below the items having lower values.

Supplemental Artifacts

Before:
https://i.imgur.com/EDSxLP8.png

After:
https://i.imgur.com/fWXdBTa.png

Note: See TracTickets for help on using tickets.