Make WordPress Core

Opened 7 years ago

Closed 5 years ago

#40580 closed enhancement (wontfix)

rest_ensure_response() can return WP_Error object, but is consequently not checked in get_item() method

Reported by: ruudjoyo's profile ruud@… Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Hi,

I have used the rest_prepare_{$this->post_type} filter to check if my single post item is allowed to generate output on a particular REST request.
If in my case this isn't allowed, I use this filter to return a WP_Error object

I was assuming this was OK to do, since also the doc-block says:
@return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.

But.. if $response becomes a WP_Error object, the call to $response->link_header fails.

I will attach a patch to check for the possibility that $response is an WP_Error object

Like to hear your thoughts.

Thanks,
Ruud

Attachments (1)

40580.patch (852 bytes) - added by ruud@… 7 years ago.
added is_wp_error check

Download all attachments as: .zip

Change History (8)

@ruud@…
7 years ago

added is_wp_error check

#1 @ruud@…
7 years ago

  • Keywords has-patch dev-feedback added

#2 @jnylen0
7 years ago

  • Keywords needs-unit-tests added

Under what situations does rest_ensure_response return a WP_Error?

This looks like a good thing to fix, one way or another, but this change does need unit tests.

#3 @ruud@…
7 years ago

Hi James,

Thanks for getting back to me.

Under what situations does rest_ensure_response return a WP_Error?

Well, in my case I used the rest_prepare_{$this->post_type} filter to check if this post is allowed to be served via the REST API. I'm checking a custom metakey value for this.

Once this check fails it generates the WP_Error which is then passed into rest_ensure_response() and then rest_unsure_response() just returns the WP_Error as well.

This looks like a good thing to fix, one way or another

Yes, but an early return might also be a (better?) option for this.

but this change does need unit tests.

Agreed. I have limited experience with unit tests, but have made a few for WP before.
You mean like creating a new method, expanding on 'test_get_item' and adding in an add_filter which returns a WP_Error, and then $this->assertInstanceOf( 'WP_Error', $response ); ?

I can see many examples of asserts for the $response not being an WP_Error, however this can be the case as well ($response being a WP_Error object), so am I on the right track here?

Thanks,
Ruud

#4 @ruud@…
7 years ago

Hi James,

I took another stab at this problem and approached it from a different angle this time.
Please have a look at this related ticket #40600

Thanks,
Ruud

#5 @ruud@…
7 years ago

Hi James,

I managed to get the job done via extending the WP_REST_Posts_Controller class and using my own custom class to add the desired additional check in the get_item() method.

Thanks,
Ruud

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

#7 @TimothyBlynJacobs
5 years ago

  • Keywords dev-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Type changed from defect (bug) to enhancement

Hi Rudd,

After discussing this during the REST API Office Hours, I'm going to close out this ticket.

Though rest_ensure_response does have that documentation, the return type should actually be referenced from the prepare_item_for_response method and filter which intentionally only allow for a valid WP_REST_Response object to be returned.

Note: See TracTickets for help on using tickets.