Permalink
Browse files

Rework CDN & Font logic to make sure fonts are only CDN'd if filter t…

…ells it to.
  • Loading branch information...
futtta committed Mar 19, 2017
1 parent 1ea4c6e commit 65cd2d1c865cffe3a925d8988cc0cc28d67160c9
Showing with 15 additions and 31 deletions.
  1. +15 −31 classes/autoptimizeStyles.php
@@ -332,11 +332,6 @@ public function minify() {
// Do the imaging!
$imgreplace = array();
- // dtbaker update. Instead of searching for the entire `background: url(), url(), url();` string we just search for the individual url() elements.
- // this allows us to find/replace multiple background images with minimal code changes below.
- // this is the old regex that searched for the entire background css rule, but it wouldn't match multiple background image url css rules.
- // preg_match_all('#(background[^;{}]*url\((?!\s?"?\'?\s?data)(.*)\)[^;}]*)(?:;|$|})#Usm',$code,$matches);
- // this new regex will be slightly faster too:
preg_match_all( self::ASSETS_REGEX, $code, $matches );
if ( ($this->datauris == true) && (function_exists('base64_encode')) && (is_array($matches)) ) {
@@ -408,42 +403,20 @@ public function minify() {
} else {
// just cdn the URL if applicable
if (!empty($this->cdn_url)) {
- $url = trim($quotedurl," \t\n\r\0\x0B\"'");
- $cdn_url=$this->url_replace_cdn($url);
- $imgreplace[$matches[0][$count]] = str_replace($quotedurl,$cdn_url,$matches[0][$count]);
- }
+ $imgreplace[$matches[0][$count]] = str_replace($quotedurl,$this->maybe_cdn_urls($quotedurl),$matches[0][$count]);
+ }
}
}
} else if ((is_array($matches)) && (!empty($this->cdn_url))) {
- // change background image urls to cdn-url
+ // change urls to cdn-url
foreach($matches[1] as $count => $quotedurl) {
- $url = trim($quotedurl," \t\n\r\0\x0B\"'");
- $cdn_url=$this->url_replace_cdn($url);
- $imgreplace[$matches[0][$count]] = str_replace($quotedurl,$cdn_url,$matches[0][$count]);
+ $imgreplace[$matches[0][$count]] = str_replace($quotedurl,$this->maybe_cdn_urls($quotedurl),$matches[0][$count]);
}
}
if(!empty($imgreplace)) {
$code = str_replace(array_keys($imgreplace),array_values($imgreplace),$code);
}
-
- // CDN the fonts!
- if ( (!empty($this->cdn_url)) && (apply_filters('autoptimize_filter_css_fonts_cdn',false)) && (version_compare(PHP_VERSION, '5.3.0') >= 0) ) {
- $fontreplace = array();
- include_once(AUTOPTIMIZE_PLUGIN_DIR.'classlesses/autoptimizeFontRegex.php');
-
- preg_match_all($fonturl_regex,$code,$matches);
- if (is_array($matches)) {
- foreach($matches[8] as $count => $quotedurl) {
- $url = trim($quotedurl," \t\n\r\0\x0B\"'");
- $cdn_url=$this->url_replace_cdn($url);
- $fontreplace[$matches[8][$count]] = str_replace($quotedurl,$cdn_url,$matches[8][$count]);
- }
- if(!empty($fontreplace)) {
- $code = str_replace(array_keys($fontreplace),array_values($fontreplace),$code);
- }
- }
- }
// Minify
if (($this->alreadyminified!==true) && (apply_filters( "autoptimize_css_do_minify", true))) {
@@ -681,4 +654,15 @@ private function can_inject_late($cssPath,$css) {
return true;
}
}
+
+ private function maybe_cdn_urls($inUrl) {
+ $url = trim($inUrl," \t\n\r\0\x0B\"'");
+ // exclude fonts from CDN except if filter returns true
+ if ( !preg_match('#\.(woff2?|eot|ttf|otf|svg)$#i',$url) || apply_filters('autoptimize_filter_css_fonts_cdn',false) ) {
@zytzagoo

zytzagoo Mar 20, 2017

Collaborator

Could this mean that regular .svg files will not be cdn-ed unless fonts are cdn-ed too?

@futtta

futtta Mar 20, 2017

Owner

yeah, was hesitating there.
either

  • all svg's are excluded unless the font-cdn filter returns true (as we "assume" they're fonts)
  • or all svg's are cdn'ed as we assume they're not fonts.

I went for the first approach as that doesn't break anything, whereas the 2nd might (if svg-fonts they could get blocked as cross-domain).

But all of that is pretty edge-case-y anyhow ...

@zytzagoo

zytzagoo Mar 20, 2017

Collaborator

Yeah, but it's kind of a BC break, in the sense that previously cdn-ed .svg urls will no longer be cdn-ed (for those that don't have the autoptimize_filter_css_fonts_cdn turned on [which is off by default, so, a lot of people]).

Considering the install numbers, someone will report it sooner or later :)

@futtta

futtta Mar 20, 2017

Owner

true enough, but I don't feel like this non-breaking regression is important enough to add extra code to check against the context in which the SVG are (background image vs font), do you?

@zytzagoo

zytzagoo Mar 20, 2017

Collaborator

No.

But, to be honest, I strongly feel turning on cdn should do exactly as it says on the box, rewrite all assets to the specified cdn url.

Users with shitty/broken cdns should either fix/change their cdn or not use the cdn feature.

Alternatively, they could maybe write/pay-for a few filters in a mu-plugin that will maybe somehow exclude their fonts from being cdn-ed or something, in case that this is causing them that much pain (which I find almost impossible to believe -- having static assets on the cdn and considering that mission-critical while not having fonts also be cdn-ed -- why would anyone do that?)

@futtta

futtta Mar 21, 2017

Owner

yeah ... so I removed SVG from the list, based on the assumption that SVG as font (and without a proper CDN) is is more rare than SVG as background-image. We'll see what happens once released ;-)

+ $cdn_url = $this->url_replace_cdn($url);
+ } else {
+ $cdn_url = $url;
+ }
+ return $cdn_url;
+ }
}

0 comments on commit 65cd2d1

Please sign in to comment.