Updated: Comment #12
Problem/Motivation
This issue was opened because of performance considerations. $.each() is - depending on the browsers javascript engine - around 70% slower than a for loop (benchmark : http://jsperf.com/jquery-each-vs-for-loop). So changing the behavior will result in better frontend performance.
Since this patch touches Drupal.attachBehaviors that would result in a big impact for frontend performance.
$.each() also changes the loop scope and does not filter properties which causes bugs for example in autocomplete.
Proposed resolution
As a proposed resolution $.each() would be replaced by filtered for-loops.
Remaining tasks
none
User interface changes
none
API changes
none
Related Issues
- #1428524: Replace all $.each() with filtered for loop
- #1920888: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable
- #432962: Add option to disable password strength checking
Original report by [nod_]
This was fixed a year ago #1428524: Replace all $.each() with filtered for loop but a few slipped by me since then (please tag your issues with "JavaScript" properly if it has anything to do with a javascript file).
Reintroduced in:
#1920888: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable
#432962: Add option to disable password strength checking
And the views merge.
Patch soon.
Comment | File | Size | Author |
---|---|---|---|
#8 | core-js-reremove-each-2057371-8.patch | 5.67 KB | Jelle_S |
#3 | core-js-reremove-each-2057371-3.patch | 5.67 KB | phiit |
#3 | interdiff-2057371-1-3.txt | 693 bytes | phiit |
#1 | core-js-reremove-each-2057371-1.patch | 5.68 KB | nod_ |
Comments
Comment #1
nod_Comment #2
phiit CreditAttribution: phiit commentedReviewing the patch I found there are 70 instances of '.each(' found using grep before applying the patch. After applying I still found 64 instances of '.each('.
I used the grep on core/modules and the command was a bit of a hack to figure out the correct amount but here goes:
grep -c -r --include=*.js ".each(" ./ | cut -d ":" -f 2 | awk '{s+=$1} END {print s}'
First part prints each file and the amount of instances found, the second part cuts so that only the number remains and the last one equals together the amount.
Am I doing something wrong here? Should the patch only apply to certain components?
edit: Figured out the problem on IRC. Changes are in order as far as I can understand and all the instances of $.each are replaced with a for loop.
Comment #3
phiit CreditAttribution: phiit commentedPatched the extra newline.
Comment #4
nod_great stuff, thanks for looking though the patch :)
We still need some help reviewing the content translation admin interface with this patch.
Comment #5
phiit CreditAttribution: phiit commentedAfter discussing this issue on IRC, nod_ pointed out that this issue only concerns replacing .each() with this kind of structure: $.each(theArray, function () {});
This fix should be enough if the content translation admin interface is reviewed.
Comment #6
YesCT CreditAttribution: YesCT commentedtagging d8mi
Comment #7
star-szrTagging for reroll.
Comment #8
Jelle_SReroll, I'll check the content translation admin interface as well.
Comment #9
Jelle_SComment #10
Jelle_SEverything on admin/config/regional/content-language seems to work as expected.
Comment #11
seutje CreditAttribution: seutje commentedAlso tested the content-language page and it works as before.
Also did a search and all remaining instances of
$.each(
I could find were in vendor code...RTBCing
Comment #12
dasrecht CreditAttribution: dasrecht commentedworking on it : updating the summary
Comment #12.0
dasrecht CreditAttribution: dasrecht commentedupdated summary
Comment #13
nod_+1 on reroll too. Thanks :)
Comment #14
YesCT CreditAttribution: YesCT commentedlooks like manual testing was done in #10 and #11 for the content translation settings, but not the views or password checker.
@dasrecht is doing that now. :) at the sprint.
Comment #15
dasrecht CreditAttribution: dasrecht commentedtested, works as before
ok tested with a little help of nod_ and it works :-)
How to test : Create a View, Enable Ajax:on use a pager or exposed filters. The Javascript is ran when you visit the page.
Comment #16
dasrecht CreditAttribution: dasrecht commentedManual testing has been completed.
Comment #17
dasrecht CreditAttribution: dasrecht commentedMove it to unassigned.
Comment #18
webchickCommitted and pushed to 8.x. Thanks!
Comment #19.0
(not verified) CreditAttribution: commentedRemoved the Remaining Testing Task done in #10 and #11