Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Category: feature » task
Priority: Normal » Critical

Yes. Now that jQuery UI (thusfar, the only Drupal 6 module I'm aware of that depends on this module) is at RC1 and doesn't require some wack-ass version of jQuery, we can do this. I won't have time for the next couple days, but feel free to roll a patch.

And let's take care of #263325: Select compression level of jQuery while we're at it.

hass’s picture

See #256285: jquery.js: Fix CVS ID, remove SVN tags about the farbtastic bug... i don't know how to solve.

mfer’s picture

http://drupal.org/node/256285#comment-873083 has a tested patch against D6 dev. If this gets into D6.3 jquery update might now have to put out a D6 release anytime soon.

If you want the patch at the link has a fix for farbtastic.

mfer’s picture

Just so everyone has an update on the core issue at http://drupal.org/node/256285#comment-880257.

Should jquery_update release a jquery 1.2.6 update before core does for D6 (if core is going to do so at all)? If so, I'll roll a patch.

webchick’s picture

mfer: What I was thinking is we could include some logic to check the core version of jQuery, and if it's less than the version included with jQuery Update, override it. Else, do nothing.

mfer’s picture

We could load the core jquery file, parse the rev, and swap it if the rev is older than that one included in jquery_update. What format of jquery should be used (minified, packed, uncompressed)?

webchick’s picture

@mfer: Whatever version is included with core. I think it's packed?

mfb’s picture

I definitely prefer minified javascript (perhaps I should make some noise about this re: the core jQuery as well).

mfer’s picture

@mfb A lot of people who are hosting sites are on cheap hosts without gzip compression so packed is used in core. It makes sense considering drupals wide spread appeal. and isn't too terribly difficult to swap out for the rest of us.

Maybe a feature request for jquery_update should be to provide both minified and packed version and providing a preference to choose which one to use.

webchick’s picture

Sure, but for now, let's just get it ported, hmm? :)

mfb’s picture

I like the idea of including minified and adding an option.
Btw I didn't stop using packed just for performance reasons, it was first of all to fix problems with IE activation, see http://dev.jquery.com/ticket/930

mfer’s picture

@mfb - That bug has to do with how IE handles eval(). Only external scripts can put objects in a page and eval makes IE think it's not external but part of the page. It is kind of a pain.

@webchick - I'll create a patch against head tonight. No D6 branch?

webchick’s picture

@mfer: dww said at Drupalcon that you should work out of HEAD until such time as you need to branch (porting to 7.x, or creating a new swanky 6.x-2.x branch), so I've been trying to abide by that guideline.

And awesome!! Thanks so much!!

mfer’s picture

Status: Active » Needs review
FileSize
147.97 KB

This patch replaces jQuery with a packed 1.2.6, adds in farbtastic (detects and replaces if it's present), and only replaces jQuery (and farbtastic) if the version in core is less than that included in the module.

Currently, the module loads both jquery files and parses the version out of them. Would it be better to keep the version included in this module in a constant so it's one less file to parse?

mfer’s picture

Here is a second version of the patch. It stores the current version of jQuery included in the module in a constant.

Either one works for me.

mfer’s picture

Patch from #15 included this time.

mfer’s picture

FileSize
148.87 KB

Patch from #15 really included this time. I mean it.

webchick’s picture

Awesome, thank you!!!

I originally had a constant, but took it out because I didn't want to have to update the src every time I released a new version of this module. OTOH, that's quite an expensive parse each page load, so I'm fine with #15/#17.

Testing now. Assuming no issues, I'll commit this in a moment. Once I get jQuery UI working again, I'll roll new releases for both.

webchick’s picture

Cool. Patch applied fine and seems to be working.

My only complaint is that it kind of pains me to see things like this:

+      // If farbtastic.js is included create a record for the new file.
+      if (isset($scripts['module']['misc/farbtastic/farbtastic.js'])) {
+        $farbtastic_path = drupal_get_path('module', 'jquery_update') . '/farbtastic.js';
+        $with_farbtastic = array($farbtastic_path => $scripts['module']['misc/farbtastic/farbtastic.js']);
+      }

I'd much rather us do something more like:

$replace_path  = drupal_get_path('module', 'jquery_update') .'/replace_scripts';
$replace_scripts = file_scan_directory($replace_path, '*.js');
foreach ($replace_scripts as $script) {
  if ($key = in_array($scripts, $script->filename)) {
    // replace the old one with the new one.
  }
}

This would mean that changing from one version of jQuery to another would require a) editing a constant, and b) adding/deleting files to the scripts_replace directory. Currently, it's going to mean adding/removing stuff to the logic, which could introduce more bugs. :(

webchick’s picture

Status: Needs review » Needs work
mfer’s picture

This will need some tweaking...

  if ($key = in_array($scripts, $script->filename)) {
    // replace the old one with the new one.
  }

We are comparing paths (e.g., 'misc/farbtastic/farbtastic.js') with a file name (e.g., 'farbtastic.js'). Any thoughts how to make the needle something that will be in the $scripts haystack? I do like the idea of having one set of logic that works for all replaced files.

webchick’s picture

Ok, I spent some time brainstorming w/ josh_k about this in #drupal.

The challenge here performance-wise is that this bit of code _has_ to run on each page load. Because $scripts can and will be vastly different from page to page. ?q=node might load no scripts, while ?q=node/#/edit will load the teaser splitter, collapsible fieldsets, etc. and ?q=admin/build/themes/configure/garland will load farbtastic.js.

Parsing through all of the scripts array and doing a preg_replace for file names is going to get messy and time-consuming. And adding 5-6 lines of logic for each file (luckily, only one file needs replacing in this jQuery version, but it might be 4-6 or more in another) is going to be a maintenance nightmare.

So how about a middle ground, which is another hard-coded piece that needs to get changed each time, but without any logic: an array of files to find/replace in the scripts array. So something like:

function jquery_update_get_replacements() {
  return array(
    'misc/jquery.js' => 'jquery.js',
    'misc/farbtastic.js' => 'farbtastic.js',
    // Add more files here if required...
  );
}

...

if (!empty(jquery_update_replacements()) {
  foreach (jquery_update_replacements() as $path => $replace) {
    $scripts[$index][$path] =JQUERY_UPDATE_REPLACE_PATH . '/' . $replace;
  }
}

We could also set the replacements a variable that gets cleared on cache; this would get cached. but since it's only going to be a 3-4 index array most of the time, I think it's fine.

webchick’s picture

Status: Needs work » Needs review
FileSize
30.31 KB
8.65 KB
4.12 KB

Ok, let's try this. You'll need to create a folder called "replace" and move jquery.js and farbtastic.js into there.

webchick’s picture

Status: Needs review » Needs work

Heh. Well that doesn't seem to work at all. :) It's including the files correctly, but the JS is all broken. I think I need to do special things so jquery.js is included first.

One moment. ;D

webchick’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

Much better. :) See #23 for testing instructions.

hass’s picture

Why not simply checking for the Drupal version constant and if <=6.2 expect jQuery 1.2.3 and if 6.3 expect 1.2.6. This would be much speedier than parsing the files on every request... hopefully we get 1.2.6 into D63 :-)

webchick’s picture

Because eventually, this module will need to ship with jQuery 3.1 and that won't be in Drupal 6.anything.

hass’s picture

Hm... but parsing shouldn't be done in real time. Maybe once or cached per day... or use the original jQuery file names and parse the filenames with a regex. This should be save enough to get the version out and much faster.

webchick’s picture

Yeah, here's a patch that sets a variable in hook_flush_caches instead, so the versions will be compared only when the cache is cleared. One hard-coded thing that needs changing each release eliminated. w00t.

webchick’s picture

Now with 10% fewer typos! :P

mfer’s picture

This update adds the replace directory, pulls out the old jquery file (version 1.2.4a), creates the replace directory, put the files in the replace directory, and removes some of the left overs from my last patch that needed to be removed.

Overall it looks good and is working for me. Nice job webchick.

webchick’s picture

Status: Needs review » Fixed

Oh, awesome! I added a hook_uninstall() to keep the variable from hanging around, but otherwise this looks good.

Committed to HEAD. w00t! :) I'll roll a dev release for people to try, and roll an official one on Monday assuming no additional errors.

Thanks SO much for your help!!

webchick’s picture

dmitri pointed out that we should be using version_compare() rather than a straight-up < sign, and while I was in there I noticed some typos not taken care of, so I committed this as an additional small fix.

mfer’s picture

I tested the dev release and had no issues in both PHP4 and PHP5.

webchick’s picture

Cool, thanks!! I also tested this w/ the dev release of jQuery UI, and found no problems. So I think a release is ready to roll as soon as http://drupal.org/node/263325 is in.

mfer’s picture

FYI, jQuery 1.2.6 was just committed to D6 core. We should see it in D6.3 :)

Anonymous’s picture

Status: Fixed » Closed (fixed)

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