Embed proxy API does not return HTML in all cases, so some providers will not embed

[notnownikki]

Issue Overview

Gutenberg uses the embed proxy API to get rendered HTML for URLs. This works for most cases, but the Kickstarter provider does not return any HTML, even though it returns a type of rich.

The classic editor POSTs to /wp-admin/admin-ajax.php requesting that the URL is rendered using an [embed] shortcode. This always returns HTML.

We do not want to call /wp-admin/admin-ajax.php because that is tied into the wp-admin area only, so we need to have an API which does the same as the call that the classic editor makes, but exposed through the REST API.

Steps to Reproduce (for bugs)

1. Try to embed a Kickstarter URL
2. Weep for all embedkind.

Note that sometimes on your second attempt to embed a specific KS url, it will return HTML, this is down to some inconsistent behaviour inside oembed. The /wp-admin/admin-ajax.php call will process the embed using the [embed] shortcode, and this always gives the desired result. The code that the admin ajax actions uses can be pretty much copied into a REST API endpoint to give us what we need, and have the same embed previewing as in the classic editor.

Todos

• Tests
• Documentation

[notnownikki]

First PR to start addressing this is at #5333 which removes the need to get the type in order to make videos responsive in the editor.

[jasmussen]

Added some REST API task labels, because of the needed endpoint. CC: @aduth

[notnownikki]

Second part of the fix is up at #5639

[aduth]

The html property is becoming unset (to false) at this line:

gutenberg/lib/compat.php

Lines 272 to 273 in a3776ed

	// Make sure the HTML is run through the oembed sanitisation routines.
	$response->html = wp_oembed_get( $_GET['url'], $_GET );

Introduced in #4226 (cc @imath)

Worryingly, this logic is such that we're fetching from the oEmbed API twice for every embed, because the cache is not shared between the original fetch by the API and the call to wp_oembed_get.

And in some brief testing, Kickstarter does have rate limiting in effect, so the empty response may very well be the result of a failed second request.

HTTP/1.1 429 Too Many Requests

[aduth]

If we're relying on some filtering to take place, the only thing different about what's happening in wp_oembed_get is that the $data response from the embed provider is being passed through this filter:

		/**
		 * Filters the HTML returned by the oEmbed provider.
		 *
		 * @since 2.9.0
		 *
		 * @param string $data The returned oEmbed HTML.
		 * @param string $url  URL of the content to be embedded.
		 * @param array  $args Optional arguments, usually passed from a shortcode.
		 */
		return apply_filters( 'oembed_result', $this->data2html( $data, $url ), $url, $args );

https://github.com/WordPress/wordpress-develop/blob/a7b3f5f759eb9b5840e3354cbb191058fcd5361b/src/wp-includes/class-oembed.php#L402-L411

We could recreate this in our filter treating $response as the $data argument.

[notnownikki]

Ah, that would explain why sometimes we only get metadata and no html...

Ok, well that filtering is only there if we're calling the embed proxy, and as this is a nee endpoint, it shouldn't be getting called.

[notnownikki]

We could recreate this in our filter treating $response as the $data argument.

That would work for embeds that go through oembed, and I'm still keen on having the new endpoint because there are plugins that handle things like youtube and bypass oembed completely, which is why we're constructing the [embed] shortcode (and why I spent so long screaming "But I'm calling a method on oembed, why isn't the code running! 😆 )

[aduth]

Can you elaborate on the purpose of a new endpoint? It's not entirely clear to me.

Ideally we don't proliferate endpoints just to solve varying use-cases or backwards-compatibility considerations, as it sounds like a future-compatibility nightmare.

[notnownikki]

Sure!

The classic editor does embedded content previews by POSTing a request to /wp-admin/admin-ajax.php for an embed-parse action. That ajax action does pretty much exactly what the new endpoint does - construct an [embed] shortcode and return the result.

We need something that does that for Gutenberg, and isn't tied to the admin area.

We need that because embed/1.0/proxy does not handle all embedding on all installations. There are plugins that bypass the oembed completely, and return different HTML. However, the [embed] shortcode picks up everything.

[aduth]

There are plugins that bypass the oembed completely, and return different HTML. However, the [embed] shortcode picks up everything.

This still sounds more like a bug with the existing proxy endpoint.

[notnownikki]

This still sounds more like a bug with the existing proxy endpoint.

I don't think it is. You can register new provider handlers with wp embed register handler which takes an ID, a regex, and a callback. The callback returns HTML. So you can unregister existing oembed providers, then register your own handler and bypass all the oembed metadata collection. We do this in some places for popular providers like youtube, so we're not hitting youtube all the time, and instead we return html from a template we put together ourselves. oEmbed's filters are never applied because our callback does everything.

[notnownikki]

But, i realised I wasn't clear about why bypassing oEmbed with a custom callback is a problem 😄

The problems are that we can't rely on the oembed_* filters getting applied (which isn't too much of a problem), and that one WP install might give a load of metadata for YouTube embeds, and another only gives us HTML. That's more of a problem, because right now, embed blocks depend on that metadata, and they shouldn't because it's not guaranteed to be there. So if we have an endpoint in Gutenberg, or stick with embed proxy, we can't rely on anything but the HTML being present in the response.

[designsimply]

Dropping by to note I tested this today and am confirming I was unable to add a Kickstarter link to an embed block—this error was returned when I tried testing:

Sorry, we could not embed that content.

Sample link used: https://www.kickstarter.com/projects/crowdcookware/the-blackbeard-the-essential-nonstick-nonscratch-f

^{Seen at http://alittletestblog.com/wp-admin/post.php?post=14025&action=edit running WordPress 4.9.7 and Gutenberg 3.2.0 using Firefox 61.0.1 on macOS 10.13.6.}

[notnownikki]

@pento @mkaz could you take a look at this with me please? Kickstarts embeds intermittently fail - I think it's some kind of rate limiting going on because there's some code in lib/rest-api.php which does the embed again to make sure it's going through oembed sanitisation, but that'll result in another call to kickstarter's oembed, and I believe that's the problem we're seeing here.

[pento]

Kickstarter does rate limiting, but it's pretty difficult to hit: I need to do 60 simultaneous wget requests to hit it. I can't hit it at all if I do them sequentially.

I can reproduce the behaviour, it seems like $response in gutenberg_filter_oembed_result() is initially populated with a proper response (with the <iframe>), but then it's overwritten later in the function.

I think we could actually return early from gutenberg_filter_oembed_result(), with something like:

$wp_oembed = _wp_oembed_get_object();
if ( false !== $wp_oembed->get_provider( $_GET['url'], array( 'discover' => false ) ) ) {
	return $response;
}

If it's a trusted embed provider, we don't need to do the second pass of sanity checking.

[swissspidy]

Related: https://core.trac.wordpress.org/ticket/45142

[mtias]

Closing as appears addressed in core and #10985.