WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 4 weeks ago

Last modified 25 hours ago

#48249 closed defect (bug) (fixed)

Check for int instead of null in new submenu priority implementation

Reported by: david.binda Owned by: desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: Administration Keywords: has-patch commit dev-reviewed i18n-change
Focuses: Cc:
PR Number:

Description

Similarly to the PHP warning reported in https://core.trac.wordpress.org/ticket/39776#comment:34 , I've run into multiple instances where developers are passing in an incorrect number of params to the add_submenu_page function when testing the 5.3 beta 2.

It's mainly due to a confusion with add_menu_page, which takes the $icon_url param, while the add_submenu_page does not.

Similar cases, which would produce issues, can be found in the wild. Some GitHub examples:
https://github.com/mallory/freeandsecure/blob/cce8e2928e63d117e56d771346d1e2514571efb2/wp-content/plugins/speakout/includes/admin.php#L19
https://github.com/miloman/Find-a-voice/blob/8a66e93b1dd69691ae19b481b0a8a5b4d52fa9f6/admin/admin_menu.php#L66

For proper backward compatibility of the new functionality, I'm proposing a better check for the type of the $position param. Currently the code is checking for null (default value/type in case the param is not set). But the code expects an int. So, perhaps, doing an is_int check would be more suitable.

Related ticket: #39776
Related revision: r46197

Attachments (2)

48249.diff (537 bytes) - added by david.binda 6 weeks ago.
48249.2.diff (845 bytes) - added by desrosj 4 weeks ago.

Download all attachments as: .zip

Change History (24)

@david.binda
6 weeks ago

#1 @SergeyBiryukov
6 weeks ago

  • Milestone changed from Awaiting Review to 5.3

#2 @123host
6 weeks ago

@davidbinda I am the developer of SpeakOut! that you have quoted as being an example.

Can you explain what issues this might cause and the solution? Is it to simply remove

, $petitions['icon_url']

from add_submenu_page?

Thanks.

Last edited 6 weeks ago by 123host (previous) (diff)

#3 @audrasjb
6 weeks ago

  • Milestone changed from 5.3 to Future Release
  • Owner set to audrasjb
  • Status changed from new to reviewing

Hi,
Thanks for the work done on this ticket.
Unfortunately, we are now at Beta 3 in the release cycle. I'm moving this ticket to Future release milestone.
Feel free to discuss it if you feel this ticket could land before 5.3 Release Candidate 1.

#4 @david.binda
6 weeks ago

Hey @123host,

Can you explain what issues this might cause

In the instances which I've been dealing with, the extra arguments (usually a string, due to the confusion with add_menu_page's $icon_url param), have led to PHP warnings and even to missing menu items. The codebase I've been examining is rather complex, so I don't have a full chain for missing menu items. But can confirm that the proposed patch, which is preserving existing functionality for cases where the $priority param is incorrectly used (w/o being noticed for years), fixes the issue.

and the solution? Is it to simply remove

, $petitionsicon_url?

from add_submenu_page?

Yes. Even if this patch lands in 5.3, then it would be better if all the add_submenu_page calls (as well as it's derivates, eg.: add_options_page) were using the correct number of args. Eg.: there is no icon URL for submenu pages - the 6th param of add_submenu_page in 5.3 is going to be a priority and current code expects it being an int.

@audrasjb , due to the breaking nature of the described bug , eventually leading to missing submenu pages (see above), in a code which is new in 5.3, I would personally vote for patching it in RC. Aren't the beta releases intended for identifying bugs in new code for the release, so it can be patched before the final one?

#5 @123host
6 weeks ago

Thanks so much @davidbinda, had it not been for the fact that you happened to use my plugin as an example and the matcher alerted me, I would never have been aware of this.

I notice at https://developer.wordpress.org/reference/functions/add_submenu_page/ that the 6th parameter is an optional callable function. Am I right in assuming that since I had $petitions['icon_url'] that is what would have thrown the warning.

But this is in 5.3, right?

#6 @david.binda
6 weeks ago

I notice at https://developer.wordpress.org/reference/functions/add_submenu_page/ that the 6th parameter is an optional callable function. Am I right in assuming that since I had $petitionsicon_url? that is what would have thrown the warning.

I'm referring to the calls which actually pass 7 arguments to the add_submenu_page calls. Eg.: https://plugins.trac.wordpress.org/browser/speakout/trunk/includes/admin.php#L19 . The 7th param in 5.3 beta is int $position, while the code in question is passing in a string ( $petitions['icon_url'] ).

#7 @123host
6 weeks ago

OK, I got it. Thanks again.

#8 @dennis_f
5 weeks ago

In one of my plugins I have a menu with several submenu items. Only for one of the submenu items the code passes the icon option mistakenly. This has been working fine until 5.3 and now with 5.3 this makes all of the submenu items (even the ones that are registered correctly) to disappear and produces some warnings.

The proposed patch fixes the issue, so I really hope we can see this land in 5.3. Of course, I'm going to fix the issue on my end as well, but I suppose that many others might experience the same issue due to a confusion of the parameters.

This can especially cause the default WordPress submenu items to disappear, in cases when plugins register submenu pages incorrectly under the default WordPress menu items. For example, if you register a submenu item under the WP Settings menu, and you pass an icon string parameter, all of the default WordPress settings submenu items (General, Writing, Reading, etc.) will disappear:

<?php
add_submenu_page(
    'options-general.php',
    'My settings',
    'My settings',
    'manage_options',
    'my_slug',
    'my_callback',
    'my-icon' 
);
Last edited 5 weeks ago by dennis_f (previous) (diff)

#9 @SergeyBiryukov
4 weeks ago

  • Keywords has-patch commit dev-feedback added
  • Milestone changed from Future Release to 5.3

Note that the issue is also reported on the original ticket, #39776, in comment:33:ticket:39776.

Passing extra arguments to add_submenu_page() might technically be a developer error, but since this is a regression that can cause default WordPress submenu items to disappear (as noted above), moving back to handle in 5.3 RC2.

The patch looks good to me, marking for a second committer's review.

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


4 weeks ago

#11 follow-up: @desrosj
4 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

48249.diff looks good to me as well.

However, I am wondering if a _doing_it_wrong() should be added here. This may not be possible to add for 5.3 (since we are in string freeze), but a follow up ticket could be created for 5.3.1.

Because passing incorrect parameters to the function is happening in the wild, that may help better educate developers when they are using add_submenu_page() incorrectly. Something like:

if ( ! is_int( $position ) ) {
	if ( null !== $position ) {
		_doing_it_wrong(
			__FUNCTION__,
			sprintf(
				/* translators: %s: add_submenu_page() */
				__( 'The seventh parameter passed to %s should be an integer.' ),
				'<code>add_submenu_page()</code>'
			),
			'5.3.0'
		);
	}

	$submenu[ $parent_slug ][] = $new_sub_menu;
} else {

Applying the commit dev-reviewed keywords so 48249.diff can be committed, but would like to know what others think of the _doing_it_wrong() suggestion.

#12 in reply to: ↑ 11 @SergeyBiryukov
4 weeks ago

Replying to desrosj:

However, I am wondering if a _doing_it_wrong() should be added here. This may not be possible to add for 5.3 (since we are in string freeze), but a follow up ticket could be created for 5.3.1.

Yes, this seems like a perfect use case for a _doing_it_wrong() message.

#13 @david.binda
4 weeks ago

+1 for adding _doing_it_wrong() message.

@desrosj
4 weeks ago

#14 @desrosj
4 weeks ago

48249.2.diff adds the _doing_it_wrong() notice. I expanded it slightly.

@SergeyBiryukov Consider this my sign off on both 48249.diff and 48249.2.diff. Since you have more i18n experience than me, I'll defer to your judgement about introducing this new string during RC. But, I think this one could be OK since this is not a user facing string.

#15 @audrasjb
4 weeks ago

  • Keywords i18n-change added

48249.2.diff looks great to me as well.
I think it would be better to directly implement this patch in 5.3, even if we are in hard string freeze: it's a totally new string, not a replacement of a previous string so polyglot’s previous work is not trashed.

#16 follow-up: @azaozz
4 weeks ago

Both 48249.diff​ and 48249.2.diff look good, but I tend to agree with @desrosj comment 11. String freeze is a string freeze :) It would be nice to have the doing-it-wrong now, but it's not absolutely essential, best to add it in a separate ticket later.

For this ticket 48249.diff seems better.

Last edited 4 weeks ago by azaozz (previous) (diff)

#17 @desrosj
4 weeks ago

  • Owner changed from audrasjb to desrosj
  • Status changed from reviewing to accepted

#18 @SergeyBiryukov
4 weeks ago

  • Component changed from Menus to Administration

#19 @SergeyBiryukov
4 weeks ago

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

In 46570:

Administration: Relax the default value check for the $position argument added to add_submenu_page() and related functions in [46197].

Due to a confusion with add_menu_page(), which takes the $icon_url parameter, while add_submenu_page() does not, some plugins were passing in a string instead of integer as $position, causing backward compatibility issues.

A _doing_it_wrong() message is now added to alert developers of the wrong parameter type.

Props david.binda, desrosj, 123host, dennis_f, MattyRob.
Reviewed by desrosj.
Fixes #48249.

#20 in reply to: ↑ 16 @SergeyBiryukov
4 weeks ago

Replying to azaozz:

For this ticket 48249.diff seems better.

Sorry, haven't refreshed the tab :)

We have one string change in #48386 and a few more changes in #47708, so I thought this one would be OK too, as it's not a user-facing string.

#21 follow-up: @123host
26 hours ago

I have an interesting thing happening. I have a plugin and in it I create a menu with submenus.

Since 5.3 some users have had an issue with the plugin and the way it calls add_submenu_page. But only some.

I couldn't reproduce this on my sites but have now worked with someone else where it was happening.

admin.php contains

$petitions = array(
		'page_title' => __( 'SpeakOut! Email Petitions', 'speakout' ),
		'menu_title' => __( 'SpeakOut!', 'speakout' ),
		'capability' => 'publish_posts',
		'menu_slug'  => 'dk_speakout',
		'function'   => 'dk_speakout_petitions_page',
		'icon_url'   => plugins_url( 'speakout/images/blank.png' )
	);

	$petitions_page = add_menu_page( $petitions['page_title'], $petitions['menu_title'], $petitions['capability'], $petitions['menu_slug'], $petitions['function'], $petitions['icon_url'] );

$addnew = array(
		'parent_slug' => 'dk_speakout',
		'page_title'  => __( 'SpeakOut!', 'speakout' ),
		'menu_title'  => __( 'Petitions', 'speakout' ),
		'capability'  => 'publish_posts',
		'menu_slug'   => 'dk_speakout',
		'function'    => 'dk_speakout_petitions_page',
		'position'	  => 1
	);
	
	$addnew_page = add_submenu_page( $addnew['parent_slug'], $addnew['page_title'], $addnew['menu_title'], $addnew['capability'], $addnew['menu_slug'], $addnew['function'] , $addnew['position']);

<more submenus snipped>

Note that I assign position a value not wrapped in quotes. This is fine on my sites but not on one I looked at.

It took ages to troubleshoot and discover that on this site instead it had to be 'position' => '1' - wrapped in quotes - so I changed the code and on my test site it now throw the same error they were having without the quotes. Warning: count(): Parameter must be an array or an object that implements Countable in /home/user/example.com/wp-admin/includes/plugin.php on line 1392 I do love that if you have a look at plugin.php there are actually only 900 or so lines :P

Is this a problem with my plugin or something going on in the core?

Last edited 26 hours ago by 123host (previous) (diff)

#22 in reply to: ↑ 21 @SergeyBiryukov
25 hours ago

Replying to 123host:

Is this a problem with my plugin or something going on in the core?

Thanks for the comment! We're tracking the issue in #48599.

Note: See TracTickets for help on using tickets.