add priority argument to add_submenu_page() function

[alexvorn2]

Add option to add new sub-menu pages with specific order - adding priority number, so if we will have a bigger priority the menu will have higher position in sidebar admin menus.

[welcher]

Initial Patch

[welcher]

Attached is an initial approach based on what is done in add_menu_page. The position numbers are not sequential, but rather multiples of 5. Adding a submenu item with a priority of 5 ( or lower ) will place it at the top of the list, 10 will be the second, 15 the third and so on.

This is due to how the menus are generated and output in includes/menu.php: https://core.trac.wordpress.org/browser/tags/4.7/src/wp-admin/includes/menu.php

The Appearance menu does deviate from this numbering system slightly which may cause some confusion when adding items to it.

I'd love a second opinion on this approach.

[welcher]

Related #40927

[alexvorn2]

So can we move this to 4.9 Milestone?

[welcher]

I think its too late for 4.9 but lets aim for 5.0

[welcher]

The ability to set the position is done with the attached patch.

However, I don't really like how the numbering seems arbitrary. 1 is the top but 5 is the second slot and then 10 is the third unless you are in the Appearance menu in which case 6 is the third.

I think we need a way to abstract that away so it's consistent and numbered in a sane fashion.

[peterwilsoncc]

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

[desrosj]

The most recent patch needs a refresh against trunk. @welcher are you able to refresh?

Punting to 5.2, but if it's ready before Beta on Thursday, this can be reconsidered.

[welcher]

Refreshed patch

[welcher]

@desrosj I've updated the patch. The numbering system is still a bit arbitrary but we can refine as needed.

[desrosj]

Thanks @welcher! The patch applies cleanly now.

5.2 beta is in just a few days. Since this still needs a proper review and testing, I am going to punt it to 5.3 and add the early tag so it can be well tested.

If a committer is confident in this for 5.2, then feel free to move it back!

[welcher]

New simplified approach.

[welcher]

I've reworked the patch with a new approach that standardizes the insertion point based on the number of items in the array instead of the actual index number.

The only thing I am a little concerned about is that part of this approach uses array_merge which doesn't preserve the array keys. Unit tests still pass but I'd love any other feedback on whether those keys are actually used anywhere that this change will break.

[welcher]

@desrosj @adamsilverstein I'd love some eyes on this - hoping for it to land in 5.3

[welcher]

Patch refresh for 5.3

[adamsilverstein]

@welcher Thanks for continuing to work on this.

It would be great to add some tests to verify this works as expected. I'm a little concerned in particular with the position slicing logic and possible edge cases, for example if the passed position exceeds the submenus array length. Unit tests would help confirm this always works as written. We can hopefully build off existing tests.

[birgire]

Agree with @adamsilverstein, adding unit tests seems ideal

An edge case comes to mind: What happens if position is negative? It seems array_slice() supports negative offsets. I guess it should be greater or equal to 0. Should that be reflected in the inline docs?

[welcher]

@adamsilverstein @birgire thanks for the feedback on this! I'll add those tests ASAP.

[welcher]

Adds unit tests and better logic around negative and large numbers

[welcher]

@adamsilverstein @birgire I've added some tests and made some changes to the logic. Now, if any negative number is passed, the new menu is added at the beginning of the list. If a number larger than the number of items is passed, it is appended to the end.

[welcher]

Don't use splitting logic if the position is 0

[welcher]

Updated the logic to use array_unshift() to prepend the item to the menu list instead of using the splitting logic.

[birgire]

Thanks for the tests @welcher

just few minor suggestions after quick skimming of 39776.7.diff​:

• Add @ticket 39776 on test method
• Avoid running code after assertions, it will not run if assertions fails.
• e.g. add @since 5.3.0 on helper test methods?
• Missing end of line dot, for some inline comments.

[welcher]

Updates as per feedback

[welcher]

@birgire thanks for your feedback, I've added a new patch.

[adamsilverstein]

39776.9.diff​ adds some small doc improvements, adds a coverage statement

[adamsilverstein]

39776.10.diff​ includes fixes from running phpcbf on the two files changed in this diff.

[adamsilverstein]

Nice work! This is looking really good @welcher.

For the tests, can you please add tests for all the touched functions here ( eg including add_management_page, add_options_page, add_theme_page, add_plugins_page, add_users_page, add_dashboard_page, add_posts_page, add_media_page, add_links_page, add_pages_page, add_comments_page) - that way we ensure none of these change unintentionally and all correctly support the priority parameter.

[adamsilverstein]

39776.11.diff​ Adds tests - runs the same priority tests for add_management_page, add_options_page, add_theme_page, add_plugins_page, add_users_page, add_dashboard_page, add_posts_page, add_media_page, add_links_page, add_pages_page and add_comments_page.

[adamsilverstein]

39776.12.diff​ linting fixes

[adamsilverstein]

skip helper tests in multisite

[adamsilverstein]

whitespace

[birgire]

@adamsilverstein Good idea to add tests for the other touched functions. Thanks for adding those.

The tests runs successfully on my install.

Just a minor thing, I justed noticed in 39776.14.diff​:

// Reset menus.
$submenu = [];
$menu    = [];

I think the array() form is now preferred regarding CS.

39776-15.diff​ expands on the PHPDocs and suggests a way to make sure no test code runs after the assertions.

[birgire]

ps: 39776-15.diff​ also adds assert failure messages, to help with debugging on failures, as it's a loop of assertions.

[welcher]

Thank you @adamsilverstein and @birgire for your updates here. Tests seem to be OK on my end. I think this is ready to go!

[adamsilverstein]

In 46197:

Menus: add a position argument to add_submenu_page and the helper functions that use it.

Add a position argument to the add_submenu_page function similar to the one already in add_menu_page. When adding sub menus enables setting the position in the sub menu where the item should appear.

In addition, add the position argument to functions that call add_submenu_page under the hood: add_management_page, add_options_page, add_theme_page, add_plugins_page, add_users_page, add_dashboard_page, add_posts_page, add_media_page, add_links_page, add_pages_page and add_comments_page.

Props welcher, birgire, alexvorn2.
Fixes #39776.

[adamsilverstein]

Nice work on this ticket @welcher & @birgire!

Adding needs-dev-note so we make sure document this new feature.

[SergeyBiryukov]

In 46198:

Docs: Add @since tag for the new $position argument added to add_submenu_page() and related functions in [46197].

See #39776.

[MattyRob]

Forgive me if I'm missing something massively obvious here but...

New line 1380 is as follows:
if ( $position >= count( $submenu[ $parent_slug ] ) ) {

$pareng_slug is passed as a string to the code WordPress functions like add_menu_page and add_submenu_page.

So, since these changes I'm seeing the following in my localhost admin:

Notice: Undefined index: s2 in /**/dev/src/wp-admin/includes/plugin.php on line 1380

Warning: count(): Parameter must be an array or an object that implements Countable in /**/dev/src/wp-admin/includes/plugin.php on line 1380

[MattyRob]

My bad - I was passing too many arguments to an add_submenu_page() call and have been for years, only now with the new parameter it's started causing issues.

[SergeyBiryukov]

Replying to MattyRob:

My bad - I was passing too many arguments to an add_submenu_page() call and have been for years, only now with the new parameter it's started causing issues.

Follow-up: #48249

[SergeyBiryukov]

In 46674:

Tests: Properly mark test_submenu_helpers_priority() as skipped for multisite, so it's not categorized as "risky".

See #39776.

[SergeyBiryukov]

In 46683:

Test: Don't skip the tests intended for single site when running on Multisite, add them to the ms-excluded group instead.

See #39776, #45747.

[youknowriad]

I've noticed some reports on the forums related to the code changes from the patches here. ​https://wordpress.org/support/topic/error-after-5-3-update

Some of you might know more.

[SergeyBiryukov]

Follow-up: #48599

[afercia]

I wasn't able to find this change mentioned in a dev-note post on Make and I see it's still marked with the needs-dev-note keyword, though it's fixed/closed against WordPress 5.3. Anyone know if a dev note was published and can point me to it, please? /Cc @jeffpaul

[JeffPaul]

@afercia I don't immediately see a dev note in my scan of the Field Guide either, will check with @ryanwelcher to see if he recalls anything being written.

[desrosj]

As far as I can tell, there was no dev note published for this ticket. I am going to remove the needs-dev-note keyword as the ship has sailed.

[SergeyBiryukov]

In 49112:

Tests: Use consistent wording for the $position parameter in add_submenu_page() tests.

This replaces the instances of $priority with $position, to match the actual parameter name and avoid confusion.

Follow-up to [46197].

See #51344, #39776.