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

Convert video src to HTTPS #1274

Merged
merged 5 commits into from Jul 23, 2018

Conversation

Projects
None yet
3 participants
@hellofromtonya
Collaborator

hellofromtonya commented Jul 19, 2018

This PR automatically handles the conversion of the src and poster attributes from HTTP to HTTPS to generate valid AMP for the <amp-video> and its subsequent child nodes.

Work is focused within the AMP_Video_Sanitizer:

  • Forces by passing true to $this-maybe_enforce_https_src( $value, true ) for both the src and poster attributes.
  • Updates the child node(s) src attribute when it changes through the filtering.

Closes #976.

For Example

As noted in #976, when using a shortcode like this (which has a poster attribute added):

[video width="740" height="428" mp4="http://ads-up.fr/app/uploads/2016/06/BingAdsEditor-Mac-Multicomptes.mp4" loop="true" poster="http://www.pngmart.com/files/3/YouTube-Transparent-PNG.png"][/video]

this PR automatically converts the http to https for the valid AMP render:

<amp-video class="wp-video-shortcode i-amphtml-element i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-video-interface i-amphtml-layout" width="740" height="428" poster="http://www.pngmart.com/files/3/YouTube-Transparent-PNG.png" loop="" autoplay="" controls="" layout="responsive">
  <i-amphtml-sizer style="display: block; padding-top: 57.9048%;"></i-amphtml-sizer>
  <video playsinline="" webkit-playsinline="" poster="https://www.pngmart.com/files/3/YouTube-Transparent-PNG.png" class="i-amphtml-fill-content i-amphtml-replaced-content" loop="" controls="">
    <source type="video/mp4" src="https://ads-up.fr/app/uploads/2016/06/BingAdsEditor-Mac-Multicomptes.mp4?_=1">
  </video>
</amp-video>

amp-issue-976-force-https

hellofromtonya added some commits Jul 19, 2018

Force HTTPS on video and source src attribute.
This commit forces HTTPS on the `src` attribute.  For each child node, it sets the new filtered `src` attribute.

Fixes #976.
@kienstra

kienstra requested changes Jul 19, 2018 edited

Looks Good, Behaves As Expected

Hi @hellofromtonya,
Thanks, this PR looks really good, and it works as expected. It forces the <amp-video> srctohttps`.

There's just one question below, which might not need a change.

amp-video-src

<source src="http://example.com/video.mp4" type="video/mp4">
<source src="http://example.com/video.ogv" type="video/ogg">
</video>',
'<amp-video width="480" height="300" poster="https://example.com/video-image.gif" layout="responsive"><source src="https://example.com/video.mp4" type="video/mp4"><source src="https://example.com/video.ogv" type="video/ogg"></amp-video>',

This comment has been minimized.

@kienstra

kienstra Jul 19, 2018

Collaborator

Nice tests to ensure HTTPS is forced.

@@ -179,16 +184,16 @@ private function filter_attributes( $attributes ) {
foreach ( $attributes as $name => $value ) {
switch ( $name ) {
case 'poster':

This comment has been minimized.

@kienstra

kienstra Jul 19, 2018

Collaborator

This is an edge case. But do you think the poster should be forced to HTTPS like it is here?

It looks like HTTP is allowed for poster. For example, passing this amp-video with an HTTP poster to the AMP Validator results in a valid page.

The HTTPS request might fail if we upgrade it from HTTP. But we're already upgrading the src to HTTPS, and that might be on the same domain. So this probably wouldn't be a common issue.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 19, 2018

Collaborator

Good observation, @kienstra. You're right, it does validate. Therefore, we do not need to force the poster to HTTPS per the edge case you identified. I'll roll that part back.

@kienstra kienstra removed their assignment Jul 19, 2018

Revert `poster` attribute back to not force HTTPS.
AMP allows HTTP for the `poster` attribute.  It is valid AMP.  Therefore, in the edge case where the poster's URL is not HTTPS, we do not want to force a failed request.

@hellofromtonya hellofromtonya changed the title from Convert video src and poster to HTTPS to Convert video src to HTTPS Jul 19, 2018

@kienstra

Approved

Hi @hellofromtonya,
Thanks, this is approved.

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 19, 2018

Request For Review

Hi @westonruter,
No hurry, but when you have a chance, could you please review this pull request? It looks good, and works as expected.

Abstract update src functionality to new method.
The task of deciding whether to update and then handling the update is a separate task.  By abstracting, we also create a reusable method.
@@ -55,7 +56,7 @@ public function sanitize() {
return;
}
for ( $i = $num_nodes - 1; $i >= 0; $i-- ) {
for ( $i = $num_nodes - 1; $i >= 0; $i -- ) {

This comment has been minimized.

@westonruter

westonruter Jul 22, 2018

Member

Is adding a space for increment/decrement now in coding standards? This is the first I've seen it.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 23, 2018

Collaborator

@westonruter It's a byproduct of the auto-formatting setup in my PhpStorm style configuration. I'll correct it as it just looks weird.

Interestingly enough, it's valid syntax and passes our sniffers too.

Fixed in commit 06e23e0.

* @param string $new_src The new src attribute.
* @param string $old_src The old src attribute.
*/
protected function update_src( &$node, $new_src, $old_src ) {

This comment has been minimized.

@westonruter

westonruter Jul 22, 2018

Member

Not a problem but curious why this logic isn't inlined in the sanitize method? It seems a bit unnecessary to have a new method for this when putting an if statement would do.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 23, 2018

Collaborator

why this logic isn't inlined in the sanitize method?

@westonruter I abstracted to reduce the cognitive load.

At first, I had this if code block inline within that method. But as I read down line-by-line in the method, there's quite of bit of tasks going on.

Now when reading the method and you get to the $this->update_src(), you understand that it handles updating the src without having to worry about the details.

Also, note the pattern in the sanitize method and how it deals with all of the attributes and each node. There is an exception with the layout, which could be abstracted into the set_layout() that first calls the parent and then sets the 'responsive' layout. But other than that, the method works at a bigger level, i.e. all attributes, the node, and its child nodes. Setting the src is a granular task such as setting the layout or video dimensions.

@westonruter If you prefer, I can move back to the inline solution and remove the abstracted method.

@westonruter westonruter added this to the v1.0 milestone Jul 23, 2018

@westonruter westonruter merged commit e3d036c into develop Jul 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/convert-video-src-to-https branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment