Make WordPress Core

Opened 12 months ago

Last modified 2 months ago

#59365 new defect (bug)

Deprecated notice when calling get_admin_page_title() on some dashboard pages.

Reported by: apedog's profile apedog Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: Administration Keywords: php81
Focuses: php-compatibility Cc:

Description

Deprecated	preg_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated	
wp-admin/includes/plugin.php:2092
preg_replace()
wp-admin/includes/plugin.php:2092
get_plugin_page_hookname()
wp-admin/includes/plugin.php:2056
get_plugin_page_hook()
wp-admin/includes/plugin.php:1980
get_admin_page_title()

This occurs on all pages where global $plugin_page is null (Plugins, Tools, etc.)

Change History (9)

This ticket was mentioned in PR #5231 on WordPress/wordpress-develop by nasimcoderex.


12 months ago
#1

  • Keywords has-patch added

Ticket : https://core.trac.wordpress.org/ticket/59365

### Description:
Check the string before using preg_replace method in wp-admin/includes/plugin.php file

Trac ticket:

#2 @hellofromTonya
2 months ago

  • Focuses coding-standards removed
  • Keywords reporter-feedback php81 added

Hello @apedog,

Welcome back to WordPress Core Trac.

Can you please provide step-by-step instructions on how to reproduce the deprecation notice? Other information that can also help:

  • What version of WordPress?
  • What plugin(s) are in your stack?
  • Are you seeing this issue with any plugin activated? If yes, are there specific URLs where it's happening?
  • "Dashboard" are you meaning the plugin's landing page or the default WordPress Dashboard page?

Why am I asking for all of this information?

I'm not able to reproduce this in trying various plugins (BuddyPress, bbPress, Yoast SEO, Query Monitor, etc.) and navigating to their settings page. In each case, the ?page=[plugin_hookname] is present in the URL.

Having the information requested helps contributors (like me) reproduce the issue as the starting point to understanding where to start with diagnosing where the root cause might be.

#3 follow-up: @hellofromTonya
2 months ago

Diving into the issue ...

The root case is not in the get_plugin_page_hookname() function.

Why? It requires a non-nullable string to be passed to its $plugin_page parameter (see the documentation for more information). Passing null to its $plugin_page parameter is incorrect and the deprecation notice is correct if this happens.

This means, the root cause is elsewhere in the call stack and needs to be fixed upstream of this function. That could be in how a plugin is registering its settings page, something is unsetting or setting the global variable to null, etc.

Are there clues in the call stack?

Possibly. The call stack (thank you for providing it):

  • get_admin_page_title() is invoked.
  • get_admin_page_title() invoked get_plugin_page_hook() and passed the global $plugin_page variable to it.
  • get_plugin_page_hook() invoked get_plugin_page_hookname() and passed $plugin_page to it.

get_plugin_page_hook() has no function parameters. Instead, it uses the global variables $title and $plugin_page. If $title is not empty, then the title is returned, exiting out of the function. Else, get_plugin_page_hook() is invoked, passing the global $plugin_page variable to it. (More on this in a moment.)

get_plugin_page_hook() also requires a non-nullable string to be passed to its $plugin_page parameter (see the documentation for more information).

So how would null be passed to the functions?

Both of these need to be true:

  1. The global variable $title is empty or not set.
  2. The URL does not have a ?page query parameter.

As noted in the description, it happens when the global variable is null.

This occurs on all pages where global $plugin_page is null (Plugins, Tools, etc.)

The Tools and Plugins admin page are built-in Core and have dedicated PHP files:

  • Tools admin page is admin/tools.php and has the global $title variable set to "Tools" within that file.
  • Plugins admin page is admin/plugins.php and has the global $title variable set to "Plugins" within that file.

If the deprecation notice is happening on those admin pages, then it means some code somewhere incorrectly setting (to null) or unsetting the global $title variable.

@apedog Can you please confirm if the deprecation notice is being thrown on the Tools and Plugins admin page please?

Last edited 2 months ago by hellofromTonya (previous) (diff)

@hellofromTonya commented on PR #5231:


2 months ago
#4

Thank you @nasimcoderex for the patch. However, it's not fixing the root cause. Let me explain (a more detailed explanation is here in the Trac ticket).

get_plugin_page_hookname() requires a non-nullable string to be passed to its $plugin_page parameter (see the documentation).

Passing null is incorrect. If null is passed to it, then the PHP native preg_replace() will throw the deprecation notice. The deprecation notice is informing of the incorrect data type received. (Currently) Core itself does not do incoming parameter data checks to ensure the incoming data is of the required type. Rather, it leans into it's documentation with the expectation of what's documented being the required data type(s) the function requires.

As this function is receiving a null instead of a string, the root cause is elsewhere. Thus, the fix is also elsewhere, i.e. where the root cause is (yet to be discovered).

I'm closing this PR. Once the root cause is found and if a Core fix needed, please consider opening a new PR to resolve it. Thank you 😄

#5 @hellofromTonya
2 months ago

  • Keywords has-patch removed

#6 in reply to: ↑ 3 ; follow-ups: @apedog
2 months ago

Replying to hellofromTonya:

This means, the root cause is elsewhere in the call stack and needs to be fixed upstream of this function. That could be in how a plugin is registering its settings page, something is unsetting or setting the global variable to null, etc.

Hello Tonya,
The issue indeed was caused by a plugin registering a settings page.

More specifically,
I had used get_admin_page_title() as the $title parameter in add_settings_section(). In hindsight, this is obviously wrong, as it would register a different title depending on the page being visited. However it would only print the title when I was on the correct settings page, so I thought nothing of it - less static strings for me to keep track of.

This worked fine up until I migrated to PHP 8+. I promptly fixed the "upstream" problem in my own plugin - and submitted the ticket at that time. I couldn't tell if this was indicative of a deeper issue or not.

@apedog Can you please confirm if the deprecation notice is being thrown on the Tools and Plugins admin page please?

Yes, the notice would appear on built-in core pages such as Tools and Plugins.

#7 in reply to: ↑ 6 @hellofromTonya
2 months ago

Replying to apedog:

The issue indeed was caused by a plugin registering a settings page.

More specifically,
I had used get_admin_page_title() as the $title parameter in add_settings_section(). In hindsight, this is obviously wrong, as it would register a different title depending on the page being visited. However it would only print the title when I was on the correct settings page, so I thought nothing of it - less static strings for me to keep track of.

Aha, that makes sense. add_settings_section() needs the title you want to appear in that settings page section. Pass it as a string or translatable string (e.g. through __()).

Can you please confirm if the deprecation notice is being thrown on the Tools and Plugins admin page please?

Yes, the notice would appear on built-in core pages such as Tools and Plugins.

That makes now too. Thank you for confirming.

This worked fine up until I migrated to PHP 8+. I promptly fixed the "upstream" problem in my own plugin - and submitted the ticket at that time.

It should still work fine with PHP 8, as it's a deprecation notice rather than a Warning or Fatal Error. Glad you got it fixed in your plugin.

#8 in reply to: ↑ 6 ; follow-up: @hellofromTonya
2 months ago

  • Keywords reporter-feedback removed

Replying to apedog:

I couldn't tell if this was indicative of a deeper issue or not.

While this specific issue is not a deeper issue within Core itself, it is an opportunity to improve the robustness and helpfulness of the code.

I'm thinking about if there's value in adding a defensive guard and a notice or deprecation message within get_admin_page_title() for when both of the $title and $plugin_page global variables are not correct (see the code from 6.5.5). Maybe something like this:

if ( ! is_string( $plugin_page ) ) {
	wp_trigger_error(
		__FUNCTION__,
		'Requires the $plugin_page global variable to be a non-nullable string data type. '.
		'This global variable is automatically set when the admin URL contains a `page` query parameter.',
		E_USER_DEPRECATED
	);
	$plugin_page = '';
}

$hook = get_plugin_page_hook( $plugin_page, $pagenow );

This kind of defensive guard moves the detection further into the stack call, closer to the root of the problem. As in your case @apedog, it's within the Core function used that triggered the issue.

@apedog would a check have helped you to more quickly identify where the plugin was doing it wrong?

#9 in reply to: ↑ 8 @apedog
2 months ago

I'm sorry, but I need to revise my previous reply.
While I did stumble across this issue when registering a settings page, the issue has nothing to do with registering settings.

The issue can be easily reproduced with this line of code:
add_action( 'admin_init', 'get_admin_page_title' );

So this is an issue in core. Especially since get_admin_page_title() does return the correct string for the page being visited - even on core pages where said globals are null/not set.

ie.
add_action( 'admin_init', fn() => wp_die( get_admin_page_title() ) );
or (less destructively if using Query Monitor):
add_action( 'admin_init', fn() => QM::debug( get_admin_page_title() ) );
will return the string "Available Tools" when visiting the Tools page.

So function get_admin_page_title() works fine on core pages (there is no "doing it wrong" here). However core throws deprecated notices. I don't think this is an upstream issue after all.

Note: See TracTickets for help on using tickets.