Fix embedding Twitter URLs with a trailing slash (Closes #12664)

[gbroques]

Description

Fixes #12664.

Fixed embedding tweets with a URL containing a trailing slash.

How has this been tested?

Manually tested the following cases:

1. Embedding valid Twitter URLs containing a trailing slash
2. Embedding valid Twitter URLs NOT containing a trailing slash
3. Tested embedding multiple Twitter blocks with trailing and non-trailing slashes

I'm looking into including a e2e test as suggested by @notnownikki, and the failed tests in the TravisCI build.

However, I can't seem to run the e2e tests locally. I'm getting the following error:

Error: The constant 'SCRIPT_DEBUG' is not defined in the 'wp-config.php' file.

Any idea on what I'm doing wrong or how to fix? I'm using the built-in local environment as suggested in End to End Testing

Types of changes

Bug fix - If the <EmbedEdit /> component can't embed the URL, then it resubmits the URL without a trailing slash.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[gziolo]

I noticed that one of the e2e tests fails on Travis (https://travis-ci.com/WordPress/gutenberg/jobs/187295522#L1062):

FAIL  specs/embedding.test.js (41.713s)
  ● Embedding content › should render embeds in the correct state
    waiting for selector "figure.wp-block-embed-wordpress" failed: timeout 30000ms exceeded
      152 | 		await page.waitForSelector( 'figure.wp-block-embed-twitter' );
      153 | 		await page.waitForSelector( 'figure.wp-block-embed-cloudup' );
    > 154 | 		await page.waitForSelector( 'figure.wp-block-embed-wordpress' );
          | 		           ^
      155 | 		// Video embed should also have the aspect ratio class.
      156 | 		await page.waitForSelector( 'figure.wp-block-embed-youtube.wp-embed-aspect-16-9' );
      157 | 
      at new WaitTask (../../node_modules/puppeteer/lib/FrameManager.js:840:28)
      at Frame._waitForSelectorOrXPath (../../node_modules/puppeteer/lib/FrameManager.js:731:12)
      at Frame.waitForSelector (../../node_modules/puppeteer/lib/FrameManager.js:690:17)
      at Page.waitForSelector (../../node_modules/puppeteer/lib/Page.js:1008:29)
      at Object.waitForSelector (specs/embedding.test.js:154:14)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (specs/embedding.test.js:9:103)

It might be related to the change in logic. It should be further investigated.

However, I can't seem to run the e2e tests locally. I'm getting the following error:

Error: The constant 'SCRIPT_DEBUG' is not defined in the 'wp-config.php' file.

Any idea on what I'm doing wrong or how to fix? I'm using the built-in local environment as suggested in End to End Testing

We changed the configuration of Docker on Friday, I'm wondering if you had your local environment set up earlier and simple Docker restart would solve it. If not, let us know, we will revert the change which updates SCRIPT_DEBUG's value causing your issue.

[notnownikki]

This looks like the right way to go, we just need those e2e tests fixing up.

The embed e2e tests mock all embed requests so we're not reliant on external services responding, they might be down, or respond with something unexpected.

In packages/e2e-tests/specs/embedding.test.js you'll see a bunch of MOCK_ setup at the start, where we configure the embed API to respond with known responses for each URL we're dealing with. Because this change will try to remove end slashes for URLs we can't embed, the URLs used in the test that respond with failure and end with a slash will also need responses for the versions that don't have the slash.

It should be straightforward to add those, but if any of it isn't clear (it might only be clear to me because I wrote it!) then ping me and I'm happy to help :)

[gbroques]

@gziolo @notnownikki Thank you both for you help! I'll see if I can find some time to look at this in the next few days and finish it out.

[gbroques]

@notnownikki @gziolo Having difficulties fixing the e2e tests for this.

I'd like to give this up to another contributor. Hopefully this issue is a little further along!

[notnownikki]

Whoever picks this up, you may need a little help with the e2e tests, they're one of the more complex ones because of the amount of mocking we have to do to test the embed block reliably. I'm happy to fix up the tests if anyone wants to collaborate :)

[gziolo]

Is there an easy way to fix the test so we could include it in WordPress 5.2? The logic is fixed and tests are less important than having this working for end users.

[notnownikki]

I can work on the test to get it working if that's ok with everyone?

[gziolo]

That would be awesome, I reopened PR to ensure it doesn’t get lost.

[notnownikki]

Picking this up now. Fix is looking great, just the amount of mocking we have to do for embed tests not to randomly fail is... more than a little :)

[notnownikki]

Looking at this more, and somehow there's an intermittent error. Sometimes the e2e tests pass, sometimes they don't, because sometimes the block gets the new preview and sometimes it doesn't. I'm guessing it's more to do with how state is being applied rather than external sites, as all the external requests for embed responses are mocked. Investigating more, it's not as simple as it seemed.

[notnownikki]

Oh boy, this was tricky to track down, but I've figured out what's causing the test to fail.

This PR is firing the slash removal if we cannotEmbed the current URL. cannotEmbed is set if we have set the URL and we haven't got a valid preview for the URL. BUT that is going to be the case when you've told the block to embed a URL and it hasn't yet fetched the preview. That's not normally a problem, because we hide the URL input UI while the fetching is going on, so we only display errors once we're no longer fetching, so we can be sure that we cannot embed.

However, the fix here fires immediately on setting the URL, and the WordPress URLs in the test have trailing slashes, and so it immediately resubmits without the slash. That's why the test intermittently passed for me locally, because things were happening fast enough for the preview to come back and the slash removal didn't fire sometimes.

The root cause of the problem is cannotEmbed is misleading. It implies that we know for sure this URL can't be embedded, when what it actually means as "at this moment in time we think this can't be embedded, but if we're still fetching the preview, that might change when we get the preview."

The fix I'd like to do here is to check if we've finished fetching before firing off resubmitWithoutTrailingSlash. The fix I want to do once the test and behaviour are fixed is to refactor the code slightly so the prop names are clear on what they actually mean.

[notnownikki]

New PR open at #14705 with an e2e test. @gbroques thank you for your work on this!

[gziolo]

We can close this one again. #14705 should land soon 🎉