Make WordPress Core

Opened 7 years ago

Closed 2 months ago

Last modified 2 months ago

#41692 closed enhancement (fixed)

REST API does not return featured_media for Audio/Video

Reported by: wonderboymusic's profile wonderboymusic Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests add-to-field-guide
Focuses: Cc:

Description

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.

Attachments (2)

rest-thumbnail.diff (1.4 KB) - added by wonderboymusic 7 years ago.
41692.2.diff (6.9 KB) - added by TimothyBlynJacobs 7 years ago.

Download all attachments as: .zip

Change History (45)

#1 @rmccue
7 years ago

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.

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


7 years ago

#3 @TimothyBlynJacobs
7 years ago

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.

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


7 years ago

#5 @kadamwhite
7 years ago

  • Milestone changed from 4.9 to Future Release

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

#6 @wonderboymusic
6 years ago

  • Milestone changed from Future Release to 5.0

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


6 years ago

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


6 years ago

#9 @desrosj
6 years ago

  • Milestone changed from 5.0 to 5.1

Punting as this falls outside the scope for 5.0.

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


5 years ago

#11 @desrosj
5 years ago

  • Keywords needs-refresh added

41692.2.diff is no longer applying to trunk

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


5 years ago

#13 @desrosj
5 years ago

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

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


5 years ago

#15 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

Patch needs refreshing, punting.

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


14 months ago

#17 @dlh
14 months ago

  • Milestone changed from Future Release to 6.3
  • Owner set to dlh
  • Status changed from new to assigned

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

#18 @spacedmonkey
10 months ago

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

#19 @dlh
10 months ago

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

This ticket was mentioned in PR #4645 on WordPress/wordpress-develop by dlh01.


10 months ago
#20

  • Keywords has-unit-tests added; needs-refresh removed

#21 @dlh
10 months ago

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.

@TimothyBlynJacobs commented on PR #4645:


10 months ago
#22

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 commented on PR #4645:


10 months ago
#23

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 commented on PR #4645:


10 months ago
#24

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 commented on PR #4645:


10 months ago
#26

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

@TimothyBlynJacobs commented on PR #4645:


10 months ago
#27

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 commented on PR #4645:


10 months ago
#28

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.

#29 @oglekler
10 months ago

  • Milestone changed from 6.3 to 6.4
  • Type changed from defect (bug) to enhancement

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.

#30 @oglekler
8 months ago

  • Keywords changes-requested added

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

#31 @spacedmonkey
8 months ago

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

#32 @dlh
7 months ago

  • Owner dlh deleted

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.

#33 @oglekler
7 months ago

  • Milestone changed from 6.4 to 6.5

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#35 @audrasjb
3 months ago

  • Milestone changed from 6.5 to Future Release

From today's bug scrub:
As this is an enhancement and since we’re still waiting for a patch, let’s move it to Future release milestone.
Happy to review and move it back to milestone 6.5 if a complete patch is proposed during the alpha cycle.

This ticket was mentioned in PR #5978 on WordPress/wordpress-develop by @swissspidy.


3 months ago
#36

Audio and Video attachments can have a featured image. This data is not currently returned by the wp/v2/media endpoint. This PR fixes that.

Supersedes #4645 and #4682 with a refreshed patch that incorporates all the feedback from the previous patches.

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

#37 @swissspidy
2 months ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from assigned to closed

In 57603:

REST API: Add featured_media field to attachments endpoint.

Audio and video attachments can have a featured image, also known as a poster image. This functionality is now properly exposed by the wp/v2/media endpoint.

Props swissspidy, timothyblynjacobs, wonderboymusic, dlh, spacedmonkey.
Fixes #41692.

#41 @spacedmonkey
2 months ago

  • Keywords needs-dev-note added; changes-requested removed

#42 @spacedmonkey
2 months ago

  • Milestone changed from Future Release to 6.5

#43 @swissspidy
2 months ago

  • Keywords add-to-field-guide added; needs-dev-note removed
Note: See TracTickets for help on using tickets.