str_replace(): Passing null to parameter #3 ($subject) of type array|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).

str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated

wp-includes/functions.php:2160
str_replace()
wp-includes/functions.php:2160
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?

functions.php:2160
	// Standardize all paths to use '/'.
	$path = str_replace( '\\', '/', $path );

[jrf]

Thanks @ipajen for reporting this. The function used wp_normalize_path() 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 wp_normalize_path() 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 wp_normalize_path() 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++the parameter++ 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.

For visibility I'm re-posting the reply I posted to the same question elsewhere:

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.

[ipajen]

Replying to jrf:

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.

It would be easier for a end user to know where to report the issue: the plugin author or WordPress with a _doing_it_wrong. Specially for one who don't know coding ;)

[hellofromTonya]

Replying to SergeyBiryukov:

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

While yes it may, how far should this go? Should all parameters in all functions be type checked with a _doing_it_wrong()? This kind of question is part of what @jrf is talking about, i.e. architectural discussions and considerations of how (or if) to handle function/method parameters input validation.

In this case, PHP is handling the notification that a type mismatch has happened, i.e. doing it wrong.

I agree with @jrf to avoid adding _doing_it_wrong() for each report but instead to shift focus towards the larger architectural and developer experience discussion of systematically and structurally handling this.

Also quoting @azaozz who has raised the flag that _doing_it_wrong() is not meant for type validation, but instead is for the most severe instances of doing something thing.

While I do sympathize and know how hard it can be to trace where the root cause is and where/how to report it, I agree with @jrf to avoid dripping type checks into the source code. I support closing this ticket in favor of a broader, encompassing architectural approach across Core's source code, rather than in a handful of places.

[SergeyBiryukov]

For visibility, re-posting my comment from #57580:

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.

That said, there are pretty good arguments above that adding _doing_it_wrong() notices on a case-by-case basis may not be the preferable solution in the long run, so I'm fine with closing in favor of a more structural approach if that's the consensus here :)

[oglekler]

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

[hellofromTonya]

The issue is in the plugin. I agree the fix needs to happen in the plugin. Thus, I'll close this ticket.

I also agree that an architectural, structured approach needs to happen. More information can be provided to help developers and users. But that discussion and effort should happen across all of Core, not just in this ticket. Thus, a new ticket and/or proposal is needed. I'll take the action item to get the ball rolling.

[sabernhardt]

#60047 was marked as a duplicate.