REST API: `password` is incorrectly included in arguments to get a media item

[jnylen0]

Media items in the API do not support passwords, so we need to check for the presence of password in the item schema before adding this argument.

[rachelbaker]

@jnylen0 What if the media is attached to a post that is password protected? What should happen then?

[jnylen0]

I think this change is still correct.

Here's such a post: ​https://nylen.io/wp-dev/2016/11/30/this-includes-attachment-tim-berners-lee-favorite-websites/

and here's the attachment page (does not require or accept a password): ​https://nylen.io/wp-dev/2016/11/30/this-includes-attachment-tim-berners-lee-favorite-websites/tim-berners-lee-favorite-websites/

I'm wondering whether it is worth calling all of get_item_schema again, maybe we should special-case 'attachment' === $this->post_type there instead.

[rachelbaker]

I am concerned about the potential performance impact of reloading the schema. We *could* special case attachment, but then shouldn't we also special case the nav_menu_item post type when adding Menu endpoints in the future?

Seems like, ideally, we would want to use something like a post_type_supports() conditional within the schema. Which is a bigger fix than we can get into 4.7, which brings me to question "is anything broken here?"

I agree that this is something we should fix, but I also don't want to rush a half-baked solution.

@jnylen0 @helen I recommend moving this ticket out of the 4.7 milestone, and into 4.8 so it can be addressed thoroughly.

[jnylen0]

Replying to rachelbaker:

I am concerned about the potential performance impact of reloading the schema.

Worth noting that we're already grabbing the schema twice for each post type during route registration (via get_endpoint_args_for_item_schema for create and edit).

We *could* special case attachment, but then shouldn't we also special case the nav_menu_item post type when adding Menu endpoints in the future?

Agreed I don't much like the idea of special casing here either.

Seems like, ideally, we would want to use something like a post_type_supports() conditional within the schema. Which is a bigger fix than we can get into 4.7

There's no post_type_supports for passwords, it's odd like that.

which brings me to question "is anything broken here?"

I think so, we have an argument that does nothing for no clear reason. If we leave it in place for 4.7, are we saying that it should be left in place for future releases also?

@jnylen0 @helen I recommend moving this ticket out of the 4.7 milestone, and into 4.8 so it can be addressed thoroughly.

I would prefer that we go with the fix in 38977.diff​ for 4.7 and get the interface behaving as expected, then we can circle back around to a more complete and performant fix in 4.8.

In any case, I would not like to move this out of 4.7 unless we can go ahead and decide that we will definitely fix it in 4.8.

[pento]

The worst case is that someone sends a password that won't do anything. We can do this in 4.7.1.

[jnylen0]

Agree with getting this into 4.7.1. While it's not the ideal fix, performance impact from calling the schema 3 times instead of 2 is going to be neglibible - it doesn't call out to the DB or anything and we recommend that people upgrade to PHP 7 which will also help.

[pento]

Let's go with 38977.diff​.

I'm not worried about the performance impact, and while I would like to see post_type_supports() support password, that'll be a fairly significant amount of work, way outside of the scope of this ticket.

[jnylen0]

In 39595:

REST API: Do not include the password argument when getting media items

Currently, attachment is the only post type exposed via the REST API that
does not support password protection, but it's possible for other post types to
remove password support.

Fixes #38977.

[jnylen0]

Needs to be ported to the 4.7 branch.

[dd32]

In 39610:

REST API: Do not include the password argument when getting media items

Currently, attachment is the only post type exposed via the REST API that
does not support password protection, but it's possible for other post types to
remove password support.

Props jnylen0.
Merges [39595] to the 4.7 branch.
Fixes #38977.