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

[peterwilsoncc]

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

[danielbachhuber]

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.

[danielbachhuber]

In 43862:

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

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

[danielbachhuber]

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

[peterwilsoncc]

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

[danielbachhuber]

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

[peterwilsoncc]

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.

[jeremyfelt]

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.

[jeremyfelt]

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.