Skip to content

Always use https for Vimeo video's. #10768

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

Merged
merged 1 commit into from
Sep 6, 2017

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Sep 4, 2017

Description

Instead of using the current protocol being used by Magento (http or https), always go for https. Since Vimeo redirects all non-https traffic to https.
See discussion with @orlangur in #10748

This still happens in the fotorama library though:

thumb = getProtocol() + 'img.youtube.com/vi/' + video.id + '/default.jpg';
img = thumb.replace(/\/default.jpg$/, '/hqdefault.jpg');
dataFrame.thumbsReady = true;
} else if (video.type === 'vimeo') {
$.ajax({
url: getProtocol() + 'vimeo.com/api/v2/video/' + video.id + '.json',

Not sure if this needs to be changed as well?

Watch out, I didn't test these changes, so please test before approving if possible :)

Once this is in the develop branch and backported to Magento 2.2 by someone, I'll update #10748 so this also gets into Magento 2.1
I don't want to have an awkward situation where this is merged in develop and 2.1 but not in 2.2

Fixed Issues (if relevant)

None

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@orlangur
Copy link
Contributor

orlangur commented Sep 5, 2017

Not sure if this needs to be changed as well?

Nope, but you can suggest such improvement to fotorama itself. Then it can appear in Magento someday after uprading the fotorama. Not sure what the process would be if there was a severe bug in third-party library but luckily this is not the case.

an awkward situation where this is merged in develop and 2.1 but not in 2.2

2.2 is currently closed for contributions, however, as to me it is not really necessary to backport as 2.2 contains another version of the fix. What do you think?

@hostep
Copy link
Contributor Author

hostep commented Sep 5, 2017

@orlangur: ok, but then #10748 should be merged in 2.1 as is without other changes.

Currently there is only a bug in 2.1, not in 2.2 or develop.
The changes proposed in this PR, are only nice-to-haves.

So summary:

  • using hardcoded http:// for vimeo url's is incorrect and will expose bugs when you run the shop over https (this is what happens on 2.1 right now)
  • using the same protocol for vimeo as the Magento shop isn't a problem, but has as a consequence that when running in Magento over http, it will perform 2 HTTP requests for every Vimeo loaded on the shop (this is what happens on 2.2 and develop right now)
  • using hardcoded https:// for vimeo url's in the best solution, as it will work perfectly on Magento shops using both http and https (this is what is proposed in this PR and will get merged to develop when approved)

@orlangur
Copy link
Contributor

orlangur commented Sep 5, 2017

then #10748 should be merged in 2.1 as is without other changes

Why? I thought it would be better similar to this PR, so that 2.1 and 2.3+ have one version and 2.2 will have redundant redirect on Vimeo for http, but it's not a big deal.

@hostep
Copy link
Contributor Author

hostep commented Sep 5, 2017

In this particular case it's not really a big problem, but it feels very wrong to me to leave 2.2 behind and have an improvement in 2.1 which is also in 2.3 but not in 2.2...

@magento-team magento-team merged commit c4da890 into magento:develop Sep 6, 2017
magento-team pushed a commit that referenced this pull request Sep 6, 2017
[EngCom] Public Pull Requests
 - MAGETWO-72350: Fix: Use all columns when running tests #10784
 - MAGETWO-72349: Fix: Move GitHub-specific documents into .github #10778
 - MAGETWO-72344: Enhancement: Configure preferred installation source in composer.json #10774
 - MAGETWO-72279: Always use https for Vimeo video's. #10768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants