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.

[nprwp]

If you are going to make the parameter only accept a real string, which could be sensible if you want the callee to check for nulls, although personally I'd wish PHP could allow you to overload the function with a version that does accept null, checks for it, then calls the original just for obeying 'be liberal in what you accept, be conservative in what you emit' principle (but alas, there's no support for that), it results in an indirect problem too, which I think should be changed.

There's a way in which this interacts with another part of wordpress that causes quite a few plugins (including paid ones that are hard to update) to fail by using wordpress in a way that seems intended, given the documentation of this other part.

I'm referring to the function plugins_url(), which is used by plugins to serve files or otherwise figure out where they're installed, which may be important to use php's file writing/reading functions do do stuff.

plugins_url( null, __FILE__ )

is the typical way you see this function called, which then indirectly calls wp_is_stream. The function signature looks like this:

/**
 * Retrieves a URL within the plugins or mu-plugins directory.
 *
 * Defaults to the plugins directory URL if no arguments are supplied.
 *
 * @since 2.6.0
 *
 * @param string $path   Optional. Extra path appended to the end of the URL, including
 *                       the relative directory if $plugin is supplied. Default empty.
 * @param string $plugin Optional. A full path to a file inside a plugin or mu-plugin.
 *                       The URL will be relative to its directory. Default empty.
 *                       Typically this is done by passing `__FILE__` as the argument.
 * @return string Plugins URL link with optional paths appended.
 */

Notice how each parameter says "optional"? This usually, in terms of type hints, in a (strongly) typed language with no ability for optional parameters, as is the case with php7, which is still supported by wordpress, the normal way to do it is to pass NULLs for values you don't need if you need to pass parameter #2, but not parameter #1. This is very common for this function; as if your plugin has a simple file structure, it typically doesn't have a subdirectory for downloadable files but uses the regular path.

Inverting the order of the parameters (which was badly chosen) is not really an option because it'd break everyone's code. So you're left with only the option to ignore it and let plugins break everywhere or fix it for them.

This ability to pass both NULL and a value is represented as string? which is different from string in that it accepts nulls. Yet, you're effectively asking for a strict string in plugins_url while denoting the param as 'optional'; but it works for PHP8 in one way and for PHP7 in another.

Should this function gain a null check that casts the value to 'empty string' for its first parameter? A lot of plugins pass null and it appears to be a sensible thing to do for compatibility reasons, which then gets them snafu'd (and the user a bunch of warning messages that mess up their site) for choosing to use NULL instead of '' when sites update to PHP8.

[sabernhardt]

#63403 was marked as a duplicate.