Attached is a backport of the JS aggregation patch that went into Drupal 6.x.

This patch is for Drupal 5.1+ and has been tested, please post any feedback as needed.

NOTE: This will *never* go into Drupal 5.x so don't even ask :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Thanks!

I will test later today :)

drumm’s picture

Status: Needs review » Closed (won't fix)

Correct, this will not go into 5.x.

Morbus Iff’s picture

m3avrck - patch contains your settings.php config.

m3avrck’s picture

Status: Closed (won't fix) » Needs review
FileSize
31.86 KB

@drumm - I left this as needs review so the won't fix won't auto close this issue so it can't be found...

Doh! Updated patch with settings.php removed.

dkruglyak’s picture

Hmmm... Looks like the patch is against D5 CVS ???

It is pretty scary to put such a moving target in production. Would we have a patch against stable releases? D5.1 or D5.2?

When would D5.2 be available, anyways?

drumm’s picture

@m3avrck - patches marked need review show up in the issue tracker as patches which need review for 5.x. It would be nice to not have patches which won't go into 5.x show up. Issues marked as 'won't fix' are not auto-closed, that only happens with 'fixed' issues.

@dkruglyak - Drupal 5.x is kept as stable as possible. 6.0 will be the next feature release.

dkruglyak’s picture

@drumm: Well, there is still an issue of keeping current with a CVS version on a particular date, especially for huge patches like this one... D5.1 is a 4 month old release and I see most of the files patched for JS aggregator way later.

Any chance of getting D5.2 cut for reference purposes, before next major security flaw hits? Anyhow who is looking for these breaches :)

Bevan’s picture

Is it possible to port this to a module? Or integrate at theme level only? Or would it break if core is not modified?

drumm’s picture

Status: Needs review » Closed (won't fix)

Sending back to won't fix after discussion on the development list.

Since I do not think that running patched versions of Drupal in production is a good idea, I can not offer any further advice.

dkruglyak’s picture

This is not an answer. Patching core in production is something that is very hard to avoid.

The question is only how to support this in the best way. Creating stable reference points (e.g. D5.1) and organizing patches around them is the path of least pain. This is very hard to do when the last reference release was cut 4 months ago.

So I think we cannot get to D5.2 fast enough for these reasons.

arthurf’s picture

I patch D5.1 with no problems. I have a fairly heavy site in development that has lots of javascript, so this is very interesting to me. I've got things running, though I noticed some problems with how some javascript code is compressed:

This does not compress correctly- I get an error about a missing ';'

$(document).ready(function () {
	  $('.showcommunitytags').click( function() {
        witnessToggle($('#communitytags'), $('.upload_tags'));
      }
    );
 -- snip --

Changing this to one line:

$(document).ready(function () {
	  $('.showcommunitytags').click( function() { witnessToggle($('#communitytags'), $('.upload_tags'));}); 
--snip--

Works well. While I can write javascript as the above, it might be good to figure out what's going on here. Also this may impact the D6 one, not sure...

mfer’s picture

There are a number of optional ; in the javascript that need to be there for the the proprocessing.

mfer’s picture

Here is the patch rerolled for drupal 5.2

Goose4all’s picture

hmm, when i enable this patch, all collapsable stuff becomes no more collabsable..(?)

drupal 5.2 with jquery_update installed

bjaspan’s picture

I encountered this problem too. See http://drupal.org/node/119441#comment-319769.

bjaspan’s picture

Here is a new Drupal 5.2 version of this patch that fixes the "missing ; after statement" bug.

arthurf’s picture

I applied the previous version to 5.3 with success.

arthurf’s picture

I applied http://drupal.org/files/issues/js-aggregation-149402-16.patch to 5.3 with success, looks like it's still good.

bjaspan’s picture

There's another bug in this patch: If Drupal cannot connect to the database or otherwise needs to generate a "maintenance page", you get a call to an undefined function error instead of the maintenance page. To fix it, add

require_once './includes/file.inc';

to the list in drupal_maintenance_theme() in bootstrap.inc.

ardelio’s picture

A wonderful solution for better performance, but TinyMCE and collapsible items don’t work anymore with 5.2 and js-aggregation-149402-16.patch. We have javascript errors "Drupal is not defined", "TinyMCE is not defined", etc. as soon as the button "Optimize JavaScript Files" is selected. Without this option everything works fine.

- Does 6.0 patch version works correctly with TinyMCE?
- Any solution for 5.x?

Wim Leers’s picture

I'm afraid it's indeed not working quite right... I keep getting boatloads of errors in Firefox. And JS simply stops working in Safari. Not good. It seems it's a good thing the compression feature was cut out of Drupal 6. Aggregation only will do for now.

I may re-roll this patch to remove the compression.

Christefano-oldaccount’s picture

I'm with Bevan's comment at #8. Is it possible to use this as a utility module without modifying core?

mfer’s picture

@christefano - I don't think it's possible to do this as a utility module without modifying core. The javascript isn't accessible to modules to alter. I'd hope this would change in D7 (a patch got posted but to late for D6).

For D6 on high performance sites I'm going to end up using javascript compression manually on the files. Then the compressed files be aggregated. It would be nice in D7 if we can have some kind of plugin architecture to allow a choice of something like the YUICompressor or Dean Edwards Packer.

ardelio’s picture

Wim,
you say at http://drupal.org/project/cdn
"For those who know of the infamous YSlow test: if you install and configure this module and apply the core patch that also adds Javascript aggregation, you will score 98. Almost the maximum! The remainder of points is due to the lack Javascript minification (compression)."

Where can we get the 5.x patch for JavaScript aggregation only (without minification)?

Thanks

Wim Leers’s picture

ardelio, that's about the CDN module. The CDN module needs a core patch that conflicts with the JS aggregation patch, since they affect the same lines in some cases. I still have to reroll the patch to only include the JS aggregation.

ardelio’s picture

Wim,

1) it’s clear for the JS aggregation. We’ll wait for this new patch.

2) I have a question that is slightly off-topic, bus as it was posted elsewhere without any answer, and as you seem to deal with these CSS/JS caching/CDN optimizations, I would like to know if you can answer it:
--
What are the best practices for cache/CSS optimization management in Drupal 5.x?
We keep normal cache enabled, CSS optimization enabled, and JavaScript aggregation disabled (as we have many issues with JS behavior when enabled). The problem is that the sites are visually broken every time we change something in single CSS files (and remove aggregated .css files from /files/css). The client requests the old aggregated CSS file whose name is included in the code of the cached page (local/proxy/Drupal cache?). As that file doesn't exist anymore on the server, the client loads the page without CSS. The issue seems to be very important for a multi-site Drupal setup. It seems that the whole /files/css folder is purged every time you enable a new module (internally calling cache/css clear) or when you change Site Configuration/Performance/Optimize CSS files option on any site.
Any thoughts about best practices and this particular "site visually broken" problem?
---

In summary:
a) if we use Drupal provided CSS optimization, it seems to pull into the consolidated file only the .css files needed for that page (which is good), but there are some disruptions when we enable other modules in that or other sites, or when we change the original css files, as we need to purge the old consolidated files in /files/css. At that time, the client display is broken as they request the old consolidated file that is not on the server anymore.
b) if we turn off Drupal CSS optimization, and minify by ourselves a large css file which consolidates all css files from the used modules, it seems that the resulting file is larger than in case a).

Which solution would you use and how? I’m not a Drupal expert and would like to hear the experts’ opinions about the best practices.

Thanks

Wim Leers’s picture

I haven't used multi site installs yet. But the solution seems trivial.
Your clients can't find the CSS file because they're using cached webpages. So just make sure the CSS files are cached too...

dkruglyak’s picture

Title: Drupal 5.1 backport of JS aggregation patch » [Drupal 5.5] Patch for JS aggregation with static file base
FileSize
40.2 KB

I have been trying to implement the new patch for offloading static file content to a separate server:
http://www.lullabot.com/articles/using-lighttpd-static-file-server

Apparently it conflicts with JS aggregation, so I merged the two and updated them to 5.5. This should work for everyone out of the box, as static base path does not break anything if unused. If you turn it on however you can reduce your Apache load.

I am not sure about this, but perhaps the static base path could be redirected to a CDN - resolving the questions above.

Christefano-oldaccount’s picture

For those who can't wait, derjochenmeyer has released a JavaScript Aggregation utility module that doesn't require patches to core.

dkruglyak’s picture

Patch updated once again to be compatible with CDN integration modules. Now every time static content is requested a new function static_file is called, which resolves depending on the configuration:

$conf = array(
  ...
  'static_file_base' => 'http://static.example.com/', // Using local static file server
  ...
);

If static base is set, this would be the constant base path, most likely served by local server such as lighttpd. To learn about Lighttpd configuration see here: http://www.lullabot.com/articles/using-lighttpd-static-file-server

$conf = array(
  ...
  'static_file_func' => 'cdn_file', // Use CDN Integration module
  ...
);

If static function is set, it would be called to resolve the location of the file. The immediate use case is with CDN Integration module (http://drupal.org/project/cdn), but any custom caching strategy could be implemented.

If neither of the above settings exist the default base is used.

kbahey’s picture

For static file serving, can we change base_path() to be what it is now, as well as static file functionality too, instead of having two functions?

dkruglyak’s picture

The patch degrades gracefully to base_path if config settings are absent.

The configuration does not really have two functions. There is merely a choice between using a fixed static base OR calculating the static URL dynamically using a custom function. The former is good for local static server the latter for CDNs.

The patch does not break anything and provides the simplest possible API with static_file function. Of course implementation and config settings for this functions are up for debate.

Wim Leers’s picture

1) JS compression has to be removed. It's problematic, or at least E. Deans' packer is.

2) I think this issue should be used for a JS aggregation patch *only*, instead of including the changes for static file serving. Alternatively, provide both the patch with JS aggregation only and the one with static file serving added.

dkruglyak’s picture

Correct, there needs to be a separate patch/issue for static file serving without JS aggregation. However since the aggregation and static file serving code conflict, this issue has the combined patch.

I filed a core issue for D6 (http://drupal.org/node/212369), hopefully it can still get into the main release. As for D5, perhaps the right place to discuss and distribute this patch is CDN module itself. CDN module can distribute this patch with JS aggregation as well.

The key point right now is to finalize the API, which is to have a static_file function defined in core and used to request static files. Behavior of the function to be driven by variables to use a fixed base or a CDN.

Wim Leers’s picture

Well, this obviously won't make it into D5. So you'll have to create a D6 patch that adds *only* your suggested static_file() function, if you want to get it into D6 core (though chances are slim that it'll make it in, no matter how simple it is).

By the way, the CDN integration module already includes the two patches you suggested ;) :)

kbahey’s picture

dkruglyak, you did not get what I am trying to say. If we want static file caching, then why create a new function. Just bury the complexity inside base_bath() and make it check for the two variables in settings.php, and act accordingly: if it finds them, it does the new stuff, if they are not set, then it behaves like it does today. No need for a new function.

I suggest you take the JS aggregation out and make it a purely static file caching patch in the other issue you created (which has no patch, but points to this one). This will make it easy to review, and will have a better chance to get in.

Oh, and it should be a 7.x thing too.

Wim Leers’s picture

kbahey: base_path() accepts no parameters and returns, well, the base path. Integration with a CDN requires the path to the local file to be passed in, so a completely new URL (with a different base path *and* different filename) can be generated. So keeping base_path() as is won't work. If we add the parameters, the name won't be accurate anymore.

And I agree: this should go against D7. D6 has been delayed enough as it is.

kbahey’s picture

The present way is:

$x = base_path() . $file;

Your proposed way:

static_file($file);

My proposed way:

base_path($file);

And base_path() would internally check the variables and do what static_file() does today.

If no separate server for static files, then the base path AND the file is returned, e.g. "/drupal/" . $file if you installed in a subdirectory, or "/" . $file for the most common case.

If there is a separate server for static files, then "http://static.example.com/foo/bar/baz/".$file is returned.

Yes, it changes the behavior, but it is like we do with any API.

I am not hung up on the name base_path() even. It can be static_base() or something else.

Wim Leers’s picture

Aha. Just a communication issue then, because that's exactly what I meant too. Since you're rewriting a file URL, and no longer just prepending a base path (either from the dynamic of static file server), I think file_url() is more appropriate.

You should also have the ability to specify *multiple* servers. I.e. (taken from http://drupal.org/node/212376#comment-699941):

  • File exists on CDN? Serve from CDN and stop, else continue
  • File exists on static file server? Serve from static file server and stop, else continue
  • Serve from Drupal's web server

So you'd have something like this in your settings.php:

$conf = array(
'file_url_rewrite' => array(
'cdn_file_url', // Preferred server at the top.
'static_file_url',
'drupal_file_url',
),
);

dkruglyak’s picture

@kbahey: base_path is a misleading name. The implication is that it returns what it says - the base path to the main site. That is why there needs to be a separate function to resolve static file urls (which is why I called it static_file), subject to relevant config settings.

@Wim Leers: Actually, I picked static_file as imitation of your cdn_file. I do not think that any "rewrite" terminology is accurate. We are not rewriting anything here, just resolving to a different base, sometimes dynamically. I am fine with finessing the invocation logic and configs, as you suggest however the changes need to fit a few criteria:

1) Support local static base if no CDN is defined
2) Support configurable invokation of CDN by function name
3) Help CDN function handle the fallback on local static content server
4) Not confuse CDN specific configs (like in your module) with configuration of static file url resolution

I have been waiting on making a standalone patch for D6 issue, till we have chance to review the design. In D5 I need the patch now and here I am only attaching what I am using. For D6 we have a chance to get it right in the core.

Wim Leers’s picture

dkruglyak: cdn_file and static_file make sense on their own. When you're going for a generic approach, it doesn't fit.

We're *not* just resolving to a different base. For Drupal's server and static file servers that's true, but not for CDNs. For CDNs, you really have to rewrite URLs.

1) Supported by my previous comment: if CDN isn't available, it will try the static file server. If that isn't available either, it'll use Drupal's server.
2) Supported by my previous comment.
3) Supported by my previous comment: that's exactly what it was about, I'm just avoiding implementation details.
4) Supported by my previous comment. This is actually completely unrelated to what we're discussing here, because it doesn't really matter where a function gets its settings from. I will keep on storing CDN settings in settings.php, at least for now.

To clarify, my previous comment was about this: the ability of this new functionality to check if a file is available on the preferred (i.e. fastest) server (typically: CDN). If it's not, it'll check if it's available on the second preferred server (typically: static file server). If it's not, serve from Drupal's server.
This is an example with 3 servers, but it can really be any number, of course.

dkruglyak’s picture

Stile file_url is confusing as it implies just about any file. This function is only relevant for static content, so static_file_url would be better. As for "rewriting" terminology, I would prefer "resolving" to better describe non-CDN cases that have nothing to do with rewriting. Maybe even the core function should be named something like resolve_file_url?

As to #4, I am just saying core functions and configs should not depend on any particular CDN module implementation. As for the rest, I would be interested in reviewing your complete patch with whatever changes, so there is no confusion.

Wim Leers’s picture

FileSize
8.12 KB

dkruglyak: No, you're interpreting things the wrong way.

All served files are static content. Only the HTML of Drupal-generated pages is not static. So putting "static" in the function name is redundant. The reason Robert Douglass used "static_base" and "static_url" as variable names, is that he wanted to make clear he's serving *files* (which are always static) from a *static file server*. So "static" indicates the *server type*, not the *file type*.

*All* file_* functions work on static files, hence they don't include "static" in their filenames. So it would be much more consistent with the rest of Drupal's File API to use file_url as the function name.


Attached is the (intial) patch (this was quickly thrown together) that I'm currently using for the CDN module, which does no longer include a single reference to the CDN module, as you can see. And it works like a charm, too!

All you have to add to your settings.php is something like:

$conf = array(
  'file_url_rewrite' => array(
    'cdn_file_url',
  ),
);
RobRoy’s picture

Just for reference, I just found some JS that is broken by the compressor so this was actually worthwhile to take out in http://drupal.org/node/183940. I would recommend merging that patch into this one if anyone is interested in the D5 backport.

$.ui.intersect = function(oDrag, oDrop, toleranceMode) {
    if (!oDrop.offset)
      return false;
    var x1 = oDrag.rpos[0] - oDrag.options.cursorAt.left + oDrag.options.margins.left, x2 = x1 + oDrag.helperSize.width,
        y1 = oDrag.rpos[1] - oDrag.options.cursorAt.top + oDrag.options.margins.top, y2 = y1 + oDrag.helperSize.height;
    var l = oDrop.offset.left, r = l + oDrop.item.element.offsetWidth,
        t = oDrop.offset.top,  b = t + oDrop.item.element.offsetHeight;
    switch (toleranceMode) {
      case 'fit':
        return (   l < x1 && x2 < r
          && t < y1 && y2 < b);
        break;
      case 'intersect':
        return (   l < x1 + (oDrag.helperSize.width  / 2)        // Right Half
          &&     x2 - (oDrag.helperSize.width  / 2) < r    // Left Half
          && t < y1 + (oDrag.helperSize.height / 2)        // Bottom Half
          &&     y2 - (oDrag.helperSize.height / 2) < b ); // Top Half
        break;
      case 'pointer':
        return (   l < oDrag.rpos[0] && oDrag.rpos[0] < r
          && t < oDrag.rpos[1] && oDrag.rpos[1] < b);
        break;
      case 'touch':
        return (   (l < x1 && x1 < r && t < y1 && y1 < b)    // Top-Left Corner
          || (l < x1 && x1 < r && t < y2 && y2 < b)    // Bottom-Left Corner
          || (l < x2 && x2 < r && t < y1 && y1 < b)    // Top-Right Corner
          || (l < x2 && x2 < r && t < y2 && y2 < b) ); // Bottom-Right Corner
        break;
      default:
        return false;
        break;
    }
  };

It doesn't remove the comments correctly in the 'touch' and 'intersect' cases and results in...

$.ui.intersect=function(oDrag,oDrop,toleranceMode){if(!oDrop.offset)return false;var x1=oDrag.rpos[0]-oDrag.options.cursorAt.left+oDrag.options.margins.left,x2=x1+oDrag.helperSize.width,y1=oDrag.rpos[1]-oDrag.options.cursorAt.top+oDrag.options.margins.top,y2=y1+oDrag.helperSize.height;var l=oDrop.offset.left,r=l+oDrop.item.element.offsetWidth,t=oDrop.offset.top,b=t+oDrop.item.element.offsetHeight;switch(toleranceMode){case'fit':return(l<x1&&x2<r&&t<y1&&y2<b);break;case'intersect':return(l<x1+(oDrag.helperSize.width/ 2) //Right Half&&x2-(oDrag.helperSize.width/ 2) < r //Left Half&&t<y1+(oDrag.helperSize.height/ 2) //Bottom Half&&y2-(oDrag.helperSize.height/ 2) < b ); //Top Half break;case'pointer':return(l<oDrag.rpos[0]&&oDrag.rpos[0]<r&&t<oDrag.rpos[1]&&oDrag.rpos[1]<b);break;case'touch':return((l<x1&&x1<r&&t<y1&&y1<b)||(l<x1&&x1<r&&t<y2&&y2<b)||(l<x2&&x2<r&&t<y1&&y1<b)||(l<x2&&x2<r&&t<y2&&y2<b));break;default:return false;break;}
158};})($);;

which is broken.

dkruglyak’s picture

I think what we should do is:

1) Make packer into a separate patch (for D5 and D6) => to be optionally applied by those who want it
2) Keep testing and document ALL problem cases => we should have one unified list of all found compression issues
3) Keep fixing JavaScript everywhere to be packer-safe => in contrib modules, in core and in 3rd party jQuery plugins

In your test case, have you tried changing comment style to this: /* Right Half */ ?

RobRoy’s picture

I know I could remove those comments or move them above that return to get compression to work, but I'd just rather remove it altogether if it's barfing on valid STRICT JavaScript (I did remove it for my D5 backport but I've got other changes so posting a patch woulda been tough). I didn't agree we should remove compression just because of missing semi-colons, but I do agree that it was wise to remove it if it was barfing on valid JavaScript that was correct and strict, as this is AFAIK.

dkruglyak’s picture

Sure the compression should be removed from the core. But there should still be a patch for those who want to use it - with caveats.

starbow’s picture

@Wim #43: Wow, it would be fantastic to get some flexibility into file_create_url. It would make it much easier for my private_upload module to play nicely with other upload modules. But is there really any chance of getting it into core before Drupal 7?

dkruglyak’s picture

Tao, this change is quite straightforward. Further details of Wim's patches are filed for his CDN module: http://drupal.org/node/212376

There is no D6 patch yet, but I filed an issue here: http://drupal.org/node/212369.

Please help review / test and I think we could get it in.

Wim Leers’s picture

@starbow: not a chance, unfortunately. I asked Gábor, and while he agreed Drupal needs it, it's simply too late in the cycle. So it'll be a D7 feature for sure.
I will try to get it in HEAD as soon as HEAD reopens.

If you want to track this more closely, make suggestions and test the patch for D5, please see http://drupal.org/node/212376.

mfer’s picture

Anyone still using this patch on drupal 5.7?

RobRoy’s picture

I am, although I'm not using a different domain for the static_file()s yet, but the rest has been working fine.

mfer’s picture

Title: [Drupal 5.5] Patch for JS aggregation with static file base » [Drupal 5.7] Patch for JS aggregation with static file base
Version: 5.x-dev » 5.7
FileSize
25.77 KB

Here is a version of the patch for 5.7. It does not deal with the cdn or offsite files. For that check out the patch at http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/cdn/patches...

In this version packing has been removed.

Flying Drupalist’s picture

Hi, I want to speed up JS on my 5.7 site. Is this the best way to proceed about it? I'm not sure how to add the patch to my site. Is there a place I can find it as a module or can someone give me installation instructions for the .patch. Thanks.

geek-merlin’s picture

http://drupal.org/project/javascript_aggregator

dont use compression with this module, its broken.
but aggregation anyway does say 90% of the boost.

skizzo’s picture

Can someone please provide the js-aggregation patch for
Drupal 5.8? A dry run with patch #53 fails on Hunk #1.
One could use http://drupal.org/project/javascript_aggregator
but, as I understand it, it does not work with Private FS. Thank you.

SocialNicheGuru’s picture

do you need the patch anymore? It didn't seem like you did if you take a look at

http://drupal.org/project/javascript_aggregator

If there is a scenerio where I would need it, can someone let me know. I am working on drupal 5.8.

thanks, C

skizzo’s picture

I can install javascript_aggregator and instrument page.tpl.php,
but I can't enable it in admin/settings/performance: the option
is grayed out... due to Private File System, I believe. I thought
that this patch would make it work also with Private FS.
Thank you.

EvanDonovan’s picture

@activelyout: There is a scenario in which you would need it - in order to substitute for javascript_aggregator.module when you are using Lighttpd. The filemtime() function fails in javascript_aggregator.module when Lighttpd is activated.

If you know of a workaround, please let me know.