Download & Extend

Breaks on paths with HTTP GET parameters

Project:Javascript Aggregator
Version:5.x-1.5
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

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.

AttachmentSize
js_aggregator_sane_exclusion.patch786 bytes

Comments

#1

You can just ignore/remove the GET parameter. The file will be aggregated and will work fine.

#2

Version:5.x-1.3» 5.x-1.5
Priority:normal» critical

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.

AttachmentSize
js_agg_query_truncate.patch 1.58 KB

#3

Title:Exclude script paths with HTTP GET parameters, helps Ubercart» Breaks on paths with HTTP GET parameters

Also, retitling for a hopefully better summary.

#4

Status:needs review» fixed

Thanks for the patch! http://drupal.org/cvs?commit=215436

#5

Status:fixed» closed (fixed)

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

#6

Status:closed (fixed)» needs review

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.

AttachmentSize
yh.patch 1.05 KB

#7

Status:needs review» fixed

Commited: http://drupal.org/cvs?commit=353776

Thanks!

#8

Status:fixed» closed (fixed)

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

nobody click here