Ubercart

I did change the way the JS was working, so it may just be that you need to refresh on the checkout page in Firefox to clear out your browser cache of the old JS. Let me know if that doesn't solve the issue.

I cleared my cache and it works fine now.

It sounds like this could break the checkout process for returning customers on a live site. I wonder if critical javascript like this should either not be cached (through the drupal_add_js call) or, better yet, the file should be versioned when upgraded. (I think adding a version # to a querystring after the javascript filename is all that would be required, but I'm not sure if drupal_add_js will allow this or filter the querystring).

Yeah, interesting thoughts... I had considered that but wasn't sure how/if it should be handled. You can pass in a FALSE value to drupal_add_js() for $cache and it will append a query string w/ the timestamp to the filename.

It seems like this would be beneficial on any JS added to the checkout page, but what about elsewhere on the site?

It's a good question. I guess that anywhere the javascript is critical, not caching it should be considered. uc_order.js would seem to fall in that category. I'll have to think of any others.

But I wonder if a best practice would be to always add a version number query when adding javascript. This is a little different than passing FALSE to drupal_add_js() for $cache, because the javascript will be cached, until the querystring is changed. For example

drupal_add_js(drupal_get_path('module', 'uc_cart') .'/uc_cart.js');

would become

drupal_add_js(drupal_get_path('module', 'uc_cart') .'/uc_cart.js?version=1');

I just tested this and uc_cart.js was cached as normal, until the version # was incremented.

The downside to this method, as opposed to the $cache method, is that you have to remember to increment the version # to make it work. The upside is that you don't lose the performance benefit of caching.

Now, if there was some way to automatically use the bazaar build # in the querystring, that would be very slick. Maybe I'm overthinking this though...

I think you're onto something good here... This may have to wait for the next version, but what if we had our own drupal_add_js() wrapper that appended the query string automatically. The question would be where to store or how to calculate the query string. Having some automated way of doing it would be best, but we can't rely on the CVS keyword expansion for folks that checkout from Bazaar.

We just need it the wrapper so we don't have to update every place in the code where a particular JS file is added in.

Isn't this a general problem that affects all Drupal sites? If so, perhaps it might be best to push this forward as a Drupal solution, not just an Ubercart workaround. But since Drupal has a lot of inertia, it does make sense to get something into UC now while we wait...

I don't think you need the bazaar version number or anything in the query string, just use the timestamp of the .js file on disk. This will change when the .js changes. The added benefit is that if you edit the .js by hand to try out a bug fix or improvement, the browser cache will get invalidated.

I think changing drupal_add_js() to add a timestamp to the query string all the time (.js file timestamp by default when $cache=true, current timestamp when $cache=false) would address the problem for UC and Drupal in general?

The change is actually in drupal_get_js() (found in includes/common.inc), where:

would become:

I made this mod to Drupal core on my test machine (), and it seems to work like I expect.

Alrighty... one vote from TR for all the time. I'll add a +1 to it after thinking it over, too. I just don't see any reason why not. It will still allow JS to be cached and will solve all the updating problems. I'll see what it'll take to get this fix in on Monday.

Ok, upon closer inspection back in the office, I see that this fix to be comprehensive includes a core hack. No bueno.

I guess what we'll have to do in the meantime is make a wrapper for drupal_add_js() (ugh) that appends the file timestamp query string. In the meantime, would you be interested in making the issue/patch for Drupal at drupal.org? You'll have to make it for Drupal 7 and then contend that this is a bugfix that should be backported to Drupal 6. I'm willing to help with that or take it on if you don't have the time.

Here's my hunch for the wrapper function... seemed to work fine:

It looks like this wrapper function also helps to build the file path, so we could get rid of drupal_get_path('module', ...) everywhere. At first I thought this was a great feature, but then I wondered if it might complicate code maintenance more than it's worth.

If uc_add_js is implemented with the same parameters as drupal_add_js, then it's a simple find a replace to add it...and another find and replace if/when drupal_add_js is updated. I might vote for something like this:

Hmm... you have an undefined $path variable in there now, though. That has to come from somewhere, and it's only possible when what's being added is a file and not some inline JS.

I thought about doing a total wrapper, but I don't see any benefit to just using the proposed uc_add_js() for instances where files should be cached based on their last modification and then use drupal_add_js() for everything else - which in practice is usually inline JS.

Ooops, sorry about that... $path should have been $data. This should work:

I'm not sure I understand your second point, but I don't have a strong opinion about this anyway...I just thought I'd propose a solution that would make it easy to update code without thinking too much

Unless I misunderstood your wrapper, it would require finding each instance of drupal_add_js that we want to update, and rewritting it to re-arrange the parameters passed and use only the filename and not the full path. Then if drupal_add_js is updated, we'd probably want to go back in and undo it all. Not that it's that hard, but it requires more thought than a find and replace...and depending on the number of modules and contribs that get updated, it seems possible that something would get goofed up in the process.

Ahh, I see the point. That makes sense to me.

Right. That's what my evil grin was for This is the only time I've hacked core, and it's turning out great - I've been doing a lot of jQuery/JavaScript development recently and boy does it make a difference! Now I don't have to keep flushing the server/browser cache all the time when I make a simple change, yet those files I don't touch are still cached so the performance doesn't suffer.

I'll look into it and bring up the issue on drupal.org. D6 has made some changes in this area, adding JS aggregation/consolidation for instance, so this same fix might not be appropriate. In any case, I doubt it will be back ported to 5.x, since TPTB have officially said they're not even going to do that with the JS aggregation. But I will try to make sure the issue is resolved for D6 at least.

chadcrew, I modified your if statement slightly... can you verify that this is right?

Yes, good call. That was sloppy on my part. I should remember to have some coffee before writing code in the morning...

I have the latest versions and Drupal 5.7. but the Payment Method in the Checkout Process is messed up when I use Firefox but works with IE. See attachment. Anyone an idea what I have to do???

That's probably your CSS, either some you added or some from your theme. Try looking at the pane with one of the standard Drupal themes like bluemarine to see what it should look like. Then you can use Firebug to fix your theme CSS for that page so it displays correctly.

Alrighty folks... I've committed the uc_add_js() fix, so we shouldn't have any more stale JS problems. I noticed this was addressed somewhat in Drupal 6, but I believe it's faulty and may reopen that issue. Their workaround just doesn't make sense.

This is a fairly light change, but it spreads across several modules. Please test it to make sure everything functions properly.