WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 months ago

Last modified 9 days ago

#39776 closed enhancement (fixed)

add priority argument to add_submenu_page() function

Reported by: alexvorn2 Owned by: welcher
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.7.2
Component: Administration Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: administration Cc:
PR Number:

Description

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.

Attachments (15)

39776.diff (13.5 KB) - added by welcher 2 years ago.
Initial Patch
39776.2.diff (13.7 KB) - added by welcher 11 months ago.
Refreshed patch
39776.3.diff (13.7 KB) - added by welcher 8 months ago.
New simplified approach.
39776.4.diff (13.7 KB) - added by welcher 2 months ago.
Patch refresh for 5.3
39776.5.diff (17.1 KB) - added by welcher 2 months ago.
Adds unit tests and better logic around negative and large numbers
39776.6.diff (17.2 KB) - added by welcher 2 months ago.
39776.7.diff (17.2 KB) - added by welcher 2 months ago.
Don't use splitting logic if the position is 0
39776.8.diff (17.2 KB) - added by welcher 2 months ago.
Updates as per feedback
39776.9.diff (17.4 KB) - added by adamsilverstein 2 months ago.
39776.10.diff (17.4 KB) - added by adamsilverstein 2 months ago.
39776.11.diff (20.4 KB) - added by adamsilverstein 2 months ago.
39776.12.diff (21.1 KB) - added by adamsilverstein 2 months ago.
39776.13.diff (21.2 KB) - added by adamsilverstein 2 months ago.
skip helper tests in multisite
39776.14.diff (21.2 KB) - added by adamsilverstein 2 months ago.
whitespace
39776-15.diff (21.1 KB) - added by birgire 2 months ago.

Download all attachments as: .zip

Change History (56)

@welcher
2 years ago

Initial Patch

#1 @welcher
2 years ago

  • Keywords has-patch dev-feedback added

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.

#2 @welcher
2 years ago

Related #40927

#3 @alexvorn2
2 years ago

So can we move this to 4.9 Milestone?

#4 @welcher
2 years ago

  • Milestone changed from Awaiting Review to 5.0

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

#5 @welcher
2 years ago

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.

#6 @welcher
2 years ago

  • Owner set to welcher
  • Status changed from new to assigned

#7 @peterwilsoncc
14 months ago

  • Milestone changed from 5.0 to 5.1

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

#8 @desrosj
11 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to 5.2

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
11 months ago

Refreshed patch

#9 @welcher
11 months ago

  • Keywords needs-refresh removed

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

Last edited 11 months ago by welcher (previous) (diff)

#10 @desrosj
8 months ago

  • Keywords early added
  • Milestone changed from 5.2 to 5.3

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!

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


8 months ago

@welcher
8 months ago

New simplified approach.

#12 @welcher
8 months ago

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.

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


3 months ago

#14 @welcher
2 months ago

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

@welcher
2 months ago

Patch refresh for 5.3

#15 @adamsilverstein
2 months ago

  • Keywords needs-testing needs-unit-tests added; early removed

@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.

#16 @birgire
2 months ago

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?

#17 @welcher
2 months ago

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

@welcher
2 months ago

Adds unit tests and better logic around negative and large numbers

#18 @welcher
2 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@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
2 months ago

@welcher
2 months ago

Don't use splitting logic if the position is 0

#19 @welcher
2 months ago

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

#20 @birgire
2 months ago

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
2 months ago

Updates as per feedback

#21 @welcher
2 months ago

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

#22 @adamsilverstein
2 months ago

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

#23 @adamsilverstein
2 months ago

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

Last edited 2 months ago by adamsilverstein (previous) (diff)

#24 @adamsilverstein
2 months ago

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.

#25 @adamsilverstein
2 months ago

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
2 months ago

skip helper tests in multisite

@adamsilverstein
2 months ago

whitespace

@birgire
2 months ago

#27 @birgire
2 months ago

@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.

#28 @birgire
2 months ago

ps: 39776-15.diff also adds assert failure messages:

$this->assertSame( 'custom-position', $actual_position, 'Menu not inserted at the expected position with ' . $test );

to help with debugging on failures, as it's a loop of assertions.

Last edited 2 months ago by birgire (previous) (diff)

#29 @welcher
2 months ago

  • Keywords commit added; dev-feedback needs-testing removed

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

#30 @adamsilverstein
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#31 @adamsilverstein
2 months ago

  • Keywords needs-dev-note added

Nice work on this ticket @welcher & @birgire!

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

#32 @SergeyBiryukov
2 months ago

In 46198:

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

See #39776.

#33 @MattyRob
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#34 follow-up: @MattyRob
2 months ago

  • Resolution set to worksforme
  • Status changed from reopened to closed

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.

#35 @SergeyBiryukov
2 months ago

  • Resolution changed from worksforme to fixed

#36 in reply to: ↑ 34 @SergeyBiryukov
5 weeks ago

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

#37 @SergeyBiryukov
2 weeks ago

In 46674:

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

See #39776.

#38 @SergeyBiryukov
13 days ago

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.

#39 @youknowriad
9 days ago

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.

#41 @SergeyBiryukov
9 days ago

  • Component changed from Menus to Administration
Note: See TracTickets for help on using tickets.