This should eliminate the incompatibility with several modules, like TinyMCE i think, and things should go well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

derjochenmeyer’s picture

Version: 5.x-0.5 » 5.x-0.x-dev
Assigned: derjochenmeyer » Unassigned
Status: Needs review » Active

I posted an idea about that here: http://drupal.org/node/212113#comment-701965

there are some problems that could result from a list like that because it possibly changes the order of the included .js files.

derjochenmeyer’s picture

I started programming something like that, but i ran into problems with line breaks in the configurable text area. Does someone have a filename and linienumbers on how the core block module for example stores and processes the include/exclude paths list?

Ill have a look.

An option would be to include by default the .js files of modules with known problems.

Unfortunately i dont know how many people use or even downloaded this module. But the only known/reported incompatibility at the moment is TinyMCE ...

derjochenmeyer’s picture

Ok i tried to figure out how this could be done by looking at the block module. My idea would be this: Comments please :)

1. add a textarea to the admin/settings/performance page under JS Aggregator settings (full paths of .js files to exclude from aggregation have to be entered here)

2. store the value in the variables table since it will be always only a rather short string (using variable_get($name, $default) variable_set($name, $value))

3. explode this string using a regular expression (This is where im a bit lost. TODO: figure out how to savely find unix, windows linebreaks in that string)

4. exclude files from aggregation

Doesnt it all make that module more complicated than it should be? Does anybody really need/want that functionality?

Hoesh’s picture

Ah, sorry I just forgot to check my submitted issues.

1) I suggest relative paths from installation dir, or end matching JS paths.
This way something.js is enough to be specified, if there are no multiple something.js in the folder hierarchy.
Then, admin must provide a longer mask to match the end of the path.

2) Sounds well

3) $exclude_masks = preg_split('/\r?\n/', $exclude_list, -1, PREG_SPLIT_NO_EMPTY);
This will produce the array, but an array_walk($arr = explode("\n", $what), "trim") should work well.
See array_walk on php.net for details.
By the way, if your implementation works, I can patch the source to
compile the list into a larger regular expressesion. That solution cancels the check loop, only one preg_match
should return TRUE or FALSE wheter or not to aggregate that file. I will think on this.

4) Yepp, exclude them.

My use case: I have tons of modules installed, and admin role .module lets my admin user pages load dozens of js files, which is pretty slow.
Aggregating CSS & JS files made a huge boost over multiple requests. The only problem is that Firebug reports several JS errors
related to aggregation.

IMHO, this module is a must have performance tool, and limitations of use can be cancelled.

I hope someone else will join in with comments.

derjochenmeyer’s picture

duplicate

derjochenmeyer’s picture

Version: 5.x-0.x-dev » 5.x-0.5
Assigned: Unassigned » derjochenmeyer
Status: Active » Needs review
FileSize
4.61 KB

I added the requested list in this patch.

If somebody could test it please. :) Ill add it to the next release if the patch seems stable.

1. i decided against the relative paths. I thinks its straight forward to copy the paths from the HTML source and paste them in the textarea
2. done :)
3. i chose the first one because it seems simpler :)
4. works for me

@hoesh: thanks for the feedback and the suggestions!!!

I ALSO CHANGED THE VARIABLE NAMES. Now they are easier to identify in the variables array. But it should make no difference when updating?

chirale’s picture

Version: 5.x-0.x-dev » 5.x-0.5
Assigned: Unassigned » derjochenmeyer
Status: Active » Needs review
FileSize
7.1 KB

Patching (patch -p0 < javascript_aggregator.module_2.patch) doesn't work (1 hunk failed, perhaps because variables name changes) but when I manually modify the module according to the patch tinymce finally works!

Here's the patched version (5.x-0.5): I don't know if I miss something, please check it.

I hope derjochenmeyer workaround will be implemented quickly in the official release. :-)

derjochenmeyer’s picture

Status: Needs review » Fixed

added to new version! Thanks a lot!

Hoesh’s picture

Status: Fixed » Needs review
FileSize
6.96 KB

Cool, I like this, while got some buf fixes, optimization, feature improvement:

1) Simple concatenation failed in my case, where a files EOF was just right after
a closing brace '}', and the next file was started with a statement. This could
cause the TinyMCE problem too, but not sure. If the explained situation happens,
the left of the aggregated JS file was discarded with syntax error.

2) I've put a comment into aggregated file with information regarding what was
aggregated. Makes inspection of js to be excluded easier.

3) Created the mentioned pattern matching compiler. Right now if you jut type ".js"
in the exclusion list, then all .js files will be excluded. Well, this is not the way you
employ this feature generally, but useful, if you dont want to type the whole path.

4) Made aggregated file name generation file modification sensitive. included files',
and the module's .module file's modification time are counted into md5 now.
This makes caching reliable when updating any module including the aggregator.

As I dont have dif & patch on my Mac right now I just attach the modified file.
(I have used Chirale's version) Please someone create a patch against HEAD in CVS,
rewiew the code (small optimization to the mail staff happened), and test it.

Thanks,

Endre

derjochenmeyer’s picture

i posted in the wrong issue...

derjochenmeyer’s picture

Status: Needs review » Fixed

@hoesh: I'm opening a new issue for your featue request here since the original issue was fixed.

http://drupal.org/node/219005

Anonymous’s picture

Status: Fixed » Closed (fixed)

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