Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect oEmbed responses where the oEmbed provider has gone away #10467

Merged
merged 3 commits into from
Oct 15, 2018

Conversation

notnownikki
Copy link
Member

@notnownikki notnownikki commented Oct 10, 2018

Description

Some embed providers still listed in core's allowed list are no longer valid. Either the third party's API endpoint has changed, or support has been removed completely.

In these cases, oEmbed returns a different response, and doesn't do the link fallback behaviour. This PR updates for those cases.

Also, it moves the logic of what is a valid preview and what couldn't be embedded out of the edit component.

How has this been tested?

Try to embed the following URLs where oEmbed has an incorrect API endpoint for the provider:

http://i294.photobucket.com/albums/mm102/magiciansteven/hel512_22.jpg
https://www.reverbnation.com/collection/186-mellow-beats
https://www.amazon.com/Maxboost-Protector-Tempered-Advanced-Accurate/dp/B073DLZWX7

You should see the "can't be embedded" message,

Try to embed this WordPress page that can't be embedded:

https://wordpress.org/gutenberg/handbook/

You should see the "can't be embedded" message,

Try to embed this invalid Twitter URL:

http://twitter.com/ufidosufdisofudisoufio

You should see the "can't be embedded" message,

Embed this valid URL:

https://twitter.com/notnownikki

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit, but the steps to test here read like they'd make an excellent E2E test. Is it possible to add an HTML page that would allow an oEmbed we could use to test (so the tests didn't require an outside connection)? If so that's worth adding (I guess even an HTML copy of a page with valid oEmbed data.)

If this isn't urgent I'd say we should add tests before merging. If it's urgent I'll cave 😉

@notnownikki
Copy link
Member Author

@tofumatt agreed, I'm writing the E2E test now. I'll put this as In Progress and add it to this PR.

@notnownikki notnownikki added the [Status] In Progress Tracking issues with work in progress label Oct 10, 2018
@notnownikki
Copy link
Member Author

To test as much as possible, we need to at least make the call to the embed proxy endpoint. I've added an e2e test that intercepts it and returns html that won't cause errors in the console, in the same way we have to for the demo page test.

The new e2e test makes sure that the embed block switches to the correct block based on the URL, aspect ratio class is applied to the embed with a fixed height/width iframe, and that embeds that can't be embedded end up back in the edit state.

@notnownikki notnownikki removed the [Status] In Progress Tracking issues with work in progress label Oct 10, 2018
@notnownikki notnownikki added this to the 4.1 milestone Oct 15, 2018
@notnownikki
Copy link
Member Author

e2e test is in this PR now

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to file a follow-up issue that maybe checked for network connectivity in general or something to skip these tests (and alert the user to why) in the future. 😄

Looks good to me though, let's 🚢

};

const addEmbeds = async () => {
await newPost();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might move this to setUp(), but that's about it.

@notnownikki notnownikki merged commit e8aa683 into master Oct 15, 2018
@youknowriad youknowriad deleted the update/embed-for-missing-oembed-endpoints branch October 15, 2018 12:20
@mtias mtias added the [Block] Embed Affects the Embed Block label Oct 18, 2018
@@ -0,0 +1,130 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "blocks" folder didn't exist at the time these tests were introduced, but at this point this file would be better placed as specs/blocks/embed.test.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants