WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 hours ago

#39776 assigned enhancement

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: Menus Keywords: has-patch dev-feedback needs-testing has-unit-tests
Focuses: administration Cc:

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 8 months ago.
Refreshed patch
39776.3.diff (13.7 KB) - added by welcher 6 months ago.
New simplified approach.
39776.4.diff (13.7 KB) - added by welcher 10 days ago.
Patch refresh for 5.3
39776.5.diff (17.1 KB) - added by welcher 9 days ago.
Adds unit tests and better logic around negative and large numbers
39776.6.diff (17.2 KB) - added by welcher 9 days ago.
39776.7.diff (17.2 KB) - added by welcher 9 days ago.
Don't use splitting logic if the position is 0
39776.8.diff (17.2 KB) - added by welcher 9 days ago.
Updates as per feedback
39776.9.diff (17.4 KB) - added by adamsilverstein 8 days ago.
39776.10.diff (17.4 KB) - added by adamsilverstein 7 days ago.
39776.11.diff (20.4 KB) - added by adamsilverstein 29 hours ago.
39776.12.diff (21.1 KB) - added by adamsilverstein 9 hours ago.
39776.13.diff (21.2 KB) - added by adamsilverstein 8 hours ago.
skip helper tests in multisite
39776.14.diff (21.2 KB) - added by adamsilverstein 7 hours ago.
whitespace
39776-15.diff (21.1 KB) - added by birgire 5 hours ago.

Download all attachments as: .zip

Change History (42)

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

Refreshed patch

#9 @welcher
8 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 8 months ago by welcher (previous) (diff)

#10 @desrosj
6 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.


6 months ago

@welcher
6 months ago

New simplified approach.

#12 @welcher
6 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 weeks ago

#14 @welcher
11 days ago

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

@welcher
10 days ago

Patch refresh for 5.3

#15 @adamsilverstein
10 days 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
10 days 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
10 days ago

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

@welcher
9 days ago

Adds unit tests and better logic around negative and large numbers

#18 @welcher
9 days 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
9 days ago

@welcher
9 days ago

Don't use splitting logic if the position is 0

#19 @welcher
9 days ago

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

#20 @birgire
9 days 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
9 days ago

Updates as per feedback

#21 @welcher
9 days ago

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

#22 @adamsilverstein
7 days ago

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

#23 @adamsilverstein
7 days ago

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

Last edited 7 days ago by adamsilverstein (previous) (diff)

#24 @adamsilverstein
7 days 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
29 hours 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
8 hours ago

skip helper tests in multisite

@adamsilverstein
7 hours ago

whitespace

@birgire
5 hours ago

#27 @birgire
5 hours 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.

Note: See TracTickets for help on using tickets.