Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#52640 new defect (bug)

The rest_prepare_attachment filter runs twice during REST request

Reported by: chrisl27's profile chrisl27 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.7
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Hi,

For each /wp/v2/media call the rest_prepare_attachment filter is being run twice. Once by WP_REST_Posts_Controller and then a second time by WP_REST_Attachments_Controller.

This is due to WP_REST_Attachments_Controller::prepare_item_for_response calling its class parent method, which calls apply_filters on "rest_prepare_{$this->post_type}"

This may cause issues with code that expects the attachment to only be prepared once, particularly where the response payload is different between the two filter callbacks (eg doesn't include media sizes the first time).

Attachments (1)

52640-1.patch (1.6 KB) - added by chrisl27 3 years ago.
Patch for this issue, with test

Download all attachments as: .zip

Change History (6)

@chrisl27
3 years ago

Patch for this issue, with test

#1 @chrisl27
3 years ago

  • Keywords has-patch has-unit-tests added

Patch has test that will fail without the code change (indicating the filter has run twice)

#2 @chrisl27
3 years ago

This patch is a simple solution to the attachment problem that doesn't help if you otherwise subclassed WP_REST_Posts_Controller for your own post type and wanted to include a filter. I think that's unlikely to happen outside of core though.

Version 0, edited 3 years ago by chrisl27 (next)

#3 follow-up: @TimothyBlynJacobs
3 years ago

  • Milestone changed from Awaiting Review to Future Release

Thanks for the ticket @chrisl27!

In core this is just the attachments controller, so we could do the hardcoded check for the attachment post type, but this is a tricky issue since it'd apply whenever a plugin tries to override prepare_item_for_response.

I wonder if instead we should refactor the attachments controller to use the filter instead of overriding the method, and encourage plugins to use the filter.

#4 in reply to: ↑ 3 @chrisl27
3 years ago

Replying to TimothyBlynJacobs:

Thanks for the ticket @chrisl27!

In core this is just the attachments controller, so we could do the hardcoded check for the attachment post type, but this is a tricky issue since it'd apply whenever a plugin tries to override prepare_item_for_response.

I wonder if instead we should refactor the attachments controller to use the filter instead of overriding the method, and encourage plugins to use the filter.

Yup, agreed it's not ideal as it makes the problem more obscure for non-attachments.

I don't think there is anywhere else in the API code where the code uses a filter the API itself created so that would be unusual too.

How about changing the method signature of prepare_item_for_response to something like

public function prepare_item_for_response( $post, $request, $suppress_filters = false );

and call parent::prepare_item_for_response($post, $request, true); from the attachments subclass.

I think this could work because the attachments controller can actually look at the same flag to see if *it* should suppress_filters on the response as well, enabling a further subclass of the attachments controller to suppress filters for both its parents.

It also has the advantage that the function signature implies there's something going on with filters that would be clearer to plugin or core authors. A boolean parameter may not be ideal for future changes, perhaps a wp_parse_args array param as the third argument if we go this way?

#5 @chrisl27
3 years ago

Although that would be a breaking change for any existing subclasses that override that method. If that's a concern then maybe a new protected variable could control this.

Note: See TracTickets for help on using tickets.