Download & Extend
Issues

tabledrag.js should not use for...in to iterate over an array

Posted by JacobSingh on May 17, 2010 at 11:16am
Project:Drupal core
Version:6.x-dev
Component:base system
Category:bug report
Priority:major
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

This is not showing up in core as an error, but if the Array prototype has been added to, this will cause the iterating code to run over properties (and methods) of the Array, as opposed to just the elements. It's bad form in JavaScript to use the for...in construct when working with arrays (should only be used for objects).

AttachmentSizeStatusTest resultOperations
tabledrag_iterator.patch3.44 KBIdlePASSED: [[SimpleTest]]: [MySQL] 20,340 pass(es).View details
Login or register to post comments

Comments

#1

Posted by JacobSingh on May 17, 2010 at 11:18am

Oops, snuck some extra changes in there.

AttachmentSizeStatusTest resultOperations
tabledrag_iterator.patch1.34 KBIdlePASSED: [[SimpleTest]]: [MySQL] 20,354 pass(es).View details

#2

Posted by ksenzee on May 17, 2010 at 9:55pm

A simpler fix is just to construct the for loop correctly.

AttachmentSizeStatusTest resultOperations
800968_tabledrag-for_2.patch551 bytesIdlePASSED: [[SimpleTest]]: [MySQL] 20,356 pass(es).View details

#3

Posted by ksenzee on May 17, 2010 at 10:37pm
Title:tabledrag.js should be using $().each instead of for...in when iterating over an array.» tabledrag.js should not use for...in to iterate over an array

Guess I should retitle the issue, too.

#4

Posted by Damien Tournoud on May 17, 2010 at 11:03pm
Status:needs review» reviewed & tested by the community

Yep. Better Javascript!

#5

Posted by Dries on May 18, 2010 at 6:46am
Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks.

#6

Posted by System Message on June 1, 2010 at 6:50am
Status:fixed» closed (fixed)

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

#7

Posted by Big Z on July 14, 2011 at 1:37pm
Version:7.x-dev» 6.x-dev
Status:closed (fixed)» patch (to be ported)

Could this please be back ported to the 6.x branch. Patch attached.

Many thanks!

AttachmentSizeStatusTest resultOperations
tabledrag-for-800968-7.patch.txt511 bytesIgnored: Check issue status.NoneNone

#8

Posted by ptaff on July 29, 2011 at 2:22pm

subscribe

#9

Posted by mstrelan on September 27, 2011 at 5:12am

+1 for backport to D6

#10

Posted by Albert Volkman on January 27, 2012 at 10:39pm
Status:patch (to be ported)» needs review

Re-posting patch from #7. No credit please!

AttachmentSizeStatusTest resultOperations
tabledrag_iterator-800968-d6-10.patch578 bytesIdlePASSED: [[SimpleTest]]: [MySQL] 190 pass(es).View details

#11

Posted by budda on February 4, 2012 at 4:20am
Component:block.module» base system
Priority:minor» major
Status:needs review» reviewed & tested by the community

Super, perfect fix for my ageing Drupal 6 site.
At present its preventing admin forms from working in Chrome so a bit more than a 'minor' issue I think.
The bug is also not just in the block.module - it also affects the taxonomy admin, webform field designer, CCK field editor etc.

#12

Posted by Gábor Hojtsy on April 16, 2012 at 1:18pm
Status:reviewed & tested by the community» fixed

Looks good. Thanks, committed, pushed.

#13

Posted by System Message on April 30, 2012 at 1:20pm
Status:fixed» closed (fixed)

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

#14

Posted by j0rd on May 11, 2012 at 7:33am

I usually check the patches before I update my Drupal and I ran across this issue and fix.

I would recommend taking it one step further and look at reducing the number of gratuitous .length calls with the hopes of improving the speed of tabledrag.js further. From my simple test (profiling patch included in issue), it could reduce the number of calls to .length from ~30,000+ to roughly 450 for a single drag.

Take a look
#1571814: tableDrag.js, reduce the number of calls of .length from 33478 to 455

nobody click here