Make WordPress Core

Opened 18 months ago

Closed 13 months ago

Last modified 13 months ago

#56566 closed defect (bug) (fixed)

Fatal error when WP_REST_Server::dispatch() returns WP_Error

Reported by: davefx's profile DaveFX Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 6.2 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

We are getting a Fatal error in our site when the WP_REST_Server::embed_links() method calls the WP_REST_Server::dispatch() method, and this latter one returns a WP_Error object.

After this, the method calls the filter rest_post_dispatch, and the two hooked functions (rest_send_allow_header and rest_filter_response_fields) will generate a fatal error if the filtered value is a WP_Error instead of a WP_Rest_Response.

Change History (21)

This ticket was mentioned in PR #3242 on WordPress/wordpress-develop by davefx.


18 months ago
#1

  • Keywords has-patch added

Trac ticket: #56566

#2 @DaveFX
18 months ago

  • Keywords has-patch removed

Created PR 3242 that replicates the transformation from WP_Error to WP_REST_Response already done in the serve_request() method

Last edited 18 months ago by DaveFX (previous) (diff)

#3 @SergeyBiryukov
18 months ago

  • Milestone changed from Awaiting Review to 6.1

#4 @TimothyBlynJacobs
18 months ago

  • Owner set to TimothyBlynJacobs
  • Status changed from new to accepted

#5 @TimothyBlynJacobs
18 months ago

  • Keywords needs-unit-tests added
  • Severity changed from major to normal
  • Version changed from 6.0.2 to 4.4

Thanks for the ticket @DaveFX!

There's a typo in the patch, it's using $result instead of $response.

Would you be interested in working on unit tests for this?

#6 @DaveFX
18 months ago

Thanks for detecting the typo!

Sorry, I'm not familiarized with unit tests for WordPress, so it would be better if I don't work on that.

Last edited 18 months ago by DaveFX (previous) (diff)

This ticket was mentioned in PR #3270 on WordPress/wordpress-develop by felipeelia.


18 months ago
#7

  • Keywords has-patch has-unit-tests added; needs-unit-tests removed

This PR is a version of #3242 with PHPUnit tests. Original PR description:

Replicating treatment of potential WP_Error that is done in the serve_request() method to avoid fatal errors while processing WP_Error as if it were a WP_REST_Response.

Props to @davefx for the fix.

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

#8 @felipeelia
18 months ago

@TimothyBlynJacobs I've created the PR above adding PHPUnit tests for the fix created by @DaveFX (thanks, Dave!)

The PR also fixes the test_link_embedding_without_links test, which was effectively testing nothing. As embed_links is a protected function, I'm testing it through response_to_data, as we are doing in some other places.

Let me know if any adjustment is needed. Thanks!

TimothyBJacobs commented on PR #3270:


18 months ago
#9

Thanks for the tests @felipeelia! We can test embed_links directly as we were doing previously. In the context of unit tests, an instance of Spy_REST_Server is used instead. This utilizes the __call magic method to let us call any protected/private methods directly.

felipeelia commented on PR #3270:


18 months ago
#10

Thanks for the info @TimothyBJacobs about calling methods directly, that is super helpful. I've (partially) reverted the change I made in test_link_embedding_without_links but kept the change where it is now comparing things against $result rather than $data. Let me know if that makes sense or not, please.

For test_link_embedding_returning_wp_error I'm keeping the response_to_data() usage, so we can take advantage of add_link(). Let me know if that is okay.

#11 follow-up: @TimothyBlynJacobs
18 months ago

Sorry ya'll. Taking another look at the patch, I'm not sure it's the correct approach actually. The WP_REST_Server::dispatch method is typed as returning a WP_REST_Response instance. I think returning a WP_Error instance here would be incorrect. WP_REST_Server::respond_to_request ensures a response object is returned as well. This error would only happen if a plugin is returning a WP_Error instance from rest_pre_dispatch.

So instead, I think we should move the error_to_response call before $result is returned at the top of WP_REST_Server::dispatch. Otherwise we are widening our return types from the method which would be incorrect.

#12 @spacedmonkey
17 months ago

@TimothyBlynJacobs Any movement on this ticket? Otherwise, I believe we are getting close to the stage where we need to punt this.

#13 @spacedmonkey
17 months ago

  • Milestone changed from 6.1 to 6.2

Punting to 6.2

#14 in reply to: ↑ 11 @DaveFX
16 months ago

Hi! I'd like to make the requested changes, but it seems I don't have access to edit the current PR that includes the changes in the tests.

@felipeelia could you grant me access to your branch, please?

#15 @felipeelia
16 months ago

Sure @DaveFX, I've added you to my fork now. Let me know if that works or not. Thanks!

#16 @DaveFX
16 months ago

Thanks, @felipeelia.

I confirm I've made the requested modifications, moving the changes inside the WP_REST_Server::dispatch() method, that now process the results from the rest_pre_dispatch filter.

@TimothyBlynJacobs Please confirm if this would be OK now.

davefx commented on PR #3242:


14 months ago
#17

Closing PR, as it's already included in PR #3270.

davefx commented on PR #3270:


14 months ago
#18

This PR should be ready to be merged, after the last changes.

This ticket was mentioned in Slack in #core by costdev. View the logs.


13 months ago

#20 @TimothyBlynJacobs
13 months ago

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

In 55361:

REST API: Normalize WP_REST_Server::dispatch() to return a response object.

Previously, the rest_pre_dispatch filter could be used to return a WP_Error instance. This would cause a fatal error for rest_post_dispath
filters that were rightly expecting a WP_REST_Response object to be passed instead.

Props DaveFX, felipeelia.
Fixes #56566.

Note: See TracTickets for help on using tickets.