Last modified: 2011-05-12 19:16:09 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T30931, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 28931 - Avoid "for ... in" loops in SemanticForms.js
Avoid "for ... in" loops in SemanticForms.js
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
SemanticForms (Other open bugs)
unspecified
PC Windows 7
: Unprioritized normal (vote)
: ---
Assigned To: Yaron Koren
: patch, patch-reviewed
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-11 11:27 UTC by Benjamin Langguth
Modified: 2011-05-12 19:16 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
This patch reverts for-in loops back to basic style. (3.58 KB, application/octet-stream)
2011-05-11 11:27 UTC, Benjamin Langguth
Details

Description Benjamin Langguth 2011-05-11 11:27:51 UTC
Created attachment 8528 [details]
This patch reverts for-in loops back to basic style.

The Semantic Forms extension uses "for ... in" loops for iterating through the indexes of an array. But "for ... in" actually iterates through the property names of an object.

If the array has additional properties (e.g. from PrototypeJS) like

a.name = 'MyArray';

the JS can break with "for ... in" loops.

More information could be found at http://proto-scripty.wikidot.com/prototype:tip-looping-through-arrays.

Therefore i recommend to revert "for ... in" loops back to the basic vanilla "for (;;)" style.

I attach a patch that applies these changes to /libs/SemanticForms.js
Comment 1 Yaron Koren 2011-05-11 16:27:59 UTC
Alright, that seemed like a reasonable suggestion, so I made those changes in SVN. Thanks for the patch.

I assume this is for use by SMW+ - out of curiosity, are you planning to switch from Prototype to jQuery at any point, or are you staying with Prototype?
Comment 2 Bawolff (Brian Wolff) 2011-05-12 00:47:58 UTC
Note, you don't necessarily have to not use for..in loops, you just need to use the hasOwnProperty method of Object to check. (see http://javascript.crockford.com/code.html#for%20statement )
Comment 3 Benjamin Langguth 2011-05-12 09:12:29 UTC
Thank you applying the patch.

Yes, the issue arised from the use of the PrototypeJS lib (in SMW+).
I don't know the current effort on transform all prototype scripts to jQuery but i'm going to ask our dev team about it. All i can say is that upcoming scripts are mostly implemented in jQuery now.

@Bawolff:
Yes, this recommendation would cover most cases since it skips prototype members of the object (e.g. added by libraries). But the following code would still not work as expected:

var a, index;
a = ['a', 'b', 'c'];
a.name = 'MyArray';
for (index in a)  // <= WRONG
{
    if (a.hasOwnProperty(index)) {
        alert(index + ' = ' + a[index]);
    }
}
Comment 4 Bawolff (Brian Wolff) 2011-05-12 19:16:09 UTC
(In reply to comment #3)
> 
> @Bawolff:
> Yes, this recommendation would cover most cases since it skips prototype
> members of the object (e.g. added by libraries). But the following code would
> still not work as expected:
> 
> var a, index;
> a = ['a', 'b', 'c'];
> a.name = 'MyArray';
> for (index in a)  // <= WRONG
> {
>     if (a.hasOwnProperty(index)) {
>         alert(index + ' = ' + a[index]);
>     }
> }

Well I suppose that varies with your definition of "expected". It would behave how I expect it would, as well as exactly the same as how such constructs behave in other languages like PHP. A basic for loop is probably always better when you just want to go through the numeric keys in order.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links