Avoid errors from null parameters in add_submenu_page()

[ipajen]

6.2-alpha-55159, PHP 8.1

When using two different plugins (WP Review Slider Pro (Premium) or Ninja Forms following are thrown as PHP warning (If any of them are enabled same warnings are shown, if disabling both plugins no warning exist).

wp-includes/functions.php:7045
strpos()
wp-includes/functions.php:7045
wp_is_stream()
wp-includes/functions.php:2153
wp_normalize_path()
wp-includes/plugin.php:768
plugin_basename()
wp-admin/includes/plugin.php:1405
add_submenu_page()
wp-content/plugins/wp-review-slider-pro/admin/class-wp-review-slider-pro-admin.php:990
WP_Review_Pro_Admin->add_menu_pages()
wp-includes/class-wp-hook.php:308
do_action('admin_menu')
wp-admin/includes/menu.php:155

Is there something Core can do to improve the experience here? Or both plugin authors need to change?

wp-includes/functions.php:7045
/**
 * Tests if a given path is a stream URL
 *
 * @since 3.5.0
 *
 * @param string $path The resource path or URL.
 * @return bool True if the path is a stream URL.
 */
function wp_is_stream( $path ) {
	$scheme_separator = strpos( $path, '://' );

	if ( false === $scheme_separator ) {
		// $path isn't a stream.
		return false;
	}

	$stream = substr( $path, 0, $scheme_separator );

	return in_array( $stream, stream_get_wrappers(), true );
}

[jrf]

Thanks @ipajen for reporting this. The function used is clearly indicated as only accepting strings, so IMO, yes, this should be fixed by the plugins which are doing it wrong.

Suggest: close.

[SergeyBiryukov]

Replying to jrf:

The function used is clearly indicated as only accepting strings, so IMO, yes, this should be fixed by the plugins which are doing it wrong.

Should we consider adding a _doing_it_wrong() notice here if $path is not a string?

In a discussion with @joostdevalk, it was pointed out that this might help surface the problem faster, and would be a better developer experience.

[jrf]

Replying to SergeyBiryukov:

Replying to jrf:

The function used is clearly indicated as only accepting strings, so IMO, yes, this should be fixed by the plugins which are doing it wrong.

Should we consider adding a _doing_it_wrong() notice here if $path is not a string?

In a discussion with @joostdevalk, it was pointed out that this might help surface the problem faster, and would be a better developer experience.

That would be the only thing we could do in Core about this issue, but there is no structural plan on whether and how to validate the type of received parameters in functions in Core (well, I have a plan, but it's not like that will get approved anytime soon).

The current hap-snap "add a _doing_it_wrong to a function whenever someone reports an issue of a plugin doing silly things" is not helping anyone in the long run.

IMO we need a more structural approach. /cc @hellofromTonya

[SergeyBiryukov]

On a related note, per @joostdevalk, some plugins also call wp_add_inline_style() with null as$handle.

if we move forward with a _doing_it_wrong() notice here, wp_add_inline_style() could probably use a similar approach if $handle is null, via the existing _wp_scripts_maybe_doing_it_wrong() function.

[hellofromTonya]

There's a mixture of parameter checks in Core:

• Do nothing and let PHP handle reporting the incompatibility (which is the case here).
• Add a _doing_it_wrong() to clear identify why it's wrong.
• Add a trigger_error() to also identify why it's wrong.

The current hap-snap "add a _doing_it_wrong to a function whenever someone reports an issue of a plugin doing silly things" is not helping anyone in the long run.

Yes, I agree with @jrf. I'm not seeing an advantage or benefit of adding this code into Core, unless it's specifically to avoid hiding the type incompatibility report due to the nature of the code itself in its context.

IMO we need a more structural approach
on whether and how to validate the type of received parameters in functions in Core

Yes, I also agree with @jrf. Type validation is needed, but not in a haphazard and inconsistent way. Architectural decisions are needed. (Let's see if that can happen in 6.3). I don't think type validation though should exist to only duplicate reporting type mismatches. It should instead exist when needed within the context of the code it's being applied to.

In this specific report, PHP is reporting the type mismatch. The function expects a string type. Passing a non-string is doing it wrong. The fix should be in the plugins, not in Core.

I agree with @jrf that to close this ticket.

@SergeyBiryukov are you okay with closing it?

[SergeyBiryukov]

Replying to hellofromTonya:

I don't think type validation though should exist to only duplicate reporting type mismatches. It should instead exist when needed within the context of the code it's being applied to.

In this specific report, PHP is reporting the type mismatch. The function expects a string type. Passing a non-string is doing it wrong. The fix should be in the plugins, not in Core.

Yeah, I agree we should not duplicate PHP notices. My thinking was more along the lines of trying to improve developer experience by providing a more comprehensible message where possible, as an extender may not always be a seasoned developer, and the PHP notice may not always be clear in context, especially if the function is not called directly and the incorrect parameter is somewhere in the call stack.

[zoonini]

Just noting that a user reported this error coming up in Twenty Twenty-One, under specific circumstances, after updating to PHP 8.2:

​https://wordpress.org/support/topic/deprecated-notice-when-outputting-footer-thru-admin-post-php/

[retrovertigo]

Duplicate of #58104?

[oglekler]

Due to lack of activity and a patch, I am moving this ticket to the 6.4 milestone.

Additionally, if Twenty Twenty-One theme is causing an error, it needs to be fixed in theme as well separately from the solution which can be made in the Core itself.

[hellofromTonya]

Replying to SergeyBiryukov:

Yeah, I agree we should not duplicate PHP notices. My thinking was more along the lines of trying to improve developer experience by providing a more comprehensible message where possible, as an extender may not always be a seasoned developer, and the PHP notice may not always be clear in context, especially if the function is not called directly and the incorrect parameter is somewhere in the call stack.

I think a proposal and/or Trac ticket is needed for a project wide architectural discussion and structured approach across all of Core. I thought there was a ticket open for it, but not finding it. I'll check with @jrf and then move forward to create it if it doesn't exist or create a proposal for consideration.

I do think this ticket can be closed. The issue needs to be fixed in the plugin.

[sabernhardt]

Update: both plugins mentioned in the description fixed their use of add_submenu_page() (​review plugin's version 11.5.9 and ​forms plugin version 3.6.17).

The add_submenu_page() function has required strings for its first five parameters since [12914], but a ​directory search found many other plugins with null as at least one of those parameters. Maybe that could justify adding a _doing_it_wrong() message to add_submenu_page() and/or add_menu_page().

[sabernhardt]

#60046 was marked as a duplicate.

[sabernhardt]

#58104 was marked as a duplicate.

[sabernhardt]

#59756 was marked as a duplicate.

[insecttrojan]

In my case I debug and was the header-footer-code-manager and the wp-google-places-review-slider
​https://wordpress.org/support/topic/deprecated-strpos-4/

[sabernhardt]

@insecttrojan
Thanks for sharing the details! Those two plugins were also using null in add_submenu_page(), and both already corrected the type (in ​Header Footer version 1.1.33 and ​Google Review version 12.5).

[sabernhardt]

#59940 was marked as a duplicate.

[sabernhardt]

#59757 was marked as a duplicate.

[sabernhardt]

Core should edit the add_submenu_page() function. The Codex incorrectly recommended using null for $parent_slug more than ten years ago, and that example remains within a ​comment in the developer documentation.

One option is to check whether $parent_slug is empty before running plugin_basename(), and it could assign an empty string (or options.php) if it is empty.

$parent_slug = ( ! empty( $parent_slug ) ) ? plugin_basename( $parent_slug ) : '';

And if it checks for an empty $parent_slug, it probably could check $menu_slug in a similar way on the line above that.

Return early if $parent_slug or $menu_slug is empty.

Trac 57580

[sabernhardt]

The patch returns false early if either the $parent_slug or $menu_slug is empty, before running plugin_basename().

This is still rather minimal to prevent the PHP 8.1+ errors. It only checks two of the five required parameters, and it does not check specifically whether the parameters are strings.

[sabernhardt]

I edited the return docblock and added an inline comment. Improvements are welcome.

[swissspidy]

Not convinced that adding such a change is worth it. With a _doing_it_wrong we could at least tell developers that they should fix their code. But the current PR just makes the function silently fail.

Suggesting to close as wontfix.

[swissspidy]

#60636 was marked as a duplicate.

[sabernhardt]

#60982 was marked as a duplicate.

• Checks the two slug parameters for incorrect type before using them in plugin_basename.
• If either is not a string, the function prints an error message and returns false.

Trac 57580

[siliconforks]

Wouldn't it be better to make add_submenu_page() work without warnings using null as the first parameter, instead of failing?

As it stands now, isn't this PR a breaking change? Because right now this trick (of passing null) actually does in fact work - it just gives a warning. If this PR is applied it will break any plugin that is doing this.

[sabernhardt]

#61707 was marked as a duplicate.

[swissspidy]

#62011 was marked as a duplicate.