Needs review
Project:
SWFUpload
Version:
6.x-2.0-beta8
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
6 Aug 2010 at 13:35 UTC
Updated:
7 May 2014 at 13:00 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
matulis commentedWorks as expected in Firefox & IE.
Comment #2
patrickroma commentedSame here. Chrome and Opera doesn't work. Anybody knows where to start for a solution?
Comment #3
David Lesieur commentedWorks as expected in FF, IE, and Safari. Fails in Chrome and Opera.
Comment #4
skilip commentedAny JavaScript errors in Chrome?
Comment #5
David Lesieur commentedNo JavaScript errors reported by Chrome.
Unfortunately, I have not taken any time yet to investigate this bug.
Comment #6
radamiel commentedThis bug appears because Chrome & Opera sort the hashtable with images.
For example,
var arr = new Array();
arr[250] = 'image1';
arr[0] = 'image2';
arr[46] = 'image3';
for(i in arr) {
alert(i);
}
in Firefox, IE and Safari we get 250, 0, 46
in Chrome & Opera - 0, 46, 250
Comment #7
CyrilD-1 commentedHi,
I have the same issue.
Is it fixed with 6.x-2.0-beta8 version?
Regards.
Comment #8
sansui commentedDrags fine in Chrome but doesn't save the order. I am noticing on some sites that all instances of tabledrag are affected, and some sites they are not.
Comment #9
sansui commentedSome good background info on what seems to be the case, supported by what stager is reporting: http://blog.bmn.name/2008/09/google-chrome-array-eval-out-of-order/
Vote for the chrome issue here: http://code.google.com/p/v8/issues/detail?id=164
This seems to be the primary bug report thread at google, although several others exist as well. I'm not 100% on this, but this seems like the issue at hand with swfupload and chrome - although perhaps Stager could offer some input on this since he reported the issue with Chrome's sort vs. other browsers
Comment #10
roderikInstead of waiting for people to stop arguing about whether or not Chrome should be changed, or programmers should change their JavaScript... I decided to just change the JavaScript.
This patch makes swfupload_add_js() pass data in an array, rather than an object.
(Note that the code which gets back the data hasn't changed. The FormAPI/CCK code which calls swfupload_widget_value() apparently doesn't mind whether it gets an object or an array, and what the keys in this object/array are.)
swfupload_widget.js is -obviously- adjusted to work with an array too.
...and in the process of rewriting ref.updateStack(), I discovered that it was buggy to begin with:
- If you add an extra file to a collection of already-existing files, all existing data in the hidden input field would be overwritten with (several times) the data from the file that was just added
- ref.upload_stack_size was just... wrong.
Fix attached. Please check my code flow/syntax for sanity; I'm far from a JS guru.
Comment #11
roderik(oh - if your spacing in swfupload.module is a bit messed up and parts of the patch apply with offsets, that's because I have #1050506: prevent adding js setting twice (causing white screen on node preview) applied. Shouldn't influence anything else.)
Comment #12
sansui commentedThanks Roderik - I'll give your patch a test this weekend and report back.
Comment #13
sansui commentedPatch applied cleanly after applying first patch you linked.
Seems to work ok in IE8 and Firefox from what I can tell.
In Chrome I uploaded a very small number of images - 3 - uploaded but did not keep sort order
I uploaded another 12 or 13 images, and I received these two warnings:
At that point, I edited the node again, and uploaded one more image. After saving the node, the only image that remained was the new image I uploaded, and I received an array_merge warning for each of the images that disappeared.
The first upload seems to work fine, any subsequent save causes these errors
Comment #14
sansui commentedAlso something else interesting I noticed in Chrome when trying to create a new node with images.
I uploaded 3 images, then clicked save - forgot to enter the title for the node. So I enter the title, go to save, and notice that my swfupload area has no thumbnails and repeats the first filename for each image instead of the actual files
Comment #15
ducktape commentedI applied the patch from #10 (and #11, for good measure).
This fixed the problem for me in Chrome (in FF all still works fine, as before). I have tested with adding and deleting images multiple times, but I could not reproduce the problems mentioned in #13. I also did not get any error messages.
I have tried to reproduce the error Sansui mentioned in #14. I can confirm the disappearing of the thumbnails, but the filenames were ok for me. Also entering a title and submitting the node form had the expected result. This behaviour is not limited to Chrome though, I could reproduce this in Firefox as well (haven't tried any other).
Comment #16
sansui commentedWhat version of PHP are you running ducktape? Not sure if it matters, but I'm on 5.2.6
I will try a base drupal install with just swfupload and see if I can reproduce the errors I'm getting on this site when I have a chance.
Comment #17
sansui commentedI think I fixed my issue. I had to change line 51 in swfupload_widget.inc to typecast the array
to
Comment #18
sansui commentedI applied the patch line by line to a customized version of swfupload I have on another site (has custom rotation options in the js), as well as my stock install, and with the change mentioned in my last post, it also functions well in Chrome.
Tested both sites in IE8, Firefox, Chrome, so far no errors or issues with creating, reordering, and saving. The issue with no thumbnails after leaving out a required field doesn't seem to be a big issue, still saves fine.
Comment #19
SchwebDesign commentedsubscribing, glad to see i'm not the only one noticing this. i will likely attempt the patch soon as well.
Comment #20
roderik@Sansui: I will assume that the errors you saw, have already been fixed outside of swfupload... because
field_file_load()always returns an array.The patch from #10 has been extended:
1) it retains the thumbnails over previews;
2) it does not take lots of time/CPU anymore when a checkbox / edit link is clicked in a table row. ( Drupal.attachBehaviors() was called -apparently- unnecessarily.)
3) the 'edit' link has been changed from '
<a href="#">edit</a>' to '<a id="..." class="active">edit</a>' (.active purely added to evoke a default CSS rule). This was necessary so that the page wouldn't start jumping around when clicking the link, after making the 2nd change.4) #1242238: Display POST errors on widget initialization (I really didn't want to split that out.)
5) lots of commenting and some rearranging of code, in the process of getting to understand the whole thing.
Comment #21
designguru commentedI tried applying the patch in #20 with the aim of allowing users to save the order of imagefield uploads using the swfupload module in Chrome but the patch can't be applied for some reason; getting the following error msg via ssh at the sites/all/modules/swfupload directory:
Was this simply because I was patching from the wrong directory or...?
q./
Comment #22
roderikYou must be running a complete different version of the swfupload module, if you have no swfupload_widget.js. Are you sure you're running Drupal 6? :-)
If you are, then try 'patch < 875706-20.patch'. (First run 'patch --dry-run < 875706-20.patch' to try.)
I don't know 'git apply' well enough yet, to know it's details - and this patch wasn't made with git yet. (My next one will be...)
Comment #23
designguru commentedRoderik,
I just tried the dry run and am still running into errors with your patch not finding the right files, though I have them on the server (and yes, am running the latest version of the module and drupal 6). Here's what I got with the dry patch command you suggested:
Any ideas? This time the patch failure seems to be related to the module, looking at this yes, it does seem that my swfupload.module file is different from yours - I'm using 6.x-2.0-beta8 how about you?
q./
Comment #24
roderikCould you try to apply the patch from #1050506: prevent adding js setting twice (causing white screen on node preview) before the above one?
Not all too professional maybe... I'll get a git repo to clone, when I have time :)
Comment #25
roderikFixed another bug:
With editable descriptions, if your description contained a double quote - this would lead to a PHP fatal error on submit/preview.
The following small patch is in addition to #20. I'll fix the patch mess later.
Comment #26
biblos commentedIt will be great to have one patch to fix problem on clear module install.
Comment #27
pfournier commented@roderik: a user reported the same error as Sansui in #13. You said:
It may return FALSE if $file['fid'] is not found in the files table. I do not really understand why this would happens, but it happens.
Comment #28
roderikHit & run post. I'm only 'gitifying' my stack of patches.
@pfournier: thank you for this observation. I'll have a look at it when I have more time.
Below patch is #20 & #25 rolled into one. Nothing changed. It applies with "patch -p1" now, like all 'git diff's.
* The patch fails if #1050506: prevent adding js setting twice (causing white screen on node preview) is not applied.
(And I've never run this #875706 without #1050506 already applied. So you must apply it first (or reroll it yourself) ... I'm blackmailing you into testing / RTBC'ing #1050506; after that's done I'll consider uploading a patch that applies against the unpatched module ;-) )
* It applies with offset -15 in the .js file. This is because of #1242238: Display POST errors on widget initialization already being applied on my side. You don't need to apply that one in order to make things work.
Comment #29
jstnhffmn commentedI just tried #1050506 then #875706-28. I get the "warning: array_merge() [function.array-merge]: Argument #1 is not an array in /home/itravlr/subdomains/stage/sites/all/modules/swfupload/swfupload_widget.inc on line 60" error.
Also, after submitting the node edit form (without even touching the photos table) all the photos are gone. I don't know if this is because of the above error or not.
I have the 6.x-2.0-beta8 version of SWFUpload, and Drupal 6.26.
Ignore the whole thing about files disappearing. It never occurred to me that this is a result of live site content being copied to a development site. (The node points to non-existent files.)
Would it be suitable to change this line
to
or
I'm not very familiar with the rest of the code so I don't know what kind of values are expected in the first parameter of this array_merge call.
Note: field_file_load(0) will return array('fid' => 0, 'filepath' => '', 'filename' => '', 'filemime' => '', 'filesize' => 0).
Comment #30
jstnhffmn commentedAnyone else getting this error: "warning: htmlspecialchars() expects parameter 1 to be string, array given in /home/itravlr/subdomains/stage/includes/bootstrap.inc on line 860"? (This is within the Drupal defined function check_plain.)
I did make some minor changes to my custom code, but a search through my local code only shows calls to check_plain within the CCK and FileField modules. (ie. It's hard to believe that my minor changes are the cause of this error as they are just valid changes to a couple of unrelated queries.)
Comment #31
mauritsl commentedPatch from #20 cleared all my files after saving.
Another approach may be to add a 'weight' attribute, instead of rewriting the object to an array. This has less impact on the rest of the code. Patch applied uses this approach and applies to beta8.