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

FIX: vimeo iframe url when data-original-href is missing #18894

Merged
merged 3 commits into from
Jan 31, 2023

Conversation

ghassanmas
Copy link
Contributor

This commit make it possible to create vimeo orignal url when
data-original-href is missing.

For more info check:
https://meta.discourse.org/t/vimeo-embed-urls-parsed-incorrectly-in-email/231042

  This commit make it possible to create vimeo orignal url when
  data-original-href is missing.

  For more info check:
  https://meta.discourse.org/t/vimeo-embed-urls-parsed-incorrectly-in-email/231042
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/vimeo-embed-urls-parsed-incorrectly-in-email/231042/14

@SamSaffron
Copy link
Member

I like this but I feel like we need some tests to ensure this does not regress. also a bit fragile cause ?xyz=1&h=...

@ghassanmas
Copy link
Contributor Author

I agree, the thing is we need to convert the iframe src to the orignal video uri.

  • We can use same API https://vimeo.com/api/oembed.json?url= checking this I found that the api will return the vido link when setting the url params to the iframe url. i.e. https://vimeo.com/api/oembed.json?url=https://vimeo.com/api/oembed.json?url=https://player.vimeo.com/video/508864124?h=fcbbcc92fa
  • Second approach might be handy where we just replace the h part with a slash and other query params are shifted, if exists.

@nachocab
Copy link
Contributor

nachocab commented Nov 28, 2022

I like this but I feel like we need some tests to ensure this does not regress. also a bit fragile cause ?xyz=1&h=...

@SamSaffron What type of tests are needed to ensure this?

@jjaffeux
Copy link
Contributor

jjaffeux commented Jan 30, 2023

@nachocab I fixed conflicts and added a spec, please let me know if it seems correct to you if you have the time, thanks!

We could also look at making it more robust as sam explained.

@nachocab
Copy link
Contributor

@jjaffeux Thanks! Looks good 👍

@jjaffeux jjaffeux merged commit 96a6bb6 into discourse:main Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants