Using the JSMIN+ option works well but when I enable the JSMIN php extension then I get javascript errors. I have discovered that this is due to the scripts being compressed in a different order than with the JSMin+ option.

I believe that the call to jsmin() is happening too early. If I enable the JSMin+ option and then make the following code change, replacing the JSMinPlus call with a call to jsmin() then everything is fine. There is much logic that is happening before this which isn't happening before the real call to jsmin() so clearly the fault is not with the JSMIN php extension but with the advagg_js_compress module.

Index: advagg_js_compress/advagg_js_compress.module
===================================================================
--- advagg_js_compress/advagg_js_compress.module	(revision 1218)
+++ advagg_js_compress/advagg_js_compress.module	(working copy)
@@ -289,11 +289,12 @@
   try {
     $contents = str_replace(pack("CCC", 0xef, 0xbb, 0xbf), "", $contents);
     ob_start();
-    $contents = JSMinPlus::minify($contents);
-    $error = trim(ob_get_contents());
-    if (!empty($error)) {
-      throw new Exception($error);
-    }
+    $contents = jsmin($contents);
     $after = strlen($contents);
   }
   catch (Exception $e) {

Comments

tangent’s picture

I may have been wrong about the file order. I suspect my issue is caused by BOMs in the source files. #538274: Aggregated javascript contains BOM's and thereby breaks JSMIN+ says that JSMIN should handle this even though JSMIN+ does not. Is there a regression in JSMIN perhaps?

Copying the BOM stripping code before the jsmin() call resolves the issue for me.

mikeytown2’s picture

Status: Active » Needs review
StatusFileSize
new1.55 KB

Thanks for finding this bug! Here's a patch for you to review.

tangent’s picture

Status: Needs review » Active
StatusFileSize
new2.37 KB

Disregard my previous comment. JSMIN does not work even with BOM stripping. I was probably see a boost cached page or something when it seemed like it was working. The $contents variable is definitely getting rebuilt when JSMIN+ compression is used, versus JSMIN. The attached patch moves the JSMIN compression closer to where the JSMIN+ compression happens and I get no more errors.

mikeytown2’s picture

Status: Active » Needs review
mikeytown2’s picture

Status: Needs review » Fixed

Patch #3 has been committed. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.