Fatal error when WP_REST_Server::dispatch() returns WP_Error

[DaveFX]

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.

Trac ticket: #56566

[DaveFX]

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

[TimothyBlynJacobs]

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?

[DaveFX]

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.

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

[felipeelia]

@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!

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.

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.

[TimothyBlynJacobs]

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.

[spacedmonkey]

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

[spacedmonkey]

Punting to 6.2

[DaveFX]

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?

[felipeelia]

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

[DaveFX]

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.

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

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

[TimothyBlynJacobs]

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.

Merged in ​https://github.com/WordPress/wordpress-develop/commit/a6d2904cb1005ae74faa0a89968760856b32e06c.