Following #1428524: Replace all $.each() with filtered for loop, we are to replace all jQuery.each() calls with filtered for loops. This is to be appllied to all non-jQuery objects iterations.

Because for is much faster and $.each() changes the loop scope and doesn't filter properties which causes bugs (in autocomplete for example).

Related

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Also be sure to look for this.each() when jQuery is 'this'.

RobLoach’s picture

Issue tags: +JavaScript
RobLoach’s picture

Status: Active » Needs review
FileSize
2.04 KB

Untested.

nod_’s picture

That's how real men do it.

I tested, worked for me, except that I can't seem to find a page where menu.admin.js is added (so yeah, field_ui.js worked).

nod_’s picture

Issue tags: -JavaScript

#3: for.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript

The last submitted patch, for.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

I was expecting a fail to apply error but not random testbot failure. I guess it doesn't need a reroll then.

Jelle_S’s picture

FileSize
2.12 KB

Problem was that in menu.admin.js the options variable is an object, not an array, changed to for loop to a for in loop.

nod_’s picture

very minor thing, is it possible to replace the variable name i to value?

(edit) and more importantly, the jquery selector needs to be out of the loop that's pretty doggy like that.

Jelle_S’s picture

FileSize
2.46 KB
+++ b/core/modules/menu/menu.admin.jsundefined
@@ -35,11 +35,13 @@ Drupal.menu_update_parent_list = function () {
+          $('fieldset#edit-menu #edit-menu-parent').append(

You mean this selector right? That's the result of a shameless copy-paste and trying to do a quick-fix on replacing the loop ;-)
new patch attached

Jelle_S’s picture

+++ b/core/modules/menu/menu.admin.jsundefined
@@ -31,15 +31,18 @@ Drupal.menu_update_parent_list = function () {
+      var $selectfield = $('fieldset#edit-menu #edit-menu-parent');

Just on a side note: can't we change this to $('#edit-menu-parent') since HTML ids are supposed to be unique anyway?

And an other question, not necessarily related to this issue, but I've had it on my mind since a while now:
What is best practice:

$('.classone .classtwo .anotherclass');

or

$('.classone').find('.classtwo').find('.anotherclass');
Jelle_S’s picture

FileSize
2.61 KB

fixed one for that should have been a for in loop.

Also in field_ui.js line 86, there's a this.each() where this is a jQuery object. Should this be changed to a for loop?

I talked with @seutje about it, and we both seemed to think this was a valid use case to use the jQuery.each() function, but we would like some feedback on this (@nod_ ?)

nod_’s picture

Oh yeah we're not replacing the jQueryElement.each just the $.each.

For now :)

nod_’s picture

Status: Needs review » Needs work

if you rename value as index in menu.admin.js that's RTBC :)

seutje’s picture

the menu.admin.js changes were incorporated in #1751398: Selectors clean-up: menu module, along with some others, so I'm rerolling this one without that bit, so either won't break if the other goes in first.

seutje’s picture

Status: Needs work » Needs review

damn status

Jelle_S’s picture

I'd put it on RTBC if I hadn't worked on this patch as well :-( ;-)

In other words: Looks good to me...

nod_’s picture

#15: 1660952-jquery.each-15.patch queued for re-testing.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Still applies and still a proper fix.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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