Make WordPress Core

Opened 23 months ago

Closed 16 months ago

Last modified 12 months ago

#57581 closed defect (bug) (invalid)

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

Reported by: ipajen's profile ipajen Owned by:
Milestone: Priority: normal
Severity: normal Version: 6.2
Component: General Keywords: PHP81 close 2nd-opinion
Focuses: Cc:

Description

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

Change History (9)

#1 follow-up: @jrf
23 months ago

  • Keywords close added

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.

#2 in reply to: ↑ 1 ; follow-ups: @SergeyBiryukov
22 months ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 6.3

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.

#3 in reply to: ↑ 2 ; follow-up: @jrf
22 months ago

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.

#4 in reply to: ↑ 3 @ipajen
21 months ago

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

#5 in reply to: ↑ 2 @hellofromTonya
21 months ago

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.

#6 @SergeyBiryukov
21 months ago

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 :)

Last edited 21 months ago by SergeyBiryukov (previous) (diff)

#7 @oglekler
17 months ago

  • Milestone changed from 6.3 to 6.4

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

#8 @hellofromTonya
16 months ago

  • Milestone 6.4 deleted
  • Resolution set to invalid
  • Status changed from new to closed

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.

#9 @sabernhardt
12 months ago

#60047 was marked as a duplicate.

Note: See TracTickets for help on using tickets.