WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#34207 closed task (blessed) (fixed)

Leverage the REST API structure for the oEmbed endpoint

Reported by: swissspidy Owned by: rmccue
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

After #32522 and #33982 we can now leverage the powerful REST API for the oEmbed API endpoint.

Luckily, the oEmbed feature plugin has supported the REST API from the start. This makes updating WP_oEmbed_Controller to use the REST API very easy.

Benefits from doing this:

  • Synergies: Robust API routing instead of doing it on our own.
  • Argument sanitization out of the box.
  • Easy JSON schema support for devs

Attachments (5)

34207.diff (30.8 KB) - added by swissspidy 4 years ago.
34207.2.diff (31.1 KB) - added by swissspidy 4 years ago.
34207.3.diff (31.1 KB) - added by swissspidy 4 years ago.
Removes remaining textdomain, uses get_status_header_desc
34207.4.diff (29.2 KB) - added by swissspidy 4 years ago.
34207.5.diff (33.9 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (32)

#1 @swissspidy
4 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#2 @rmccue
4 years ago

  • Milestone changed from Awaiting Review to 4.4

I'm Commander Shepard, and this is my favourite ticket on Trac.

@swissspidy
4 years ago

#3 @swissspidy
4 years ago

  • Keywords has-patch added; needs-patch removed

Consider this being my contribution to the REST API :)

34207.diff does the following:

  • Remove unneeded query vars
  • Registers the API route in WP_oEmbed_Controller
  • Removes wp_oembed_parse_query in favour of wp_oembed_register_route
  • Introduces _oembed_rest_pre_serve_request to handle XML format
  • Updates tests

All in all these are rather small changes, which just shows us how easy it is to work with the API.

I'd like @rmccue and @pento to have a look at this.

#4 @danielbachhuber
4 years ago

Easy JSON schema support for devs

The JSON schema implementation wasn't ever formalized, or even discussed at a ton of depth, so we should do so before we commit too much does that depends on it.

Just to clarify, it shouldn't block merge of the patch, but there may be minor changes afterwards.

Last edited 4 years ago by danielbachhuber (previous) (diff)

#5 @swissspidy
4 years ago

The JSON schema implementation wasn't ever formalized, or even discussed at a ton of depth, so we should do so before we commit too much does that depends on it.

Just to clarify, it shouldn't block merge of the patch, but there may be minor changes afterwards.

We could also skip the schema part and wait for 4.5 and the other endpoints to add it.

#6 @DrewAPicture
4 years ago

Maybe I'm missing something, but it seems like doing this in 4.4 would kind of defeat the purpose of splitting the REST API merge into a multi-stage process: Infrastructure in 4.4, the goodies in 4.5.

@danielbachhuber also has a point about the JSON schema not having been finalized yet.

#7 @wonderboymusic
4 years ago

the endpoint split is because there is no good auth solution yet - if this endpoint doesn't require auth, shouldn't matter

#8 follow-up: @pento
4 years ago

I'm cool with this patch, pending some discussion on the schema.

@danielbachhuber, are there any particular changes you're expecting to the schema implementation? Particularly, anything that would change the output format?

#9 @rmccue
4 years ago

The schema-based pieces that we merged are just for exposing it via an OPTIONS request, so it's really just bundled documentation more than anything. The schema can be used for more, but that's part of the base controller (WP_REST_Controller), which is in the endpoints stage. I think it's fine as-is.

The patch looks good to me, though I've only taken a cursory glance so far.


One thing we might want to consider is a different namespace other than wp/v2 - while this is a core endpoint, it's kind of separate, as it's linked to the oEmbed spec, not our endpoint versioning. Perhaps oembed/v1 for the namespace, with the endpoint itself as embed.

Having a separate namespace means a few things. One of these is that there's no potential conflicts with the future endpoints (unlikely, but possible).

The main one IMO though is feature detection support: the namespaces key in the API index indicates which namespaces are available, and lets clients detect features. To detect the change from 4.4 to our future version with the endpoints, it'd be nice to simply check for wp/v2 in this list. Likewise, you could check for oembed/v1 here to see if the site has oEmbed enabled (if it's missing, the site has probably disabled oEmbed).

If a future oEmbed spec comes out, we could then use oembed/v2 or similar for that, which is a bonus. (This is because we're implementing an external specification, rather than making our own.)

I've convinced myself; let's change it to oembed/v1 ;)

#10 in reply to: ↑ 8 @danielbachhuber
4 years ago

Replying to pento:

I'm cool with this patch, pending some discussion on the schema.

@danielbachhuber, are there any particular changes you're expecting to the schema implementation? Particularly, anything that would change the output format?

Nothing specific — just that I'd rate the implementation a v0.6 out of v1.0, and it merits some discussion as to what we might need to do (if anything) to feel comfortable calling it v1.0.

We can discuss during Monday / Tuesday's REST API chat. I'll do my best to come prepared with some implementation questions to guide the conversation.

@swissspidy
4 years ago

#11 @swissspidy
4 years ago

  • Keywords has-unit-tests added

I just attached a new patch. Changes in 34207.2.diff compared to the previous version:

  • Patch applies cleanly again against trunk
  • No JSON schema for now. We can add that when the implementation is ready.
  • Uses /oembed/1.0/embed for the endpoint instead of /wp/v2/oembed.

Note: Providers like VideoPress and Tumblr use 1.0 in their routes as well so I thought this is more fitting.

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


4 years ago

#13 @danielbachhuber
4 years ago

tl;dr: you shouldn't include the JSON schema code in this patch.

Here are my notes on the JSON schema conversation:

Q: What is the purpose of defining the resource with a schema?

  • To have the format of the object documented in a structured manner. Because it's documented in a structured manner, we can automagically create human-readable documentation from it.
  • Closely-coupling structured documentation to the code will help prevent it from getting out of date.
  • We can also use the schema internally to programmatically manipulate the request or response (e.g. validate required parameters, filter response based on context)

Q: Why JSON schema?

  • It is quite graybeard. JSON schema has been around for a long time, and is a well-known standard.
  • We can extend JSON schema base spec to include our own attributes, and publish this extension.
  • There aren't any competing standards that are better.

Q: What needs to be done to finalize our implementation for 4.4?

  • Most pieces related to our schema implementation shouldn't be committed to core for 4.4.
  • However, we have support for schema in the registration, but the implementation takes an arbitrary array/object and spits it out on OPTIONS requests. There's no need to remove it at this time.

Q: What might be done later?

  • Document and publish the ways in which our schema differs from JSON schema draft 4
  • Probably lots of other little things.

@swissspidy
4 years ago

Removes remaining textdomain, uses get_status_header_desc

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


4 years ago

#15 @swissspidy
4 years ago

  • Owner changed from swissspidy to rmccue
  • Status changed from assigned to reviewing

@rmccue Can you have a look at this? I heard you know a little bit about the REST API.

#16 @wonderboymusic
4 years ago

  • Type changed from enhancement to task (blessed)

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


4 years ago

@swissspidy
4 years ago

#18 @swissspidy
4 years ago

34207.4.diff is a refreshed patch against trunk.

#19 @swissspidy
4 years ago

  • Keywords commit added

#20 @wonderboymusic
4 years ago

  • Keywords commit removed

Patch is blowing up

@swissspidy
4 years ago

#21 @swissspidy
4 years ago

34207.5.diff is a new patch that applies cleanly against trunk.

#22 @pento
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 35436:

Embeds: Who put this REST API infrastructure in my WordPress?

Well, while it's here, we probably should make use of it. The oEmbed endpoint now uses the REST API infrastructure, instead of providing its own.

Props swissspidy.

Fixes #34207.

#23 follow-up: @mark-k
4 years ago

This feels premature. Does it mean that I can not have oEmbed provider without REST-API? What happens if I already have a wp-json directory on my server? There is no option I can see to control it so I am just going to have to hack the code to find the reason why my json/oembed do not work? What happens if I already have a wp-json site on my network?

#24 in reply to: ↑ 23 @johnbillion
4 years ago

Replying to mark-k:

Does it mean that I can not have oEmbed provider without REST-API?

Correct, but you don't need to worry because they are both new features in WordPress 4.4.

What happens if I already have a wp-json directory on my server? What happens if I already have a wp-json site on my network?

These are edge cases. The same argument could be applied to any new feature which requires an endpoint, in which case we would never add any new features. The rest_url filter is available to filter the REST API endpoint if you do indeed have a site on your network or a subdirectory named wp-json.

#25 follow-up: @mark-k
4 years ago

Replying to johnbillion:

Why do I have to have REST-API for a feature that doesn't make any use of it?

<devil advocate>

I have a simple blog. I have no desire to have it meshed up in some other site, but I do want to oEmbed a post in my other posts from time to time. I am running on a free hosting and would prefer to keep it that way, but the price to pay for that is that I need to optimize all aspects of my site, get rid of any service I do not use, therefor naturally I want to block the REST-API. But now (ok, in 4.5 when there will be actual active end points) I can not do that because it will bring down oEmbed.

This is what happens now with XML-RPC that you can not just turn off as there are so many services running over it (and turning off, is just at webserver level as loading wordpress and all the plugins and parsing the URLs is just too expensive).

</devil>

For me this is violation of software engeneering rules

  1. Separation of interests. If oEmbed do not depend on REST API, then it should not depend on it in any way
  1. (this is really an rest-api issue) No hardcoding of anything that do not have to be hardcoded in a name space where you do not expect things to be hardcoded. It is one thing when a plugin wants to use a specific URL structure as it will most likely fail at activation or right after it, but a wordpress upgrade that breaks plugins? How should anyone handle it? where is the backward compatibility promised by core to convince people to auto upgrade?

Rest-API endpoint should be wp-json.php (why json? why not wp-rest? hopefully it can return data in XML and CSV formats as well) an oembed should be oembed.php. Pretty URLing for people that feel the need for it (probably no one as after all this is a computer to computer communication and computers do not care about url structure) should be easy to do at htaccess level or plugins that add rewrite rules.

#26 @mark-k
4 years ago

And of course what will happen to my glorious post about wp-json that I wrote in my blog which use %postname% for permalink structure?

#27 in reply to: ↑ 25 @rmccue
4 years ago

Replying to mark-k:

Why do I have to have REST-API for a feature that doesn't make any use of it?

It does make use of it, hence the point of this ticket. It replaces the JSON handling in oEmbed, and uses well-tested infrastructure instead of duplicating it.

naturally I want to block the REST-API. But now (ok, in 4.5 when there will be actual active end points) I can not do that because it will bring down oEmbed.

In future versions, we'll likely start using the REST API instead of certain admin-ajax.php calls in the admin as well (with fallbacks for people without JS). I'd suspect that disabling the REST API will cause problems (or at least a worse experience) in the future.

For me this is violation of software engeneering rules

  1. Separation of interests. If oEmbed do not depend on REST API, then it should not depend on it in any way

In the previous code, there was duplication of things already in the API, including JSONP handling (which has security implications if we need to duplicate that code, since it increases the potential attack surface). Combining the two makes a lot of sense.

  1. (this is really an rest-api issue) No hardcoding of anything that do not have to be hardcoded in a name space where you do not expect things to be hardcoded. It is one thing when a plugin wants to use a specific URL structure as it will most likely fail at activation or right after it, but a wordpress upgrade that breaks plugins? How should anyone handle it? where is the backward compatibility promised by core to convince people to auto upgrade?

wp-json isn't hardcoded, as @johnbillion noted; you can filter it, and clients are expected to work automatically with this via the discovery process.

I'm not sure what backwards compatibility you're expecting that we're breaking here?

Rest-API endpoint should be wp-json.php

We've tried that in the past, but there are various problems with having it as a real PHP file that are handled quite nicely by using rewrites instead. For those without rewrites, there's already a fallback.

Note: See TracTickets for help on using tickets.