WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 23 months ago

#45269 closed defect (bug) (fixed)

REST API: `rest_insert_attachment` and `rest_after_insert_attachment` actions fire twice on update.

Reported by: peterwilsoncc Owned by: danielbachhuber
Milestone: 5.0 Priority: normal
Severity: normal Version: 5.0
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

As \WP_REST_Attachments_Controller::update_item() calls \WP_REST_Posts_Controller::update_item(), this causes the rest_insert_attachment and rest_after_insert_attachment hooks to be fired twice on update requests, once in each controller class.

Creating as item isn't affected as the attachment controller does not call the parent class.

rest_after_insert_attachment is new for version 5.0
rest_insert_attachment is from a previous version of WP

Attachments (2)

45269.diff (3.9 KB) - added by peterwilsoncc 2 years ago.
45269.2.diff (5.7 KB) - added by danielbachhuber 2 years ago.

Download all attachments as: .zip

Change History (12)

@peterwilsoncc
2 years ago

#1 @danielbachhuber
2 years ago

  • Focuses rest-api added
  • Milestone changed from Awaiting Review to 5.0

#2 @danielbachhuber
2 years ago

In 45269.2.diff:

  • Adds a second test_rest_insert_attachment_hooks_fire_once_on_create test.
  • Avoids changing the execution order of $response = $this->prepare_item_for_response( $post, $request ); by duplicating the call inside if ( 'attachment' === $this->post_type ) {.
  • Includes a comment mentioning where the rest_after_insert_ action is fired.

#3 @danielbachhuber
2 years ago

  • Owner set to danielbachhuber
  • Resolution set to fixed
  • Status changed from new to closed

In 43862:

REST API: Prevent duplicate firing of rest(_after)?_insert_attachment actions.

Props danielbachhuber, peterwilsoncc.
Fixes #45269.
See #42864.

#4 @danielbachhuber
2 years ago

Thanks for catching this, @peterwilsoncc. Leaving the ticket closed; this can be backported with #42864.

#5 @peterwilsoncc
2 years ago

I had some late shower thoughts on this, this-morning. Is it worth allowing the if statement to be filterable to allow for custom endpoints making use of a similar pattern to the attachments?

<?php
$subclass_endpoints = apply_filters( 'rest_api_subclass_endpoints', array( 'attachment' ) );

if ( in_array( $this->post_type, $subclass_endpoints, true ) ) {
    // etc

#6 @danielbachhuber
2 years ago

@peterwilsoncc Hm, good question. It exposes the problem that if you're providing an endpoint implementation that subclasses one of the core controllers, then we run into the execution order problem again.

With this being said, I'm not sure how much subclassing is the formally supported extensibility pattern. I think it'd be much more common for developers to either use register_rest_field(), or write completely custom controllers.

At this point, I don't have a strong feeling on the appropriate path forward. I'd love to know more about how serious of a real-world problem this is, vs. simply a theoretical concern.

More specifically, in the Jetpack and Yoast examples outlined in https://core.trac.wordpress.org/ticket/45114#comment:7, it would be really valuable to know:

  1. What is the specific bug that the issue causes.
  2. What's the level of effort and complexity to implement an alternative workaround.

With this information, we can better assess the severity of the issue.

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


2 years ago

#8 @peterwilsoncc
2 years ago

At this point, I don't have a strong feeling on the appropriate path forward. I'd love to know more about how serious of a real-world problem this is, vs. simply a theoretical concern.

Cool, I didn't think it was a likely problem but wanted to ask in case someone else knew better.

#9 @jeremyfelt
23 months ago

In 44225:

REST API: Prevent duplicate firing of rest(_after)?_insert_attachment actions.

Merges [43862] from the 5.0 branch to trunk.

Props danielbachhuber, peterwilsoncc.
Fixes #45269.
See #42864.

#10 @jeremyfelt
23 months ago

In 44234:

Tests: Replace uses of $this->server with rest_get_server().

In [42724], $this->server was replaced with rest_get_server() for better memory recycling.

[43862], from the 5.0 branch, was merged into trunk in [44225] and used the now unavailable $this->server.

This updates the new tests from the 5.0 branch to use the expected rest_get_server().

See #45269, #41641.

Note: See TracTickets for help on using tickets.