Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bail out if get_current_screen() returns null. #33261

Merged

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Jul 7, 2021

Description

In function gutenberg_widgets_customize_load_block_editor_scripts_and_styles() on < WordPress 5.8-RC2, a PHP notice is thrown when get_current_screen() returns null.

PHP Warning:  Attempt to read property "base" on null in /.../wp-content/plugins/gutenberg/lib/widgets-customize.php on line 162

This happens because the instance is used to get the base property as part of the conditional check.

This PR breaks up the conditional checks into guard clauses and then checks if the value returned from get_current_screen(0 is null. If yes, it bails out. If no, then the base property is checked.

How has this been tested?

Local machine => Notice in debug log before fix. No notice after fix.

Environment:

  • OS: macOS Big Sur
  • WP: 5.7.2
  • Browser: Chrome and Firefox

Note: The PHP notice does not happen with WordPress 5.8-RC2.

Screenshots

Screenshot 2021-07-07 at 13 17 15

Types of changes

  • Breaks up the conditionals and converts each into a guard clause
  • Checks for null and bails out if returned

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2021-07-07 at 14 21 54

Tested with Plugin build of this PR and found no errors in Widgets 🥳

@getdave getdave added this to the Gutenberg 11.0 milestone Jul 7, 2021
@getdave
Copy link
Contributor

getdave commented Jul 7, 2021

Does this need backport to WP 5.8?

@aristath
Copy link
Member

aristath commented Jul 7, 2021

Does this need backport to WP 5.8?

It would be good to backport it, I ran into this issue when trying to patch https://core.trac.wordpress.org/ticket/53429 and had to resort to a somewhat ugly hack (WordPress/wordpress-develop@069d32f)
If this gets backported we'll be able to properly fix the other issues as well 👍

@hellofromtonya
Copy link
Contributor Author

Does this need backport to WP 5.8?

This particular callback function is not in WP 5.8. The PHP notice is not thrown in 5.8-RC2. That said, would be worth opening a new Trac ticket to investigate the problem you saw @aristath as there may be a hole in the merged implementation that needs a check like this.

@getdave getdave merged commit 891d448 into WordPress:trunk Jul 7, 2021
@hellofromtonya hellofromtonya deleted the fix/null-get_current_screen branch July 7, 2021 13:53
}

$current_screen = get_current_screen();
if ( is_null( $current_screen ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not check for the instanceof WP_Screen?

  • As this is a plugin, an additional check might be required to ensure the class WP_Screen exists.
  • get_current_screen returns null on failure or an instance of WP_Screen on success.

The check here is for the failure state. Using this approach keeps the check positive and simple, while avoiding an additional class_exists check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, you can safely do an instanceof check with a class that does not exist., https://3v4l.org/X4n64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants