Make WordPress Core

Opened 22 months ago

Closed 19 months ago

Last modified 19 months ago

#55705 closed enhancement (fixed)

Safeguarding has_blocks() Against Fatal Errors

Reported by: howdy_mcgee's profile Howdy_McGee Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.0
Component: Editor Keywords: has-patch has-unit-tests has-testing-info commit
Focuses: Cc:

Description

The has_blocks() has type conversion at the end function call:

return false !== strpos( (string) $post, '<!-- wp:' );

Where $post is the passed parameter. While this should be a WP_Post, or type that returns a WP_Post, the function should not throw a Fatal Error if $post is an object by the end of the call.

I think this should be revised to soft-error out if whatever parameter is passed cannot be converted to a string. For example, if a WP_Error is given, the system Fatal Errors. If the function's get_post() call returns false, the function continues to try to string convert the passed parameter and could throw a Fatal Error.

Attachments (4)

55705.diff (1.3 KB) - added by costdev 22 months ago.
Adds a _doing_it_wrong() and returns false when get_post() returns null. Includes PHPUnit test.
has_blocks.zip (839 bytes) - added by costdev 22 months ago.
A plugin for reproducing the issue and testing the patch. Requires PHP 7+. See instructions below.
Capture d’écran 2022-08-04 à 00.29.36.png (219.0 KB) - added by audrasjb 19 months ago.
Before patch, using comment 2 instructions - fatal error
Capture d’écran 2022-08-04 à 00.32.39.png (319.2 KB) - added by audrasjb 19 months ago.
After patch, using comment 2 instructions - _doing_it_wrong notice 👌

Download all attachments as: .zip

Change History (19)

#1 @costdev
22 months ago

  • Component changed from General to Editor
  • Milestone changed from Awaiting Review to 6.1
  • Version set to 5.0

@costdev
22 months ago

Adds a _doing_it_wrong() and returns false when get_post() returns null. Includes PHPUnit test.

@costdev
22 months ago

A plugin for reproducing the issue and testing the patch. Requires PHP 7+. See instructions below.

#2 @costdev
22 months ago

  • Keywords has-patch has-unit-tests needs-testing has-testing-info added

Reproduction/Testing steps

  1. Download has_blocks.zip.
  2. Navigate to Plugins > Add New > Upload Plugin.
  3. Select has_blocks.zip, upload and activate.
  4. 🐞 There should be an admin notice:
    A Fatal Error occurred: Object of class stdClass could not be converted to string.
    
  5. Apply patch 55705.diff.
  6. Refresh the page.
  7. ✅ There should be an admin notice and a _doing_it_wrong() notice:
    Admin notice:
    A Fatal Error did not occur.
    
    _doing_it_wrong() notice:
    Notice: Function has_blocks was called incorrectly.
    $post is not a valid post.
    Please see Debugging in WordPress for more information.
    (This message was added in version 6.1.0.) in /wp-includes/functions.php on line <number>
    

Clean up

  • Deactivate and delete the "has_blocks() Fatal Error" plugin.
  • Revert the patch.

#3 @colonelphantom
22 months ago

  • Keywords needs-testing removed

The attached patch works as expected. I tested it using the steps from the previous comment and it behaves like described.

#4 @audrasjb
19 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Many thanks for the great testing instruction!
Self assigning for review.

@audrasjb
19 months ago

Before patch, using comment 2 instructions - fatal error

@audrasjb
19 months ago

After patch, using comment 2 instructions - _doing_it_wrong notice 👌

#6 @audrasjb
19 months ago

I put together a PR to refresh the previous patch against trunk and to address a small WPCS issue.

#7 @audrasjb
19 months ago

  • Keywords changes-requested added

@costdev some other unit tests are failing using your patch: Unexpected incorrect usage notice for has_blocks.. See the above PR for more details.

Looks like it will need a few more work :)

#8 @costdev
19 months ago

Thanks @audrasjb - I'll look into this!

#10 @costdev
19 months ago

@audrasjb PR 3059 is ready for review.

#11 @costdev
19 months ago

I have updated PR 3059 to remove the _doing_it_wrong() as this is inconsistent with Core's usual approach: If the post can't be retrieved, return false (or similar). The revised PR is ready for review.

Big props to @dlh and @peterwilsoncc for advising on this!

audrasjb commented on PR #3059:


19 months ago
#12

I tested the PR and it looks good to me 👍

#13 @audrasjb
19 months ago

  • Keywords commit added; changes-requested removed

#14 @audrasjb
19 months ago

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

In 53858:

Editor: Safeguard has_blocks() against fatal errors.

This changeset ensures has_blocks() doesn't return a fatal error when $post is not a valid post. If the post can't be retrieved, the function now returns false.

Props Howdy_McGee, costdev, colonelphantom, audrasjb, dlh, peterwilsoncc.
Fixes #55705.

Note: See TracTickets for help on using tickets.