Check for int instead of null in new submenu priority implementation

[david.binda]

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

[123host]

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

[audrasjb]

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.

[david.binda]

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?

[123host]

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?

[david.binda]

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'] ).

[123host]

OK, I got it. Thanks again.

[dennis_f]

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' 
);

[SergeyBiryukov]

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.

[desrosj]

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.

[SergeyBiryukov]

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.

[david.binda]

+1 for adding _doing_it_wrong() message.

[desrosj]

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.

[audrasjb]

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.

[azaozz]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[123host]

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?

[SergeyBiryukov]

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.

[audrasjb]

In 52569:

Administration: Ensure an integer is used for menu priority in add_menu_page().

This change adds a verification of the $position parameter in add_menu_page() to ensure an integer is used. If not, the function informs developers of the wrong parameter type via a _doing_it_wrong message. This brings consistency with a similar check used in add_submenu_page().

This change also typecasts any floating number to string to ensure that in case a float value was passed, at least it doesn't override existing menus.

Follow-up to [46570].

Props kirtan95.
Fixes #54798. See #48249.