Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 :-)
Comment | File | Size | Author |
---|---|---|---|
#53 | js-aggregation-149402-53.patch | 25.77 KB | mfer |
#43 | d5_file_url_rewrite.patch | 8.12 KB | Wim Leers |
#30 | js_packer_with_static_files_5_5.patch | 39.5 KB | dkruglyak |
#28 | js_packer_plus_static_base_5_5.patch | 40.2 KB | dkruglyak |
#16 | js-aggregation-149402-16.patch | 33.05 KB | bjaspan |
Comments
Comment #1
Wim LeersThanks!
I will test later today :)
Comment #2
drummCorrect, this will not go into 5.x.
Comment #3
Morbus Iffm3avrck - patch contains your settings.php config.
Comment #4
m3avrck CreditAttribution: m3avrck commented@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.
Comment #5
dkruglyak CreditAttribution: dkruglyak commentedHmmm... 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?
Comment #6
drumm@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.
Comment #7
dkruglyak CreditAttribution: dkruglyak commented@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 :)
Comment #8
Bevan CreditAttribution: Bevan commentedIs it possible to port this to a module? Or integrate at theme level only? Or would it break if core is not modified?
Comment #9
drummSending 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.
Comment #10
dkruglyak CreditAttribution: dkruglyak commentedThis 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.
Comment #11
arthurf CreditAttribution: arthurf commentedI 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 ';'
Changing this to one line:
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...
Comment #12
mfer CreditAttribution: mfer commentedThere are a number of optional ; in the javascript that need to be there for the the proprocessing.
Comment #13
mfer CreditAttribution: mfer commentedHere is the patch rerolled for drupal 5.2
Comment #14
Goose4all CreditAttribution: Goose4all commentedhmm, when i enable this patch, all collapsable stuff becomes no more collabsable..(?)
drupal 5.2 with jquery_update installed
Comment #15
bjaspan CreditAttribution: bjaspan commentedI encountered this problem too. See http://drupal.org/node/119441#comment-319769.
Comment #16
bjaspan CreditAttribution: bjaspan commentedHere is a new Drupal 5.2 version of this patch that fixes the "missing ; after statement" bug.
Comment #17
arthurf CreditAttribution: arthurf commentedI applied the previous version to 5.3 with success.
Comment #18
arthurf CreditAttribution: arthurf commentedI applied http://drupal.org/files/issues/js-aggregation-149402-16.patch to 5.3 with success, looks like it's still good.
Comment #19
bjaspan CreditAttribution: bjaspan commentedThere'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.
Comment #20
ardelio CreditAttribution: ardelio commentedA 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?
Comment #21
Wim LeersI'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.
Comment #22
Christefano-oldaccount CreditAttribution: Christefano-oldaccount commentedI'm with Bevan's comment at #8. Is it possible to use this as a utility module without modifying core?
Comment #23
mfer CreditAttribution: mfer commented@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.
Comment #24
ardelio CreditAttribution: ardelio commentedWim,
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
Comment #25
Wim Leersardelio, 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.
Comment #26
ardelio CreditAttribution: ardelio commentedWim,
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
Comment #27
Wim LeersI 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...
Comment #28
dkruglyak CreditAttribution: dkruglyak commentedI 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.
Comment #29
Christefano-oldaccount CreditAttribution: Christefano-oldaccount commentedFor those who can't wait, derjochenmeyer has released a JavaScript Aggregation utility module that doesn't require patches to core.
Comment #30
dkruglyak CreditAttribution: dkruglyak commentedPatch 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: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
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.
Comment #31
kbahey CreditAttribution: kbahey commentedFor 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?
Comment #32
dkruglyak CreditAttribution: dkruglyak commentedThe 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.
Comment #33
Wim Leers1) 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.
Comment #34
dkruglyak CreditAttribution: dkruglyak commentedCorrect, 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.Comment #35
Wim LeersWell, 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 ;) :)
Comment #36
kbahey CreditAttribution: kbahey commenteddkruglyak, 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.
Comment #37
Wim Leerskbahey: 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.
Comment #38
kbahey CreditAttribution: kbahey commentedThe 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.
Comment #39
Wim LeersAha. 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):
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',
),
);
Comment #40
dkruglyak CreditAttribution: dkruglyak commented@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 itstatic_file
), subject to relevant config settings.@Wim Leers: Actually, I picked
static_file
as imitation of yourcdn_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.
Comment #41
Wim Leersdkruglyak: 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.
Comment #42
dkruglyak CreditAttribution: dkruglyak commentedStile
file_url
is confusing as it implies just about any file. This function is only relevant for static content, sostatic_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 likeresolve_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.
Comment #43
Wim Leersdkruglyak: 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:Comment #44
RobRoy CreditAttribution: RobRoy commentedJust 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.
It doesn't remove the comments correctly in the 'touch' and 'intersect' cases and results in...
which is broken.
Comment #45
dkruglyak CreditAttribution: dkruglyak commentedI 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 */ ?
Comment #46
RobRoy CreditAttribution: RobRoy commentedI 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.
Comment #47
dkruglyak CreditAttribution: dkruglyak commentedSure the compression should be removed from the core. But there should still be a patch for those who want to use it - with caveats.
Comment #48
starbow CreditAttribution: starbow commented@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?
Comment #49
dkruglyak CreditAttribution: dkruglyak commentedTao, 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.
Comment #50
Wim Leers@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.
Comment #51
mfer CreditAttribution: mfer commentedAnyone still using this patch on drupal 5.7?
Comment #52
RobRoy CreditAttribution: RobRoy commentedI am, although I'm not using a different domain for the static_file()s yet, but the rest has been working fine.
Comment #53
mfer CreditAttribution: mfer commentedHere 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.
Comment #54
Flying Drupalist CreditAttribution: Flying Drupalist commentedHi, 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.
Comment #55
geek-merlinhttp://drupal.org/project/javascript_aggregator
dont use compression with this module, its broken.
but aggregation anyway does say 90% of the boost.
Comment #56
skizzo CreditAttribution: skizzo commentedCan 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.
Comment #57
SocialNicheGuru CreditAttribution: SocialNicheGuru commenteddo 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
Comment #58
skizzo CreditAttribution: skizzo commentedI 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.
Comment #59
EvanDonovan CreditAttribution: EvanDonovan commented@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.