Problem/Motivation

If JS settings array contains numeric keys, there could be some unpleasant consequences, if drupal_array_merge_deep() is applied to it. For example:

  var_dump(drupal_array_merge_deep(array('key' => 'value'), array('key' => 'value')));
  // array(1) {
  //   'key' =>
  //   string(5) "value"
  // }
  var_dump(drupal_array_merge_deep(array('2' => 'value'), array('2' => 'value')));
  // array(2) {
  //   [0] =>
  //   string(5) "value"
  //   [1] =>
  //   string(5) "value"
  // }

And there is a code in webform_conditional_prepare_javascript() which can produce this, because $conditional['rgid'] is numeric.

I can't provide easy STR, because I've met this issue on really uncommon conditions.

Proposed resolution

Add text prefix when using the rule group identifier as a JS settings array key.

CommentFileSizeAuthor
#1 webform-numeric_keys_in_js_seetings-2424243-1.patch1.96 KBLeksat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Leksat’s picture

Leksat CreditAttribution: Leksat commented
Status: Active » Needs review
FileSize
webform-numeric_keys_in_js_seetings-2424243-1.patch1.96 KB

The patch prefixes keys with "rgid_".

Status: Needs review » Needs work

The last submitted patch, 1: webform-numeric_keys_in_js_seetings-2424243-1.patch, failed testing.

DanChadwick’s picture

DanChadwick CreditAttribution: DanChadwick commented

I looked at this briefly. I *think* you are suggesting that the call to drupal_array_merge_deep_array lies in core ajax.inc ajax_render():

  if (!empty($scripts['settings'])) {
    $settings = $scripts['settings'];
    array_unshift($commands, ajax_command_settings(drupal_array_merge_deep_array($settings['data']), TRUE));
  }

I haven't confirmed what you said, but I do see a comment in drupal_array_merge_deep that refers to renumbering numeric-keyed array elements. I couldn't reproduce the problem, and it seems that the jQuery settings in other areas have numeric keys.

So I guess the question is how do we reproduce this and how do we determine other areas that might be affected.

I think the issue with drupal_array_merge only applies when actually merging data, and I'm not sure when it would possible merge at the array depth where the indices are numeric.

Leksat’s picture

Leksat CreditAttribution: Leksat commented

You should be able to reproduce the JS error if you use webform in a cached panel display, but only on the latest code of the Panels 7.x-3.x branch.
Here is the code which does settings merge: http://cgit.drupalcode.org/panels/tree/includes/plugins.inc?id=baed7ee#n193
In my case this line merged the webform settings array with itself.

Leksat’s picture

Leksat CreditAttribution: Leksat commented
Status: Needs work » Needs review

Yes. I just tested it on the simplytest.me. The Drupal.settings.webform.conditionals had duplicated ruleGroups and sourceMap.

DanChadwick’s picture

DanChadwick CreditAttribution: DanChadwick commented

Thanks for digging deeper.

Yeaaah, maybe that's a bug in panels. It seem reasonable for a module to assume that the settings that it creates aren't modified. The intention of that code in panels is to merge a settings array with a newer version of itself. It would seem that it should use a version of the deep array merge that does not have the exception for numerically-indexed.

It might be better to fix the place that is breaking the settings, rather than fix lots of places (perhaps in modules other than webform) that are triggering the breaking code.

Discussion??

DanChadwick’s picture

DanChadwick CreditAttribution: DanChadwick commented
Status: Needs review » Needs work

Needs work if it's to be fixed in webform; needs to be moved to the panels queue if not.

DanChadwick’s picture

DanChadwick CreditAttribution: DanChadwick commented
Version: 7.x-4.x-dev » 8.x-4.x-dev
Status: Needs work » Fixed

While I continue to believe that merging another module's settings is an unreliable act, I also think that exposing webform's settings to this is undesirable. So I propose a belt-and-suspenders approach.

I used your patch, with the additional case of "stringifying" the rule id (rid). Thank you for the patch.

I also changed the variable name in the jQuery so that a future maintainer doesn't confuse the rgid itself with the stringified key.

Committed to 7.x-4.x and 8.x.

  • DanChadwick committed ef8dfb2 on 7.x-4.x 
    Issue #2424243 by Leksat, DanChadwick: Fixed numeric keys in Javascript...
  • DanChadwick committed 8f8e037 on 8.x-4.x
    Issue #2424243 by Leksat, DanChadwick: Fixed numeric keys in Javascript...
DanChadwick’s picture

DanChadwick CreditAttribution: DanChadwick commented
Version: 8.x-4.x-dev » 7.x-4.x-dev

Status: Fixed » Closed (fixed)

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