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).
Comment | File | Size | Author |
---|---|---|---|
#12 | javascript_aggregator_bom.patch | 1013 bytes | derjochenmeyer |
Comments
Comment #1
Kiphaas7 CreditAttribution: Kiphaas7 commentedIn 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 :).
Comment #2
tetsuo2501 CreditAttribution: tetsuo2501 commentedSame here. I get Parse error: Illegal token in file '[inline]' on line 2132
Subscribe.
Comment #3
Kiphaas7 CreditAttribution: Kiphaas7 commentedTetsuo: 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/
Comment #4
gausarts CreditAttribution: gausarts commentedSame error here:
Any solution yet?
Comment #5
murokoma CreditAttribution: murokoma commentedSubscribing
Comment #6
Nigel CunninghamSubscribing
Comment #7
derjochenmeyer CreditAttribution: derjochenmeyer commentedThe 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?
Comment #8
Kiphaas7 CreditAttribution: Kiphaas7 commented#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.
Comment #9
derjochenmeyer CreditAttribution: derjochenmeyer commentedHow could we implement this?
just by calling this
before
?
Comment #10
Kiphaas7 CreditAttribution: Kiphaas7 commentedThis worked for me! No more errors, and all javascript works, allthough I only tested this rather quickly.
Comment #11
Kiphaas7 CreditAttribution: Kiphaas7 commentedChanging to needs work, since there is no patch, but a proposal for a working solution (at least working for me :)).
Comment #12
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere is the patch that i just committed in http://drupal.org/cvs?commit=354378
Thanks.
Comment #13
Kiphaas7 CreditAttribution: Kiphaas7 commented+ $contents = JSMinPlus::minify($file); }
I don't want to whine, but shouldn't that "}" be on a new line?
Comment #14
Kiphaas7 CreditAttribution: Kiphaas7 commentedstatus...
Comment #15
derjochenmeyer CreditAttribution: derjochenmeyer commentedfixed: http://drupal.org/cvs?commit=354514
Thanks :)
Comment #17
yonailo CreditAttribution: yonailo commentedHello,
After applying this patch I have problems with WAMP-2.2 under Windows 7 x64 bits.
If I leave these lines:
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.