REST API does not return featured_media for Audio/Video

[wonderboymusic]

Audio and Video attachments can have a featured image. This data is not currently returned by the REST API wp/v2/media endpoint. Part of the problem is that the schema for attachments is generated by the Post controller, even though the Post controller does not return data for attachments. Because the schema is used by the OPTIONS endpoint, before knowing what mime-type the attachment is, thumbnail should be set for attachment's attributes so that featured_media is added to the responses from wp/v2/media.

[rmccue]

Makes sense to me.

On the patch, is there a reason to explicitly set featured_media? It looks to me like setting in the schema should be enough to add it to the response.

[TimothyBlynJacobs]

Taking a look at this after discussion in the REST API bug scrub Friday.

Core doesn't register the attachment post type with support for thumbnail. So just including thumbnail in the fixed schema is causes issues with the ::check_post_data() unit test method.

The first patch lists featured_media in the schema as read-write, but it only supports read. This is because the attachments controller doesn't call parent::create_item() which has the support for setting the featured media.

So that leaves us with three options I think.

1. Add thumbnail support for the attachments post type and manually update the thumbnail in the attachments controller. I've attached the patch that does this. I have no idea what the ramifications are for listing thumbnail as supported in the post type, though.
2. Same as #1, except don't include thumbnail support in the attachment post type and update ::check_post_data to either use the schema instead of post_type_supports() or just make an exception for the attachment post type.
3. Continue with the first patch, but mark the featured media schema as readonly.

[kadamwhite]

Punting to future release, per discussion with @TimothyBlynJacobs in this week's REST API team chat

[desrosj]

Punting as this falls outside the scope for 5.0.

[desrosj]

41692.2.diff​ is no longer applying to trunk

[desrosj]

Per today's bug scrub, @TimothyBlynJacobs will refresh the patch. I also dropped this in ​#core-media for some additional feedback.

[pento]

Patch needs refreshing, punting.

[dlh]

​Per discussion in Slack, I'm going to try to refresh this patch for 6.3.

[spacedmonkey]

@dlh Did you get anytime to work on this? If not, then we can punt this to 6.4.

[dlh]

@spacedmonkey Yes, I've got a refreshed branch that's nearly ready. It needs some test updates, and then I should have a PR submitted in the next couple of days.

[dlh]

I've opened a Pull Request for this ticket with an approach that tries to handle the complications raised in the previous discussions.

First, with respect to post type support: Core registers thumbnail post type support for the "pseudo post types" of attachment:audio and attachment:video. See [27209]. These pseudo-types are checked in a few places in core already. I decided to not try to change anything about that and left out adding thumbnail support to the attachment type.

Second, as mentioned in the ticket description, the post type schema is generated for OPTIONS requests without reference to a specific attachment. So, thumbnail is added to the fixed schema attributes as was also done in 41692.2.diff​.

Third, what to do about a case where someone tries to add a featured image to a MIME type that isn't audio or video?

I considered having the API reject the submission with an error, but that punishment seemed disproportionate to the violation. It would also be technically annoying, since the attachment would need to be created in order to get the MIME type, then immediately deleted if it was rejected.

I also considered conditionally adding thumbnail to the attachment schema based on whether post thumbnail support existed on the audio or video media types. But I think that would imply that featured media could be added to only those types, when in actuality the API wouldn't reject featured media added to other types for the reason just given.

So, I landed on what you see in the PR: Featured media is always added to the attachment schema and always handled, but a _doing_it_wrong() will be issued if the attachment MIME type doesn't support thumbnails.

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.

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?

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.

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

@TimothyBJacobs @dlh01 As a talking point, I put together a PR for this ​https://github.com/WordPress/wordpress-develop/pull/4682

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?

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.

[oglekler]

Slim chances that this will be ready for 6.3, so I am moving this to 6.4. Please pay attention to failing unit tests due to the addition of featured_media, they need to be changed as well. The issue in question looks more like an enhancement than a bug fix, so I am changing type as well.

[oglekler]

@dlh and @spacedmonkey can you proceed with this? This looks like a straight forward task, the Unit tests need to be changed as well.

[spacedmonkey]

I am going to leave this with @dlh to continue to work on. My pr was just what I was thinking.

[dlh]

Unfortunately, I don't anticipate having the capacity to work on this ticket during the 6.4 cycle, so I'm going to remove myself as the ticket owner for now. If it's still open in the 6.5 cycle, I'll do my best to pick it back up.

[oglekler]

Because we are getting close to Beta 1, I am moving this ticket into 6.5.