Make WordPress Core

Opened 4 years ago

Closed 19 months ago

Last modified 19 months ago

#49089 closed defect (bug) (fixed)

hook_suffix is undefined in WP_Screen class

Reported by: splendorstudio's profile splendorstudio Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Hi There,

There is an bug in WP_Screen class in 213 line which we are getting via $id = $GLOBALS['hook_suffix']; hook_suffix global, You can see this issue via some functions but,
I encountered with this issue via convert_to_screen() in ajax situation. Please let me know if there is any question.

Change History (17)

#1 @SergeyBiryukov
4 years ago

  • Component changed from General to Administration
  • Description modified (diff)

Related: #22039, #29933.

This ticket was mentioned in PR #1490 on WordPress/wordpress-develop by htdat.


3 years ago
#2

  • Keywords has-patch added

#3 @htdat
3 years ago

When working on plugin Edit Flow, we came across this error https://github.com/Automattic/Edit-Flow/issues/654

Here is what we've got so far:

To replicate this issue with core

Error with track trace:

[12-Jul-2021 04:28:47 UTC] PHP Notice: Undefined index: hook_suffix in /var/www/html/wp-admin/includes/class-wp-screen.php on line 223
[12-Jul-2021 04:28:47 UTC] PHP Stack trace:
[12-Jul-2021 04:28:47 UTC] PHP 1. {main}() /var/www/html/wp-admin/admin-ajax.php:0
[12-Jul-2021 04:28:47 UTC] PHP 2. do_action() /var/www/html/wp-admin/admin-ajax.php:187
[12-Jul-2021 04:28:47 UTC] PHP 3. WP_Hook->do_action() /var/www/html/wp-includes/plugin.php:484
[12-Jul-2021 04:28:47 UTC] PHP 4. WP_Hook->apply_filters() /var/www/html/wp-includes/class-wp-hook.php:316
[12-Jul-2021 04:28:47 UTC] PHP 5. {closure:/var/www/html/wp-content/mu-plugins/trac-49089.php:11-24}() /var/www/html/wp-includes/class-wp-hook.php:292
[12-Jul-2021 04:28:47 UTC] PHP 6. WP_List_Table->construct() /var/www/html/wp-content/mu-plugins/trac-49089.php:14
[12-Jul-2021 04:28:47 UTC] PHP 7. convert_to_screen() /var/www/html/wp-admin/includes/class-wp-list-table.php:149
[12-Jul-2021 04:28:47 UTC] PHP 8. WP_Screen::get() /var/www/html/wp-admin/includes/template.php:2571

Root cause

The error happens around this line http://github.com/WordPress/WordPress/blob/9f91305af2bd18c914096cc5e5cc1d6882163200/wp-admin/includes/class-wp-screen.php#L223-L223

                        $id = $GLOBALS['hook_suffix'];

admin-ajax.php requests do not load file wp-admin/admin.php, which is loaded when admins browse wp-admin pages.
Then global $hook_suffix is not added and defined, hence, the PHP error Undefined index: hook_suffix happens.

A side note here: I searched hook_suffix and $hook_suffix across WordPress core and found out that this global $hook_suffix var is set up in file wp-admin/admin.php:

Solution

I think the ultimate solution here is to change the way we handle $GLOBALS['hook_suffix'] and property id of class WP_List_Table.

That's why I proposed assigning a value for property id with the wp_ajax_ prefix for this case in PR: https://github.com/WordPress/wordpress-develop/pull/1490

#4 @htdat
3 years ago

Another note here (thanks to @mikeyarce): This notice Undefined index will be converted into a warning in PHP8 and that might cause a bigger headache:

Ref: https://www.php.net/manual/en/migration80.incompatible.php:

A number of notices have been converted into warnings:
...
Attempting to read an undefined array key.

#5 @mukesh27
19 months ago

  • Version 5.3.1 deleted

@htdat I face some what similar error in PHP 8.1. Here is more information: https://github.com/WordPress/wordpress-develop/pull/3152#issuecomment-1246552921

This ticket was mentioned in PR #3415 on WordPress/wordpress-develop by dd32.


19 months ago
#6

Avoids a E_NOTICE: Undefined index: hook_suffix in wp-admin/includes/class-wp-screen.php:224 notice.

Trac ticket: https://core.trac.wordpress.org/ticket/49089

#7 @dd32
19 months ago

Just noting that this can also happen when using WP_List_Table through admin-post.php, not just admin-ajax.php.

One solution is for List Tables to pass the screen property upstream to WP_List_Table so it doesn't have to guess what the screen is.

class Example_List_Table extends WP_List_Table {

	public function __construct( $details ) {
		$this->details = $details;

		parent::__construct(
			array(
				'screen' => 'toplevel_page_example' . $this->details['slug']
			)
		);
	}

Another is to simply only use $hook_suffix when it's known and valid. As in PR #3415.

#8 @dd32
19 months ago

#29933 was marked as a duplicate.

#9 @dd32
19 months ago

#40571 was marked as a duplicate.

#10 @dd32
19 months ago

#56052 was marked as a duplicate.

mukeshpanchal27 commented on PR #3415:


19 months ago
#11

@dd32 thanks for the PR. We have similar issue in https://core.trac.wordpress.org/ticket/16502 we end up with this error. https://github.com/WordPress/wordpress-develop/pull/3152#issuecomment-1246552921.

if ( $hook_name ) {
    $id = $hook_name;
} elseif ( wp_doing_ajax() && isset( $_REQUEST['action'] ) ) {
    $id = 'wp_ajax_' . esc_attr( $_REQUEST['action'] );
} elseif ( isset( $GLOBALS['hook_suffix'] ) ) {
    $id = $GLOBALS['hook_suffix'];
} else {
    $id = ''; // Or just let it fail here.
}

#12 @costdev
19 months ago

  • Milestone changed from Awaiting Review to 6.1

As this patch also unblocks a cleaner solution for #16502, I'm milestoning this for 6.1 so that Core Tech leads can take a look.

Pinging @mike, @davidbaumwald, @jeffpaul for your thoughts. The patch for this ticket is relatively minor.

Additional props to @mukesh27 for putting this on my radar.

Version 0, edited 19 months ago by costdev (next)

#13 @davidbaumwald
19 months ago

  • Keywords commit assigned-for-commit added
  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#14 @davidbaumwald
19 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54414:

Administration: Guard against undefined $GLOBALS['hook_suffix'] in WP_Screen::get().

When initially defaulting the screen $id in WP_Screen::get(), if the $hook_name parameter is not supplied, an else fallback uses $GLOBALS['hook_suffix']. However, in some cases, hook_suffix doesn't exist in the global scope. This produces an "Undefined index" notice on < PHP 8, and a warning in >= PHP 8.

This change ensures $GLOBALS['hook_suffix'] has a value before using it as a fallback for the screen ID.

Props splendorstudio, SergeyBiryukov, htdat, mukesh27, dd32, costdev.
Fixes #49089.

#15 @davidbaumwald
19 months ago

  • Keywords assigned-for-commit removed

dream-encode commented on PR #1490:


19 months ago
#17

Thanks for the PR @htdat! This was merged into core for the upcoming version 6.1 in https://core.trac.wordpress.org/changeset/54414.

Note: See TracTickets for help on using tickets.