Kinda cryptic title, but I wanted to cram as much info in there as possible. Basically the following happened. Installed the new 1.3 version that includes JSMIN+, and since I'm a regular on tweakers.net where the author of JSMIN+ resides, I decided to give it spin. Long story short, it doesn't work. It threw an error like this:

Parse error: Illegal token in file '[inline]' on line 4377
The corresponding line is:
;// $Id: drupal.js,v 1.41.2.3 2008/06/25 09:06:57 goba Exp $

At first, since JSMIN worked fine, and JSMIN+ didn't, I assumed it was a bug within JSMIN+. After contacting the author (the comments are in dutch, sorry), he told me that the "funny character" you see in the codeline, is actually a non-stripped BOM (Byte Order Mark), which is caused by aggregation.

He also said that even without minification, BOM's should throw js errors, but because JSMIN really parses and validates the code, it get's stripped. JSMIN+ does not do real parsing and validating, so it stays in and throws an error.

So, BOM's should be stripped, but I'm not sure if this is an issue with this module, or drupal core (since aggregation is part of core, but I'm not sure if the module does aggregation on it's own).

CommentFileSizeAuthor
#12 javascript_aggregator_bom.patch1013 bytesderjochenmeyer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Kiphaas7’s picture

In the comment above, the BOM is stripped away. Clicking on the link to the author of JSMIN+ leads you to the exact same example on the authors page where the BOM isn't stripped, but changed into entity, so it is really there, just not in the comment above :).

tetsuo2501’s picture

Same here. I get Parse error: Illegal token in file '[inline]' on line 2132
Subscribe.

Kiphaas7’s picture

Tetsuo: since the error is kinda vague (illegal token could be anything), are you sure it's also a BOM?

EDIT: a solution would be according to http://www.xs4all.nl/~mechiel/projects/bomstrip/

$fp = fopen('php://stdin','r');
$str = fread($fp, 3);
if ($str != "\xef\xbb\xbf")
    printf("%s",$str);
fpassthru($fp);
fclose($fp);
gausarts’s picture

Same error here:

Parse error: Syntax error in file '[inline]' on line 6648

Any solution yet?

murokoma’s picture

Subscribing

Nigel Cunningham’s picture

Subscribing

derjochenmeyer’s picture

The D6 version of this module only minifies the javascripts. Aggregation is done by Drupal core, so I'm not sure if we should fix it here.

Has anyone tried intregrating #3 into Javascript Aggregator?

Kiphaas7’s picture

Status: Active » Postponed (maintainer needs more info)

#7: Aggregation is indeed done by drupal core, but BOM's only become a problem for JSMIN+, because (according to the author of JSMIN+), JSMIN+ really parses & validates the file, and therefore fails when it encounters BOM's. JSMIN doesn't, and apparently browsers do not care about BOM's either.

So it's debatable whether it should be fixed in core, because it's not a problem in core, but the problem for this module stems for the aggregation in code.

Obviously the JSMIN+ author doesn't want to do BOM stripping, so if core does not want to fix it, a fix in this module for a problem which only this module has seems appropriate.

Allthough I also agree it should be fixed in core.

derjochenmeyer’s picture

How could we implement this?

just by calling this

$fp = fopen('script-to-minify.js','r');
$str = fread($fp, 3);
if ($str != "\xef\xbb\xbf")
    printf("%s",$str);
fpassthru($fp);
fclose($fp);

before

$contents = JSMinPlus::minify(file_get_contents(script-to-minify.js));

?

Kiphaas7’s picture

This worked for me! No more errors, and all javascript works, allthough I only tested this rather quickly.

        if (variable_get('javascript_aggregator_jsminplus', FALSE)) {
          // JSMin+ the contents of the aggregated file.
          require_once(drupal_get_path('module', 'javascript_aggregator') .'/jsminplus.php');
          // Strip Byte Order Marks (BOM's) from the file, JSMin+ cannot parse these.
          $file = str_replace(pack("CCC",0xef,0xbb,0xbf), "", file_get_contents(file_directory_path() . $aggregated_file_name));
          $contents = JSMinPlus::minify($file);
        }
Kiphaas7’s picture

Status: Postponed (maintainer needs more info) » Needs work

Changing to needs work, since there is no patch, but a proposal for a working solution (at least working for me :)).

derjochenmeyer’s picture

Status: Needs work » Fixed
FileSize
1013 bytes

Here is the patch that i just committed in http://drupal.org/cvs?commit=354378

Thanks.

Kiphaas7’s picture

+ $contents = JSMinPlus::minify($file); }

I don't want to whine, but shouldn't that "}" be on a new line?

Kiphaas7’s picture

Priority: Critical » Minor
Status: Fixed » Needs work

status...

derjochenmeyer’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

yonailo’s picture

Priority: Minor » Normal
Status: Closed (fixed) » Needs work

Hello,

After applying this patch I have problems with WAMP-2.2 under Windows 7 x64 bits.

If I leave these lines:

      //$file = str_replace(pack("CCC", 0xef, 0xbb, 0xbf), "", file_get_contents(file_directory_path() . $aggregated_file_name));
          $file = file_get_contents(file_directory_path() . $aggregated_file_name);
          $contents = JSMinPlus::minify($file);

Everything works well.

Has anybody tried the patch under windows + wamp ? I think that the str_replace + pack is too risky here, and under my DRUPAL installation I dont see the point. As was stated early javascript_aggregator minifies the files before DRUPAL aggregates them.

Anyway I am not an expert but I can tell you that Jsminplus version 1.4 + wamp 2.2 + my drupal installation does not work with the patch of this issue, the funny thing is that it does work in linux, so maybe a wamp issue here, I dont really know.