Support featured media in REST attachment controller #4645

dlh01 · 2023-06-19T19:28:40Z

Trac ticket: https://core.trac.wordpress.org/ticket/41692

spacedmonkey · 2023-06-19T23:23:13Z

src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

+		$post = get_post( $attachment_id );
+
+		if ( ! $post instanceof WP_Post || 'attachment' !== $post->post_type ) {
+			return;
+		}


Use $this->get_post and return WP_Error.

spacedmonkey · 2023-06-19T23:23:50Z

src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

+			_doing_it_wrong(
+				$method,
+				sprintf(
+					/* translators: %s: attachment mime type */
+					__( 'This site does not support post thumbnails on attachments with MIME type %s.' ),
+					$post->post_mime_type
+				),
+				'6.2.0'
+			);


return WP_Error.

Can you explain more? Why return a WP_Error here?

spacedmonkey · 2023-06-19T23:24:49Z

src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

+	 * @param int    $attachment_id Attachment ID.
+	 * @param string $method        Method name.
+	 */
+	private function maybe_did_featured_media_wrong( $attachment_id, $method ) {


What does this method do?

This method is responsible for determining whether a created or updated attachment supports featured images. If an attachment is created or updated via the API with a featured image, but the attachment has a MIME type that doesn't support featured images, this method sends a _doing_it_wrong().

Doing it wrong is not how the REST API handles errors. I don't think this method is a good idea.

spacedmonkey · 2023-06-20T09:45:03Z

src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

+			$this->handle_featured_media( $request['featured_media'], $attachment_id );
+			$this->maybe_did_featured_media_wrong( $attachment_id, __METHOD__ );


Suggested change

$this->handle_featured_media( $request['featured_media'], $attachment_id );

$this->maybe_did_featured_media_wrong( $attachment_id, __METHOD__ );

$update_media = $this->handle_featured_media( $request['featured_media'], $attachment_id );

if ( is_wp_error( $update_media ) ) {

return $update_media;

}

spacedmonkey · 2023-06-20T09:45:17Z

src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

+			$this->handle_featured_media( $request['featured_media'], $data['id'] );
+			$this->maybe_did_featured_media_wrong( $data['id'], __METHOD__ );


Suggested change

$this->handle_featured_media( $request['featured_media'], $data['id'] );

$this->maybe_did_featured_media_wrong( $data['id'], __METHOD__ );

$update_media = $this->handle_featured_media( $request['featured_media'], $attachment_id );

if ( is_wp_error( $update_media ) ) {

return $update_media;

}

spacedmonkey · 2023-06-20T09:55:12Z

src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php

+	 * @param int    $attachment_id Attachment ID.
+	 * @param string $method        Method name.
+	 */
+	private function maybe_did_featured_media_wrong( $attachment_id, $method ) {


Instead of this method, let's extend handle_featured_media. Something like this.

protected function handle_featured_media( $featured_media, $post_id ) { if ( wp_attachment_is( 'audio', $post_id ) && post_type_supports( 'attachment:audio', 'thumbnail' ) || current_theme_supports( 'post-thumbnails', 'attachment:audio' ) ) { return parent::handle_featured_media( $featured_media, $post_id ); } else if ( wp_attachment_is( 'video', $post_id ) && post_type_supports( 'attachment:video', 'thumbnail' ) || current_theme_supports( 'post-thumbnails', 'attachment:video' ) ) { return parent::handle_featured_media( $featured_media, $post_id ); } return new WP_Error( 'rest_feature_meta_update', sprintf( /* translators: %s: attachment mime type */ __( 'This site does not support post thumbnails on attachments with MIME type %s.' ), $post->post_mime_type ), array( 'status' => rest_authorization_required_code() ) ); }

I'm concerned that returning an error in this case will lead to a poor developer experience for a few reasons.

Unless more code is added to roll back the work done by the method up to this point, the attachment will be left in an partially created or partially updated state in the database.

An error will be returned to the client, but, again, the attachment will have been partially created or updated. For a create operation, if the client tries to correct the request and send it again, a second attachment will be created.

The punishment of an error seems to me to not really fit the crime. Is setting the featured image on the wrong MIME type that big a deal?

There is some precedent for a _doing_it_wrong(), for example

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php

Lines 2560 to 2573 in b17064a

if ( array_key_exists( $base, $schema['properties'] ) ) {

$taxonomy_field_name_with_conflict = ! empty( $taxonomy->rest_base ) ? 'rest_base' : 'name';

_doing_it_wrong(

'register_taxonomy',

sprintf(

/* translators: 1: The taxonomy name, 2: The property name, either 'rest_base' or 'name', 3: The conflicting value. */

__( 'The "%1$s" taxonomy "%2$s" property (%3$s) conflicts with an existing property on the REST API Posts Controller. Specify a custom "rest_base" when registering the taxonomy to avoid this error.' ),

$taxonomy->name,

$taxonomy_field_name_with_conflict,

$base

),

'5.4.0'

);

}

.

For these reasons, if _doing_it_wrong() isn't going to work in this case, I think my next move would be to skip the error altogether rather than halt processing.

What do you think?

I don't agree. When update of meta data fails, it returns a WP_Error. Returning errors, allows user of the rest api, maybe not even have access to WP and the error log, to error that the request failed. You have to opt-in to send the featured media id to see this error. If you dont that, you need to know what you are doing.

TimothyBJacobs · 2023-06-23T04:45:50Z

I agree using _doing_it_wrong isn't ideal. It doesn't really fit our preferred model. However, I agree we ideally don't want to leave an item in a partial state. This however is an issue throughout a lot of the REST API already, https://core.trac.wordpress.org/ticket/48822.

Another approach is to do this validation earlier which is what I'd prefer. We can add a validate_callback to the register_rest_route that does this validation. Then an error will be returned before any processing has happened.

dlh01 · 2023-06-23T06:16:38Z

Another approach is to do this validation earlier which is what I'd prefer. We can add a validate_callback to the register_rest_route that does this validation. Then an error will be returned before any processing has happened.

Is there prior art for determining the MIME type-to-be of a new attachment that can be checked for support?

spacedmonkey · 2023-06-23T09:14:18Z

Another approach is to do this validation earlier which is what I'd prefer. We can add a validate_callback to the register_rest_route that does this validation. Then an error will be returned before any processing has happened.

Is there prior art for determining the MIME type-to-be of a new attachment that can be checked for support?

That is a good point. We can easily check / know the mime type of the attachment on create endpoint. So we can't use validate_callback sadly.

spacedmonkey · 2023-06-23T12:59:56Z

@TimothyBJacobs @dlh01 As a talking point, I put together a PR for this #4682

TimothyBJacobs · 2023-06-23T15:19:09Z

We have the full request object, so we should be able to detect the mime type. I think we could use wp_check_filetype_and_ext on the temp file?

dlh01 · 2023-06-26T05:14:05Z

We have the full request object, so we should be able to detect the mime type. I think we could use wp_check_filetype_and_ext on the temp file?

OK, that's a path I can look into, but probably not in time for 6.3 at this point.

I'll also say up-front that I don't have the experience with media component to be able to think in detail about any security risks that might come with attempting to analyze a file before it's been accepted into the media library, so any guidance about that (now or when the PR is updated) would be appreciated.

swissspidy · 2024-02-12T22:16:30Z

Committed in https://core.trac.wordpress.org/changeset/57603

dlh01 added 3 commits June 19, 2023 15:18

Support featured media in REST attachment controller

beb0920

Fix double-semi

6a3643d

Update a few more tests

591b960

spacedmonkey reviewed Jun 19, 2023

View reviewed changes

Use internal get_post()

9a68ab6

spacedmonkey reviewed Jun 20, 2023

View reviewed changes

swissspidy mentioned this pull request Jan 30, 2024

REST API: add featured_media support for video/audio #5978

Closed

swissspidy closed this Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support featured media in REST attachment controller #4645

Support featured media in REST attachment controller #4645

dlh01 commented Jun 19, 2023

spacedmonkey Jun 19, 2023

spacedmonkey Jun 19, 2023

dlh01 Jun 20, 2023

spacedmonkey Jun 19, 2023

dlh01 Jun 20, 2023

spacedmonkey Jun 20, 2023

spacedmonkey Jun 20, 2023

spacedmonkey Jun 20, 2023

spacedmonkey Jun 20, 2023 •

edited

Loading

dlh01 Jun 20, 2023

spacedmonkey Jun 20, 2023

TimothyBJacobs commented Jun 23, 2023

dlh01 commented Jun 23, 2023

spacedmonkey commented Jun 23, 2023

spacedmonkey commented Jun 23, 2023

TimothyBJacobs commented Jun 23, 2023

dlh01 commented Jun 26, 2023

swissspidy commented Feb 12, 2024

		$this->handle_featured_media( $request['featured_media'], $attachment_id );
		$this->maybe_did_featured_media_wrong( $attachment_id, __METHOD__ );

-			$this->handle_featured_media( $request['featured_media'], $attachment_id );
-			$this->maybe_did_featured_media_wrong( $attachment_id, __METHOD__ );
+						$update_media = $this->handle_featured_media( $request['featured_media'], $attachment_id );
+			if ( is_wp_error( $update_media ) ) {
+			      return $update_media;
+		        }

	if ( array_key_exists( $base, $schema['properties'] ) ) {
	$taxonomy_field_name_with_conflict = ! empty( $taxonomy->rest_base ) ? 'rest_base' : 'name';
	_doing_it_wrong(
	'register_taxonomy',
	sprintf(
	/* translators: 1: The taxonomy name, 2: The property name, either 'rest_base' or 'name', 3: The conflicting value. */
	__( 'The "%1$s" taxonomy "%2$s" property (%3$s) conflicts with an existing property on the REST API Posts Controller. Specify a custom "rest_base" when registering the taxonomy to avoid this error.' ),
	$taxonomy->name,
	$taxonomy_field_name_with_conflict,
	$base
	),
	'5.4.0'
	);
	}

Support featured media in REST attachment controller #4645

Support featured media in REST attachment controller #4645

Conversation

dlh01 commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacedmonkey Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyBJacobs commented Jun 23, 2023

dlh01 commented Jun 23, 2023

spacedmonkey commented Jun 23, 2023

spacedmonkey commented Jun 23, 2023

TimothyBJacobs commented Jun 23, 2023

dlh01 commented Jun 26, 2023

swissspidy commented Feb 12, 2024

spacedmonkey Jun 20, 2023 •

edited

Loading