Opened 10 months ago
Closed 4 months ago
#57580 closed defect (bug) (invalid)
strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in functions.php
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 6.2 |
Component: | General | Keywords: | PHP81 close |
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).
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 ); }
Change History (10)
#2
in reply to:
↑ 1
;
follow-up:
↓ 3
@
9 months ago
- Keywords 2nd-opinion added
- Milestone changed from Awaiting Review to 6.3
Replying to jrf:
The function used is clearly indicated as only accepting
string
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
@
9 months ago
Replying to SergeyBiryukov:
Replying to jrf:
The function used is clearly indicated as only accepting
string
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.
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
#4
@
9 months ago
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.
#5
follow-up:
↓ 6
@
9 months ago
- Keywords 2nd-opinion removed
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?
#6
in reply to:
↑ 5
;
follow-up:
↓ 10
@
9 months ago
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.
#7
@
7 months ago
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/
#9
@
5 months ago
- Milestone changed from 6.3 to 6.4
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.
#10
in reply to:
↑ 6
@
4 months ago
- Milestone 6.4 deleted
- Resolution set to invalid
- Status changed from new to closed
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.
Thanks @ipajen for reporting this. The function used is clearly indicated as only accepting
string
s, so IMO, yes, this should be fixed by the plugins which are doing it wrong.Suggest: close.