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

Updating Embed URLs to Strip Trailing Slashes #12739

Closed
wants to merge 1 commit into from
Closed

Updating Embed URLs to Strip Trailing Slashes #12739

wants to merge 1 commit into from

Conversation

alpinealex
Copy link

@alpinealex alpinealex commented Dec 9, 2018

Description

Fixes #12664.

Updating oEmbed URLs to have the trailing slash stripped from them to prevent the "Sorry, we could not embed that content." when the url is actually a valid embeddable URL.

How has this been tested?

Manually - Adding new embed blocks to the page and testing different valid and invalid embed urls, with and without trailing slashes to ensure behavior still works as expected

Types of changes

Bug fix (non-breaking change which fixes an issue): #12664

Checklist:

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

@gziolo gziolo requested a review from notnownikki December 14, 2018 13:08
@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Block] Embed Affects the Embed Block labels Dec 14, 2018
Copy link
Member

@notnownikki notnownikki left a comment

Choose a reason for hiding this comment

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

I'd rather this removed the trailing slash if the embed failed, just so we can support embed URLs that do end with a trailing slash. That should be easy enough to do in componentDidUpdate.

Also, please add an e2e test for the behaviour :)

@alpinealex
Copy link
Author

@notnownikki That makes sense. I'll give it a go and push an update once its ready.

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

@alpinealex do you plan to revisit this PR? It has been almost 2 months since the last update.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 6, 2019
@arod2634
Copy link

arod2634 commented Feb 6, 2019

@gziolo I don't think I'll be able to get to this anytime soon. Feel free to pass on to someone else if there is desired.

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

Thanks for your time invested to fix this issue. I will close this PR to let someone else take it as a starting point and open their own branch. Feel free to reopen and continue when you find some time for it 👍

@gziolo gziolo closed this Feb 7, 2019
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 [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twitter embed fails with trailing slash on URL
4 participants