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 handling of font stylesheets with non-HTTPS scheme or scheme-less URLs #1077

Merged
merged 3 commits into from Apr 17, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter commented Apr 17, 2018

  • When someone enqueues a font CDN stylesheet without the required HTTPS scheme, automatically supply it before checking against the regex and ensure the link has the HTTPS URL set during processing.
  • Improve URL checking by first ensuring the host name matches.
  • Ignore differences for URL scheme (even scheme-less URLs) when comparing URLs since the scheme is irrelevant.

Fixes #1072

westonruter added some commits Apr 16, 2018

Test actual codes for errors raised in validation error callback
Add tests for disallowed_file_extension and file_path_not_found
Improve get_validated_url_file_path to check URL host names and ignor…
…e differing schemes or scheme-less URLs
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Apr 17, 2018

Hi @westonruter, thanks for this PR. I'm reviewing it now.

) );
$sanitizer->sanitize();
$this->assertEqualSets( $error_codes, wp_list_pluck( $validation_errors, 'code' ) );

This comment has been minimized.

@kienstra

kienstra Apr 17, 2018

Collaborator

Nice use of $this->assertEqualSets(), I didn't know about that.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Apr 17, 2018

Approved

Hi @westonruter,
This pull request looks good, and it worked locally.

When enqueuing the Google Font file that @douglyuckling listed in #1072, the <link> appeared as expected, using both "Paired mode" (with a child theme of twentyten) and "Native AMP" (with the ampconf theme):

wp_enqueue_style( 'hemingway_googleFonts', '//fonts.googleapis.com/css?family=Raleway' );

Also, as expected, the <link> appeared with https, even if I changed the enqueuing URL to http://fonts...

@douglyuckling

This comment has been minimized.

Copy link
Contributor

douglyuckling commented Apr 17, 2018

Yep, works for me, too. Thanks!

@westonruter westonruter merged commit 5754263 into develop Apr 17, 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/font-url-protocol branch Apr 17, 2018

@westonruter westonruter modified the milestone: v1.0 Apr 17, 2018

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