Embeds: Replace Facebook embeds handler with oEmbeds

[ockham]

Changes proposed in this Pull Request:

If WordPress doesn't find a handler for a given embed, it falls back to the corresponding oEmbed handler, if present. The opposite, however, is apparently not true. For this reason, for WP versions >= 5.5.3, where the Facebook oEmbed handler was removed (since Facebook started requiring an auth token to access its API), wp_oembed_get returns false for a Facebook URL. This was reported by @emrikol (thanks!)

The solution is to replace the 'oldschool' embed handler with an oEmbed one, and add some filters to include the auth token if present, or proxy the request through WP.com's oembed proxy endpoint otherwise (the latter functionality hasn't been tested yet; it will require applying the WP.com counterpart diff to a sandbox, and sending API requests to that sandbox).

This PR is modeled largely after #17356 that did pretty much the same for Instagram oEmbeds.

Jetpack product discussion

p1605888255049400-slack-CDD9LQRSN

Does this pull request change what data or activity we track or use?

No

Testing instructions:

TBD. See #17356 for now.

Don't forget to set the auth token (by either setting the JETPACK_FACEBOOK_EMBED_TOKEN constant, or using the jetpack_facebook_embed_token filter), as the WP.com proxy fallback hasn't been tested yet.

Proposed changelog entry for your changes:

Embeds: Replace Facebook embeds handler with oEmbeds

TODO

Get WP.com proxy fallback to work.

FYI @jeherve @ebinnion

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D53118-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[jetpackbot]

Scheduled Jetpack release: December 1, 2020.
Scheduled code freeze: November 23, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17849

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 6899cd7

[ebinnion]

I'm not sure that we need to, nor should, do this. Unless I'm misunderstanding, this will:

• Add a network request to get the embed where we didn't need to make one before
• As we proxy that request, we'll also increase how many authenticated requests we need to make which will go against our quota of API calls

In testing Facebook embeds with WP 5.5.3, we get a placeholder embed in the editor and a proper embed on the frontend. Would this improve the embed experience for users? For example, would this allow us to actually embed in the editor?

[WickedWolfy]

Thank you for the change. This should fix the issue we are having with Facebook oEmbeds.

[emrikol]

@ebinnion With the caveat that I've not tested these changes yet, here's some thoughts I have:

This should allow WordPress core to use the oEmbed API, which would result in a remote request, but it should also cache the result in post meta like other oEmbeds.

The primary issue we are seeing is that oEmbeds are working for Instagram URLs, via wp_oembed_get() but they are not working for Facebook URLs. This results in a mixed experience for sites who have more complex workflows, such as displaying post content and/or embeds via the REST API.

As I'd mentioned, I've not tested this yet and I don't know what kind of contents are returned for the Facebook oEmbeds, but if this proxying isn't an option, one thing we might try is to add a hook and capture Facebook URL oEmbed requests and return the data from jetpack_facebook_embed_handler() instead?

[ockham]

In testing Facebook embeds with WP 5.5.3, we get a placeholder embed in the editor and a proper embed on the frontend. Would this improve the embed experience for users? For example, would this allow us to actually embed in the editor?

@ebinnion TBH, I think that part has always been a different issue: We're setting previewable: false for the block embed:

jetpack/extensions/extended-blocks/core-embed/facebook.js

Line 24 in 3e88dca

previewable: false,

I copied that over verbatim from Gutenberg in #17192 when they deprecated the embeds there. I tried to track down why they did that in the first place, but my findings weren't too conclusive: The best I found was this comment:

[...] previewing facebook embeds is disabled, because fb scripts want to change global things about the document they're running in (which the sandboxing iframe highlighted and wouldn't allow).

We might revisit that though, I'm not sure if that still holds.

[ockham]

Thanks for weighing in @ebinnion, @WickedWolfy, and @emrikol!

@ebinnion With the caveat that I've not tested these changes yet, here's some thoughts I have:

This should allow WordPress core to use the oEmbed API, which would result in a remote request, but it should also cache the result in post meta like other oEmbeds.

👍

The primary issue we are seeing is that oEmbeds are working for Instagram URLs, via wp_oembed_get() but they are not working for Facebook URLs. This results in a mixed experience for sites who have more complex workflows, such as displaying post content and/or embeds via the REST API.

Yeah, that's the main rationale for this PR. The oEmbed-based approach seems overall more compatible/versatile.

As I'd mentioned, I've not tested this yet and I don't know what kind of contents are returned for the Facebook oEmbeds, but if this proxying isn't an option, one thing we might try is to add a hook and capture Facebook URL oEmbed requests and return the data from jetpack_facebook_embed_handler() instead?

I've tested it so far with providing my own access token (through the jetpack_facebook_embed_token filter), which bypasses the WP.com proxy. Could that be an option for y'all, @WickedWolfy?

As for the WP.com proxy fallback: That actually doesn't work yet 🙃 Unlike IG, we don't have a custom oEmbed proxy endpoint on WP.com for FB embeds, so instead, I'm trying to use Core's /oembed/1.0 proxy endpoint. In theory, that's a cleaner approach than a custom one anyway, but in practice, it's currently not working 😅

FWIW, I've started a PR to move the IG oEmbed handler from the custom endpoint to /oembed/1.0, where I'll try to isolate the issue.

[ebinnion]

@ebinnion TBH, I think that part has always been a different issue: We're setting previewable: false for the block embed:

Yes sir. I was familiar with this. My purpose in mentioning the placeholder and the embed on the frontend was that things were working as they had before for the majority of users.

This should allow WordPress core to use the oEmbed API, which would result in a remote request, but it should also cache the result in post meta like other oEmbeds.

I understand that. I'm not so concerned about the network request out, since it's cached. I'm more concerned that that network request will likely then be proxied to the Facebook embed API and we'll end up with 10, or 10s, of Millions of API requests when we don't need to make those requests.

Yeah, that's the main rationale for this PR. The oEmbed-based approach seems overall more compatible/versatile.

As long as the WP.com patch that goes along with this then doesn't proxy the request to Facebook's servers, I have no issues.

[ockham]

@ebinnion TBH, I think that part has always been a different issue: We're setting previewable: false for the block embed:

Yes sir. I was familiar with this. My purpose in mentioning the placeholder and the embed on the frontend was that things were working as they had before for the majority of users.

Gotcha 👍

This should allow WordPress core to use the oEmbed API, which would result in a remote request, but it should also cache the result in post meta like other oEmbeds.

I understand that. I'm not so concerned about the network request out, since it's cached. I'm more concerned that that network request will likely then be proxied to the Facebook embed API and we'll end up with 10, or 10s, of Millions of API requests when we don't need to make those requests.

The proxied approach seemed nicer in that it was consistent with what we're doing with IG already, and I was (maybe falsely) assuming that our quota would either be enough (I recall that you mentioned in Slack that we saw a dropoff at some point), or FB would be willing to up it some more if all else failed, but I don't really have any skin in this game.

Yeah, that's the main rationale for this PR. The oEmbed-based approach seems overall more compatible/versatile.

As long as the WP.com patch that goes along with this then doesn't proxy the request to Facebook's servers, I have no issues.

That's going to be tricky with the current approach, since without it, FB embeds would break on JP sites.

The alternative would be to basically move the 'faux' embed logic from jetpack_facebook_embed_handler into a filter that hooks into pre_ombed_result. This is what @emrikol originally had (p1605892075053000-slack-CDD9LQRSN). The faux embed might be less elegant, but won't require proxying through WP.com servers.

A hybrid approach could also work: Move the 'faux' embed to pre_ombed_result, but keep the authenticated oEmbed API filters (jetpack_facebook_oembed_fetch_url and jetpack_facebook_oembed_remote_get_args) around, and add checks for the return value of jetpack_facebook_get_access_token() to all filters in order to determine which approach to use. We'd need to verify that both approaches return the same markup though, and it's overall questionable if it's worth the hassle.

@ebinnion @emrikol Can I hand this one over to y'all? This was kind of a one-off based on the IG oEmbed stuff I did about a month ago, but my primary focus isn't really JP anymore these days (but Gutenberg and WP.com), so I'm afraid I don't have a lot of time and headspace to see this through 😅 (as is probably evident from the low frequency that I comment on this PR)

[ebinnion]

The proxied approach seemed nicer in that it was consistent with what we're doing with IG already, and I was (maybe falsely) assuming that our quota would either be enough (I recall that you mentioned in Slack that we saw a dropoff at some point), or FB would be willing to up it some more if all else failed, but I don't really have any skin in this game.

To be fair, I don't have concerns over our quota in the near, and perhaps mid, future. That being said, I'd rather not add to our usage unnecessarily.

@ebinnion @emrikol Can I hand this one over to y'all?

My opinion is to pass on this being handled in Jetpack for the moment. In this case, I'd suggest that we work with the VIP client to get pre_ombed_result to work as expected.

[jeherve]

I'll close this PR for now because of the lack of activity on this. We can always reopen in the future if needed, but it will need a rebase, so it may be easier to start a new PR at this point.