jquery.dashboard.js - Fix bad variable scoping (Possible behavioral change)
authorTim Otten <totten@civicrm.org>
Fri, 13 Nov 2015 19:42:43 +0000 (11:42 -0800)
committerTim Otten <totten@civicrm.org>
Fri, 13 Nov 2015 19:42:43 +0000 (11:42 -0800)
These snippets were declaring new variables (eg `if (...) var ids = foo`) in
one scope then accessing them in a later scope - which properly triggers a
warning from jshint. In my mind, this would be a nullop (always leaving `ids`
undefined), although JS does allow values to leak through.

I *suspect* this leaking was *partially* intentional.  The variable (`ids`
or `id`) is clearly intended to be used a few lines later.  However, in the
case where the conditional is *false*, the content of `ids` or `id` is
unclear.  You might expect it to be `undefined` (and it probably would be in
the first few loop-iterations), but eventually you'd would start leaking
values across iterations, and that doesn't seem normal/intentional.

js/jquery/jquery.dashboard.js

index e2c2c71014f99ea19561663f8cdb5072aa5b866f..489345b89c4b60cb06960ddf2732ac17fd62584d 100644 (file)
       for (var c2 in dashboard.columns) {
 
         // IDs of the sortable elements in this column.
-        if( typeof dashboard.columns[c2] == 'object' ) var ids = dashboard.columns[c2].element.sortable('toArray');
+        var ids = (typeof dashboard.columns[c2] == 'object') ? dashboard.columns[c2].element.sortable('toArray') : undefined;
 
         // For each id...
         for (var w in ids) {
           // Chop 'widget-' off of the front so that we have the real widget id.
-          if( typeof ids[w] == 'string' ) var id = ids[w].substring('widget-'.length);
+          var id = (typeof ids[w] == 'string') ? ids[w].substring('widget-'.length) : undefined;
 
           // Add one flat property to the params object that will look like an array element to the PHP server.
           // Unfortunately jQuery doesn't do this for us.