#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 | 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)
#2
in reply to:
↑ 1
;
follow-ups:
↓ 3
↓ 5
@
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 acceptingstring
s, 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:
↓ 4
@
22 months ago
Replying to SergeyBiryukov:
Replying to jrf:
The function used
wp_normalize_path()
is clearly indicated as only acceptingstring
s, 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++the parameter++ is not a string?$path
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
@
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
@
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
@
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 :)
#7
@
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
@
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.
Thanks @ipajen for reporting this. The function used
wp_normalize_path()
is clearly indicated as only acceptingstring
s, so IMO, yes, this should be fixed by the plugins which are doing it wrong.Suggest: close.