#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 |
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)
Change History (61)
#1
@
7 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.
#4
@
7 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
@
7 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.
#7
@
6 years 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
@
6 years 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.
#9
@
6 years ago
- Keywords needs-refresh removed
@desrosj I've updated the patch. The number is still a bit arbitrary but we can refine as needed.
#10
@
5 years 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.
5 years ago
#12
@
5 years 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.
5 years ago
#14
@
5 years ago
@desrosj @adamsilverstein I'd love some eyes on this - hoping for it to land in 5.3
#15
@
5 years 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
@
5 years 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
@
5 years ago
@adamsilverstein @birgire thanks for the feedback on this! I'll add those tests ASAP.
#18
@
5 years 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.
#19
@
5 years ago
Updated the logic to use array_unshift()
to prepend the item to the menu list instead of using the splitting logic.
#20
@
5 years 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.
#22
@
5 years ago
39776.9.diff adds some small doc improvements, adds a coverage
statement
#23
@
5 years ago
39776.10.diff includes fixes from running phpcbf
on the two files changed in this diff.
#24
@
5 years 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
@
5 years 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.
#26
@
5 years ago
39776.12.diff linting fixes
#27
@
5 years 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
@
5 years 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.
#29
@
5 years 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!
#31
@
5 years 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.
#33
@
5 years 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:
↓ 36
@
5 years 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.
#39
@
5 years 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.
#42
@
5 years ago
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
This ticket was mentioned in Slack in #core by afercia. View the logs.
5 years ago
#44
@
5 years ago
@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.
Initial Patch