Make WordPress Core

Opened 16 months ago

Last modified 4 weeks ago

#57580 reopened enhancement

Avoid errors from null parameters in add_submenu_page()

Reported by: ipajen's profile ipajen Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: PHP81 has-patch dev-feedback
Focuses: administration 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 (29)

#1 follow-up: @jrf
16 months ago

  • Keywords close added

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.

#2 in reply to: ↑ 1 ; follow-up: @SergeyBiryukov
15 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 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 @jrf
15 months ago

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

#4 @SergeyBiryukov
15 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: @hellofromTonya
14 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: @SergeyBiryukov
14 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.

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

#7 @zoonini
13 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/

#8 @retrovertigo
11 months ago

Duplicate of #58104?

#9 @oglekler
11 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 @hellofromTonya
10 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.

#11 @sabernhardt
5 months ago

Update: both plugins mentioned in the description fixed their use of add_submenu_page() (review plugin's version 11.5.9 and forms plugin version 3.6.17).

The add_submenu_page() function has required strings for its first five parameters since [12914], but a directory search found many other plugins with null as at least one of those parameters. Maybe that could justify adding a _doing_it_wrong() message to add_submenu_page() and/or add_menu_page().

#12 @sabernhardt
5 months ago

#60046 was marked as a duplicate.

#13 @sabernhardt
5 months ago

#58104 was marked as a duplicate.

#14 @sabernhardt
5 months ago

#59756 was marked as a duplicate.

#15 @insecttrojan
5 months ago

In my case I debug and was the header-footer-code-manager and the wp-google-places-review-slider
https://wordpress.org/support/topic/deprecated-strpos-4/

#16 @sabernhardt
5 months ago

@insecttrojan
Thanks for sharing the details! Those two plugins were also using null in add_submenu_page(), and both already corrected the type (in Header Footer version 1.1.33 and Google Review version 12.5).

#17 @sabernhardt
4 months ago

#59940 was marked as a duplicate.

#18 @sabernhardt
4 months ago

#59757 was marked as a duplicate.

#19 @sabernhardt
4 months ago

  • Focuses administration added
  • Keywords needs-patch added; close removed
  • Milestone set to 6.5
  • Resolution invalid deleted
  • Status changed from closed to reopened

Core should edit the add_submenu_page() function. The Codex incorrectly recommended using null for $parent_slug more than ten years ago, and that example remains within a comment in the developer documentation.

One option is to check whether $parent_slug is empty before running plugin_basename(), and it could assign an empty string (or options.php) if it is empty.

$parent_slug = ( ! empty( $parent_slug ) ) ? plugin_basename( $parent_slug ) : '';

And if it checks for an empty $parent_slug, it probably could check $menu_slug in a similar way on the line above that.

This ticket was mentioned in PR #5970 on WordPress/wordpress-develop by @sabernhardt.


4 months ago
#20

  • Keywords has-patch added; needs-patch removed

Return early if $parent_slug or $menu_slug is empty.

Trac 57580

#21 @sabernhardt
4 months ago

  • Summary changed from strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in functions.php to Avoid errors from null parameters in add_submenu_page()

The patch returns false early if either the $parent_slug or $menu_slug is empty, before running plugin_basename().

This is still rather minimal to prevent the PHP 8.1+ errors. It only checks two of the five required parameters, and it does not check specifically whether the parameters are strings.

#22 @sabernhardt
4 months ago

  • Type changed from defect (bug) to enhancement
  • Version 6.2 deleted

#23 @mukesh27
4 months ago

  • Keywords changes-requested added

#24 @sabernhardt
4 months ago

  • Keywords changes-requested removed

I edited the return docblock and added an inline comment. Improvements are welcome.

#25 @swissspidy
3 months ago

  • Keywords dev-feedback added
  • Milestone changed from 6.5 to Future Release

Not convinced that adding such a change is worth it. With a _doing_it_wrong we could at least tell developers that they should fix their code. But the current PR just makes the function silently fail.

Suggesting to close as wontfix.

#26 @swissspidy
3 months ago

#60636 was marked as a duplicate.

#27 @sabernhardt
4 weeks ago

#60982 was marked as a duplicate.

This ticket was mentioned in PR #6415 on WordPress/wordpress-develop by @sabernhardt.


4 weeks ago
#28

  • Checks the two slug parameters for incorrect type before using them in plugin_basename.
  • If either is not a string, the function prints an error message and returns false.

Trac 57580

#29 @siliconforks
4 weeks ago

Wouldn't it be better to make add_submenu_page() work without warnings using null as the first parameter, instead of failing?

As it stands now, isn't this PR a breaking change? Because right now this trick (of passing null) actually does in fact work - it just gives a warning. If this PR is applied it will break any plugin that is doing this.

Note: See TracTickets for help on using tickets.