Permalink
Browse files

minify-single: almost there (remove querystrings; check)

  • Loading branch information...
1 parent 29787cf commit 0163289002223629c63a6b3e479fb1a28ddd8af3 @futtta committed Mar 6, 2017
Showing with 15 additions and 6 deletions.
  1. +7 −2 classes/autoptimizeScripts.php
  2. +8 −4 classes/autoptimizeStyles.php
@@ -128,9 +128,14 @@ public function read($options) {
if (!empty($_CachedMinifiedUrl)) {
// replace orig URL with URL to cache
$newTag = str_replace($url, $_CachedMinifiedUrl, $tag);
- // TODO: remove querystring from URL in newTag
- $this->content = str_replace($tag,$newTag,$this->content);
+ } else {
+ $newTag = $tag;
}
+ // remove querystring from URL in newTag
+ $_querystr = next(explode('?',$source[2],2));
+ $newTag = str_replace("?".$_querystr,"",$newTag);
+ // and replace
+ $this->content = str_replace($tag,$newTag,$this->content);
}
// non-mergeable script (excluded or dynamic or external)
if (is_array($excludeJS)) {
@@ -173,9 +173,15 @@ public function read($options) {
if (!empty($_CachedMinifiedUrl)) {
// replace orig URL with URL to cache
$newTag = str_replace($url, $_CachedMinifiedUrl, $tag);
- // TODO: remove querystring from URL in newTag
- $this->content = str_replace($tag,$newTag,$this->content);
+ } else {
+ $newTag = $tag;
}
+
+ // remove querystring from URL
+ $_querystr = next(explode('?',$source[2],2));
+ $newTag = str_replace("?".$_querystr,"",$newTag);
+ // and replace
+ $this->content = str_replace($tag,$newTag,$this->content);
}
}
}
@@ -293,8 +299,6 @@ public function minify() {
$thiscss = str_replace($import,'',$thiscss);
$fiximports = true;
}
-
- // TODO: minify_single for non-aggregated files (but update import url in orig. css!!)
}
$thiscss = preg_replace('#/\*FILESTART\*/#','',$thiscss);
$thiscss = preg_replace('#/\*FILESTART2\*/#','/*FILESTART*/',$thiscss);

4 comments on commit 0163289

Owner

futtta commented on 0163289 Mar 6, 2017

@zytzagoo the above is part of brand new functionality to allow AO to minimize non-aggregated files. now I'm trying to become a better developer (well, a little bit) and I have scrutinizer complaining:

explode('?', $source[2], 2) cannot be passed to next() as the parameter $array expects a reference.

I based this on existing code, which uses current to get the URL, whereas this one tries to get the querystring. In all of my tests this is simply working, so how big of a problem is this do you think?

Contributor

zytzagoo replied Mar 6, 2017

@futtta hard to tell from a cursory glance, but strictly speaking, next() function takes its param by reference... which means it needs to be a reference to an existing variable (well, sort of).

you could maybe avoid the scrutinizer complaint by first storing the result of explode() in a variable and then passing/using that variable as an argument to next().

but if all you're doing is just trying to remove the query string from $source[2], maybe just a simple strtok() could work? Something like:

$newTag = strtok( $source[2], '?' ); // gets everything before the question mark
Contributor

zytzagoo replied Mar 6, 2017

As to the question of how much of a problem could it be in the wild if left as is... not really sure.
Depends on error reporting level among other things (php version etc.).

It should throw strict standards notice/warning along the lines of "only variables should be passed by reference", not sure how/why you're not seeing it? Have a look at http://stackoverflow.com/a/20165940 and http://stackoverflow.com/q/2354609

Static analysis isn't perfect, but usually, if you can make scrutinizer and tools like it happy(ier), the code is more robust.

Owner

futtta replied Mar 6, 2017

did it differently; exploding the URL and using the relevant array-value, doing away with exploding multiple times as well, cfr. 6ca290b

Please sign in to comment.