Breaks on paths with HTTP GET parameters
k8n - October 24, 2008 - 06:24
| Project: | Javascript Aggregator |
| Version: | 5.x-1.5 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Description
I think it is to defeat caching that Ubercart appends a ?get=parameter to paths of some js files it references.
javascript_aggregator (1) fails to find such js files (e.g. ../uc_cart.js?03423), (2) should at least exclude such files from aggregation in good faith.
Attached is a patch. A regexp was modified to fail match against javascript file urls that contain get parameters. This does not however solve javascript_aggregator's Ubercart compatibility.
| Attachment | Size |
|---|---|
| js_aggregator_sane_exclusion.patch | 786 bytes |
#1
You can just ignore/remove the GET parameter. The file will be aggregated and will work fine.
#2
Stumbled on the same issue with Ubercart. I'd suggest to remove these URL tricks, since javascript_aggregator users should be accustomed to clearing their JS cache on site upgrades. Here is a suggested patch. Without this fixed, the module breaks badly on pages using ubercart, since it thinks the files were added but they were not.
Alternatively we could also do a !file_exists() check and add the script back to the $scripts_js_links if the file was not there (eg. it contained a ? argument), but that would also make us keep 404 references in JS files, which are a good thing to remove as a side effect of this patch IMHO.
Issue still exists in the 5.x-1.5 version, so setting that.
#3
Also, retitling for a hopefully better summary.
#4
Thanks for the patch! http://drupal.org/cvs?commit=215436
#5
Automatically closed -- issue fixed for 2 weeks with no activity.
#6
I think the patch was wrongly applied. The current state of the module is that every file is included twice - once as is, and another with paramteres removed.
$ grep misc/jquery.js e71d1bfc0feea86a918c51550ca47ac8.js/* AGGREGATED JS FILE: misc/jquery.js */
/* AGGREGATED JS FILE: misc/jquery.js */
The current code has:
<?php$data = file_get_contents($scripts_js_file);
$contents .= ";\n/* AGGREGATED JS FILE: $scripts_js_file */\n".$data."\n";
// Eliminate any query arguments or hash strings from the end of the name.
// These could happen because some smart modules try to help us version
// their Javascript files (get browsers reload them when we update the
// modules, even when the file name stays the same). Since Javascript
// aggregator users know they need to clear their JS cache on update,
// they will solve this issue manually.
$scripts_js_file = preg_replace('!(.+)([\?#].*)!', '\1', $scripts_js_file);
if (file_exists($scripts_js_file)) {
$data = file_get_contents($scripts_js_file);
$contents .= ";\n/* AGGREGATED JS FILE: $scripts_js_file */\n".$data."\n";
}
?>
the first two lines need be removed (as was is the original patch posted in #2.
Patch attached.