This piece of code is pure evil:

<?php
 
// AHAH is not being nice to us and doesn't know the "other" button (that is,
  // either "Upload" or "Delete") yet. Which in turn causes it not to attach
  // AHAH behaviours after replacing the element. So we need to tell it first.
 
$javascript = drupal_add_js(NULL, NULL);
  if (isset(
$javascript['setting'])) {
   
$output .= '<script type="text/javascript">jQuery.extend(Drupal.settings, '. drupal_to_js(call_user_func_array('array_merge_recursive', $javascript['setting'])) .');</script>';
  }
?>

Suppose you're always adding a Drupal.settings.foo.bar = 1; setting in PHP, i.e. on the server side.
And on the client side, you're adding a Drupal.settings.foo.baz = 2; setting.

Then you upload a file using filefield (or an image through imagefield, because imagefield uses filefield). Because you're actually including all settings again, all of them will be overwritten! Drupal.settings.foo.baz will no longer exist.

You should only output the settings that you actually need to prevent problems in other modules, which is the Drupal.settings.ahah part. Hierarchical Select suffered from this: http://drupal.org/node/354445.

Comments

Mark Theunissen’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for this Wim, I'm using Hierarchical Select and Filefield (much to my nervousness due to the very alpha-ish state of these modules), but this patch definitely fixes the problem.

quicksketch’s picture

Status:Reviewed & tested by the community» Needs work

Excellent, I think this is the cause of #374204: a script on this page is causing Internet explorer to run slowly also. I'd prefer to have an even more solid solution though, as this approach might cause the same problems for a smaller amount of scenarios, since we'll still have the problem where the Drupal.settings.ahah settings are incorrectly merged.

We need a way to specifically add the "Remove" button into the object.

quicksketch’s picture

Status:Needs work» Needs review
StatusFileSize
new4.12 KB

Here's a revised patch that adds exactly the information FileField needs to operate properly. This should prevent any nasty side-effects caused by merging in unknown data into the settings array. It's a bit heavier, but there's not much we can do to get the JavaScript settings array in a more readable structure.

quicksketch’s picture

StatusFileSize
new4.08 KB

Seems like there's an additional process function that doesn't exist in FileField anywhere. Since the last patch was updating this #process property but the callback didn't even exist, it'd make sense to just remove it entirely.

Wim Leers’s picture

quicksketch: the bigger question is of course "how do we support this cleanly from within core", because clearly *many* AHAH-powered modules do something like this…
It's 3 AM here so please forgive me if I'm not thinking of something better, however wouldn't it be better if we'd not be importing/setting anything blindly in Drupal.settings and be smarter about it: only import/set the *new* settings?
The only alternative I can think of right now is having some sort of per-page/per-session Drupal.settings cache. But that seems extremely ugly.

quicksketch’s picture

Right, I agree, there's clearly a much bigger problem here, but we only have what we have in core for now. By the way, this problem is partially solved with my core patch #360081: Pass Settings Locally to JS Behaviors.

This new approach used above at least doesn't import anything "blindly", it specifically adds the two buttons used by FileField, so it'd be nearly impossible to encounter a conflict with other modules.

quicksketch’s picture

Title:Drupal.settings.ahah instead of Drupal.settings» Merge only needed FileField Drupal.settings
Status:Needs review» Fixed

I went ahead and committed my later patch to get this problem fixed. If we come up with a better solution later I'll be happy to replace it, but for now it'd be good just to stop FileField from borking every other Drupal.setting on uploads.

mudd’s picture

I have a question about upgrading from alpha7

I'm trying to get past a problem with uploading 3+ files at once (php runs amok), and because this above patch is against cvs Feb-6-09, I decided to try to upgrade to 6.x-3.x-dev.

Upgrading results in the following when I try to upload a file:

user warning: Unknown column 'audio_format' in 'field list' query: INSERT INTO filefield_meta (fid, width, height, duration, audio_format, audio_sample_rate, audio_channel_mode, audio_bitrate, audio_bitrate_mode) VALUES (56, 726, 669, 0, '', 0, '', 0, '') in /var/www/html/includes/common.inc on line 3422.

Well, since I already have many nodes with files in cck/filefield's then surely I can't just uninstall filefield's tables?? (e.g. dump filefield_meta).

How would I go about upgrading?

quicksketch’s picture

justinz, Thanks please open a new issue for this problem. I haven't changed anything that affects database schema or filefield_meta. You might try upgrading filefield.module and the .inc files, but not filefield_meta. But again, let's open a new issue to fix the upgrade in filefield_meta.

Status:Fixed» Closed (fixed)

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

dkane’s picture

Does anyone know, will this patch work with filefield version 3.0-rc1? I'm trying to get rid of the IE error "A Script on this Page is causing Internet Explorer to Run Slowly" and was led to this page by http://drupal.org/node/374204. I just upgraded my version of filefield in an attempt to fix the issue to no avail. Should I revert before applying the patch or will it work in the new version? Thanks!