strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in functions.php

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