I'm creating what is (to me) a fairly complex multi-page form with many conditional fields. After reordering some number of fields, I noticed that the selection list of Components in the Conditional Rules section doesn't show everything I expect to see. (In this particular case, it doesn't show anything after the first page break.)

I've been able to recreate the problem with a much smaller form, so it doesn't appear to be strictly related to the size of the form.

I've tried to track down the problem, and I've found some code that raises questions in my mind. I think the problem might be in the foreach loop within function webform_component_list in includes/webform.components.inc. 

  foreach ($components as $cid => $component) {
    if (!isset($feature) || webform_component_feature($component['type'], $feature)) {
      $prefix = '';
      $page_num = $component['page_num'];
      if ($indent && ($parent_count = count(webform_component_parent_keys($node, $component)) - 1)) {
        $prefix = str_repeat('-', $parent_count);
      }
      if ($optgroups && $component['type'] == 'pagebreak') {
        $page_names[$page_num] = $component['name'];
      }
      elseif ($optgroups && $page_num > 1) {
        $options[$page_num][$cid] = $prefix . $component['name'];
      }
      else {
        $options[$cid] = $prefix . $component['name'];
      }
    }
  }

In the elseif clause, you're assigning an element to $options with a key equal to $page_num, but in the else clause, the key is equal to $cid. If $options already has a key = 2 (because of a pagebreak), and then much later in the loop it encounters a field with $cid = 2 (because it was moved down from earlier in the form, isn't that going to overwrite the previously-assigned value?

I'm going to try to see if I can puzzle this out for myself - I'll let you know what I find. Also, if you want me to, I'll try to define a simple test-case that might demonstrate this.

I searched through other issues, and didn't find anything that I thought was related to this.

(And if you can't tell, yes, this is my first drupal-related issue. I think I've followed all the appropriate guidelines, but I'll apologize in advance if I've violated any protocol or convention.)

CommentFileSizeAuthor
#19 webform-test.jpg237.88 KBjmrivero
#19 webform-test-issue.jpg173.9 KBjmrivero
#11 selection_list_fix.patch1.24 KBquicksketch
#9 selection_list_fix.patch1.28 KBtskww

Comments

tskww’s picture

tskww commented

More information that I hope might be helpful:

It doesn't appear to be related to moving the pagebreaks around. I've been able to recreate it on a new form from scratch.

In trying to debug this, I added some print statements within the loop. (wish I knew a better way) In each conditional statement I added a drupal_set_message call with the location of the statement, and at the end of the loop I added a dsm($options) statement.

This "trace" on my test page shows that $options is looking correct to me until the second page break. The field after that doesn't get added correctly, and I also see the problem described in the issue titled "Conditional Rules Component Name Gibberish" - http://drupal.org/node/736044. (One of the existing fields has the first character of this new field appended to it rather than creating an entry of its own.)

Creating a test case:

- Create a webform

- Create the following fields in order:
Field 1
Field 2
PageBreak1
Field 3
Field 4
PageBreak2
Field 5
Field 6
PageBreak3
Field 7
Field 8
(All fields are generic textfields, no specific options selected)

If you now select "Edit" on any field after the second page break, you'll see the problem.

tskww’s picture

tskww commented

One final comment - I was able to get it to 'work' by changing line 775 from:

$options[$cid] = $prefix . $component['name'];

to:

$options[$page_num][$cid] = $prefix . $component['name'];

The indentations now work correctly and all the expected fields are displayed.

The downside is that the Component selection box now shows a '1' as the header for the first page. But beyond that, everything appears to be functional.

(Note, that's line 775 in Beta6. It looks like line 827 in the most current version in CVS.)

Ken

quicksketch’s picture

quicksketch
he/him
commented

Hm, interesting. That shouldn't make any difference, since CID is unique per webform.

tskww’s picture

tskww commented

Yes, CID is unique, but CID is not unique from page_num.

In other words, if I'm reading this, and my traces, correctly, this line will be executed first:

$options[$cid] = $prefix . $component['name'];

So lets say that if $cid = 2, the statement becomes:

$options[2] = $prefix . $component['name'];

$options[2] is now assigned a value.

Later on, when a page-break is encountered, $page_num becomes 2. So, a later iteration of that loop executes the line with a CID of 5:

$options[$page_num][$cid] = $prefix . $component['name'];

becomes:

$options[2][5] = $prefix . $component['name'];

Since $options[2] is already defined as a string, $options[2][5] refers to the 5th character of that string, not to an array element with an index of 5. So instead of adding an element to an array, a character in the existing string is overridden - which is what is causing the corruption in the selection list - and which is also why that entry doesn't show up.

quicksketch’s picture

quicksketch
he/him
commented

Ahh, nice one tskww!

wheels394’s picture

wheels394 commented

subscribing as I am having this same issue. Cannot see any other components past page 1 under conditional rules.

Thanks

tskww’s picture

tskww commented

The quick change given in comment #2 does provide a work-around, provided you don't have a problem with the front page being titled "1".

I've got two other ideas for solving this that I'm hopefully going to get to try this weekend and then offer them to the maintainer to see what he thinks.

The question will become what the maintainer's "vision" is for that selection box.

If the desire is for something like:

Front page
    Field 1
    Field 2
Second page
    Field 3
    Field 4

then my original idea is a suitable starting point. (I'm guessing that I still need to make the code follow all the appropriate standards and prepare a patch.)

OTOH, if the intent is something like:

Field 1
Field 2
Second page
    Field 3
    Field 4

which I _think_ might be the case, then a solution is a 3-line change that again I should clean up before offering.
The basic idea is something like the following:

  foreach ($components as $cid => $component) {
    if (!isset($feature) || webform_component_feature($component['type'], $feature)) {
      $prefix = '';
      $page_num = $component['page_num'];
      if ($indent && ($parent_count = count(webform_component_parent_keys($node, $compon$
        $prefix = str_repeat('-', $parent_count);
      }
      $page_index = "p".$page_num;
      if ($optgroups && $component['type'] == 'pagebreak') {
        $page_names[$page_index] = $component['name'];
      }
      elseif ($optgroups && $page_num > 1) {
        $options[$page_index][$cid] = $prefix . $component['name'];
      }
      else {
        $options[$cid] = $prefix . $component['name'];
      }
    }
  }

The key is to make sure that there's no conflict between the array elements used for the pages and those used for the CIDs.

quicksketch’s picture

quicksketch
he/him
commented

Thanks tskww for your suggestions. I think my preferred approach would be the second suggestion:

Field 1
Field 2
Second page
    Field 3
    Field 4

However if you wanted the first page to be labeled, you can add a page break as the first component in the form (I think). If not, we should make the first pagebbreak an optional thing, so that you could label the first page if you wanted.

tskww’s picture

tskww commented
StatusFileSize
new selection_list_fix.patch1.28 KB

quicksketch,

Patch respectfully submitted for your consideration. It was created against HEAD and not against Beta6. Please let me know if I need to re-do it.

Ken

quicksketch’s picture

quicksketch
he/him
commented
Status: Active » Needs review

I'm looking at this now.

This looks like it's also the cause of #736044: Conditional Rules Component Name Gibberish, which I've marked duplicate.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Fixed
StatusFileSize
new selection_list_fix.patch1.24 KB

Thanks tskww, this looks great! I changed your double quotes to single quotes since that's what we use throughout Drupal unless you're using variable interpolation. Otherwise identical. Committed to 3.x versions.

scott m. sanders’s picture

scott m. sanders commented

Subscribing

tskww’s picture

tskww commented

quicksketch,

Thank you for the feedback. I'll try to keep that in mind for next time (single vs double quotes).

Ken

Status: Fixed » Closed (fixed)

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

jmrivero’s picture

jmrivero commented
Version: 6.x-3.0-beta6 » 6.x-3.2

This bug is still there, I think that the patch cover some of the many diferent forms structures or "trees".
Im using fieldsets to contain almost all components:

Front page
 FieldSet1
    Field 1
    Field 2
Second page
 FieldSet2
    Field 3
    Field 4
 FieldSet3
    Field 5
    Field 6
...
jmrivero’s picture

jmrivero commented
Status: Closed (fixed) » Needs work
quicksketch’s picture

quicksketch
he/him
commented

jmrivero, You'll need to provide more information on how to reproduce this problem. Note that components that come after the component being edited are intentionally not displayed in the list.

jmrivero’s picture

jmrivero commented

The issue it self is the same as described for version 6.x-3.0-beta6, the items missed are before a page break and may have several components between them.

Scenario:

 Fieldset
   Select (Mandatory - 3 options)

Pagebreak (1 Fieldset for each option of the previous page)

 Fieldset (conditioned by previous page Select option)
   Select (Mandatory - X options)
 Fieldset (conditioned by previous page Select option)
   Select (Mandatory - X options)
 Fieldset (conditioned by previous page Select option)
   Select (Mandatory - X options)

Pagebreak (1 Fieldset for each option of the previous page)

 Fieldset (conditioned by previous page Select option)
   Select (Mandatory - X options)
 Fieldset (conditioned by previous page Select option)
   Select (Mandatory - X options)
 ...
 Fieldset-X (conditioned by previous page Select option)
   Select (Mandatory - X options)
 
Pagebreak 

   Select(unconditional)

Pagebreak

 Fieldset (conditioned by previous page Select option)
   Select
 Fieldset (conditioned by previous page Select option)
   Select
   Textfield
   Select
   Textfield

Pagebreak

 Fieldset (conditioned by previous page Select option)
   Select
   Textfield
   Grid
 Fieldset (conditioned by previous page Select option)
   Select
   Textfield
   Grid
   Select
   Grid

Im able to get the right components listed in the conditions if I move the Fieldset up and set it right after the page break. After saveing the form the list of components for the condition is generated right.

jmrivero’s picture

jmrivero commented
StatusFileSize
new webform-test-issue.jpg173.9 KB
new webform-test.jpg237.88 KB

Added two screen captures of a test form, in this case, "Información General 5" fieldset shows "4.Por favor indique cuál es su principal área de actuación." for the conditional component, being this inside "Información General 4" which is in the same page.
Moving it up, right after the page brake fix it.

Incresing the number of components may cause diferent behaviors, Currently im working on a form with 1 fieldset in page one, 7 fieldsets in page 2 and 32 in page 3 in a tree structure of conditions.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Postponed (maintainer needs more info)

The screenshots you've provide make it look like everything is operating the way it should. You're only ever shown components that are earlier in the form than the component you're editing.

Could you either:

A) Provide a simpler use-case (the absolute minimum to replicate the problem) or
B) Export a node with the problem using Node Export.

Then provide exact instructions on where the problem exists. You shouldn't need a node with any more than 5-10 components to demonstrate the problem.

jmrivero’s picture

jmrivero commented

Correct me if im wrong but i thought that the components that showld be show for the condition are those before the previous page break of a component. The screen shoots show that the fieldset "Información General 5" is showing "4 Por favor indique..." which is under fieldset "Información General 4", both in the same page.
Just noticed that this bug only arises when there is at least 2 fieldset in the previous page.

Here is the export for testing:

array(
  'nid' => '31302',
  'type' => 'webform',
  'language' => 'es',
  'uid' => '1',
  'status' => '1',
  'created' => '1286559886',
  'changed' => '1286902117',
  'comment' => '2',
  'promote' => '0',
  'moderate' => '0',
  'sticky' => '0',
  'tnid' => '0',
  'translate' => '0',
  'vid' => '31305',
  'revision_uid' => '1',
  'title' => 'Test',
  'body' => 'Test webform',
  'teaser' => 'Test webform',
  'log' => '',
  'revision_timestamp' => '1286902117',
  'format' => '1',
  'name' => 'admin',
  'picture' => '',
  'data' => 'a:6:{s:14:\"picture_delete\";s:0:\"\";s:14:\"picture_upload\";s:0:\"\";s:19:\"biblio_crossref_pid\";s:0:\"\";s:7:\"contact\";i:0;s:13:\"form_build_id\";s:37:\"form-96de5ddc94fc027ee8df346e3403e980\";s:9:\"swekey_id\";s:0:\"\";}',
  'path' => 'encuesta',
  'settings' => FALSE,
  'attributes' => FALSE,
  'print_display' => 1,
  'print_display_comment' => 0,
  'print_display_urllist' => 1,
  'print_mail_display' => 1,
  'print_mail_display_comment' => 0,
  'print_mail_display_urllist' => 1,
  'print_pdf_display' => 1,
  'print_pdf_display_comment' => 0,
  'print_pdf_display_urllist' => 1,
  'webform' => array(
    'nid' => '31302',
    'confirmation' => '',
    'teaser' => '0',
    'submit_text' => '',
    'submit_limit' => '-1',
    'submit_interval' => '-1',
    'additional_validate' => '',
    'additional_submit' => '',
    'confirmation_format' => '1',
    'submit_notice' => '1',
    'allow_draft' => '0',
    'redirect_url' => '',
    'roles' => array(
      '0' => '2',
    ),
    'emails' => array(),
    'components' => array(
      '4' => array(
        'nid' => '31302',
        'cid' => '4',
        'form_key' => 'fieldset1',
        'name' => 'Fieldset 1',
        'type' => 'fieldset',
        'value' => '',
        'extra' => array(
          'collapsible' => 0,
          'collapsed' => 0,
          'conditional_component' => '3',
          'conditional_operator' => '=',
          'conditional_values' => '1',
          'description' => '',
        ),
        'mandatory' => '0',
        'pid' => '0',
        'weight' => '3',
        'page_num' => 1,
      ),
      '13' => array(
        'nid' => '31302',
        'cid' => '13',
        'form_key' => 'select1',
        'name' => 'Select 1',
        'type' => 'select',
        'value' => '',
        'extra' => array(
          'items' => '1|Show Fieldset 3
2|Show Fieldset 4',
          'multiple' => 0,
          'aslist' => 0,
          'optrand' => 0,
          'conditional_component' => '3',
          'conditional_operator' => '=',
          'other_option' => NULL,
          'other_text' => 'Otro...',
          'description' => '',
          'custom_keys' => FALSE,
          'options_source' => '',
          'conditional_values' => '',
        ),
        'mandatory' => '1',
        'pid' => '4',
        'weight' => '2',
        'email' => 1,
        'page_num' => 1,
      ),
      '14' => array(
        'nid' => '31302',
        'cid' => '14',
        'form_key' => 'fieldset2',
        'name' => 'Fieldset 2',
        'type' => 'fieldset',
        'value' => '',
        'extra' => array(
          'collapsible' => 0,
          'collapsed' => 0,
          'conditional_component' => '3',
          'conditional_operator' => '=',
          'conditional_values' => '1',
          'description' => '',
        ),
        'mandatory' => '0',
        'pid' => '0',
        'weight' => '3',
        'page_num' => 1,
      ),
      '15' => array(
        'nid' => '31302',
        'cid' => '15',
        'form_key' => 'select2',
        'name' => 'Select 2',
        'type' => 'select',
        'value' => '',
        'extra' => array(
          'items' => '1|Foo
2|Bar',
          'multiple' => 0,
          'aslist' => 0,
          'optrand' => 0,
          'conditional_component' => '3',
          'conditional_operator' => '=',
          'other_option' => NULL,
          'other_text' => 'Otro...',
          'description' => '',
          'custom_keys' => FALSE,
          'options_source' => '',
          'conditional_values' => '',
        ),
        'mandatory' => '1',
        'pid' => '14',
        'weight' => '2',
        'email' => 1,
        'page_num' => 1,
      ),
      '8' => array(
        'nid' => '31302',
        'cid' => '8',
        'form_key' => 'pagebreak1',
        'name' => 'Pagebreak 1',
        'type' => 'pagebreak',
        'value' => '',
        'extra' => array(
          'conditional_component' => '13',
          'conditional_operator' => '=',
          'conditional_values' => '',
        ),
        'mandatory' => '0',
        'pid' => '0',
        'weight' => '5',
        'page_num' => 2,
      ),
      '11' => array(
        'nid' => '31302',
        'cid' => '11',
        'form_key' => 'fieldset3',
        'name' => 'Fieldset 3',
        'type' => 'fieldset',
        'value' => '',
        'extra' => array(
          'collapsible' => 0,
          'collapsed' => 0,
          'conditional_component' => '13',
          'conditional_operator' => '=',
          'conditional_values' => '1',
          'description' => '',
        ),
        'mandatory' => '0',
        'pid' => '0',
        'weight' => '6',
        'page_num' => 2,
      ),
      '12' => array(
        'nid' => '31302',
        'cid' => '12',
        'form_key' => 'select3',
        'name' => 'Select 3',
        'type' => 'select',
        'value' => '',
        'extra' => array(
          'items' => '1|foo',
          'multiple' => 0,
          'aslist' => 0,
          'optrand' => 0,
          'conditional_component' => '13',
          'conditional_operator' => '=',
          'other_option' => NULL,
          'other_text' => 'Otro...',
          'description' => '',
          'custom_keys' => FALSE,
          'options_source' => '',
          'conditional_values' => '',
        ),
        'mandatory' => '1',
        'pid' => '11',
        'weight' => '2',
        'email' => 1,
        'page_num' => 2,
      ),
      '9' => array(
        'nid' => '31302',
        'cid' => '9',
        'form_key' => 'fieldset4',
        'name' => 'Fieldset 4',
        'type' => 'fieldset',
        'value' => '',
        'extra' => array(
          'collapsible' => 0,
          'collapsed' => 0,
          'conditional_component' => '13',
          'conditional_operator' => '=',
          'conditional_values' => '2',
          'description' => '',
        ),
        'mandatory' => '0',
        'pid' => '0',
        'weight' => '7',
        'page_num' => 2,
      ),
      '10' => array(
        'nid' => '31302',
        'cid' => '10',
        'form_key' => 'select4',
        'name' => 'Select 4',
        'type' => 'select',
        'value' => '',
        'extra' => array(
          'items' => '1|Bar',
          'multiple' => 0,
          'aslist' => 0,
          'optrand' => 0,
          'conditional_component' => '13',
          'conditional_operator' => '=',
          'other_option' => NULL,
          'other_text' => 'Otro...',
          'description' => '',
          'custom_keys' => FALSE,
          'options_source' => '',
          'conditional_values' => '',
        ),
        'mandatory' => '1',
        'pid' => '9',
        'weight' => '2',
        'email' => 1,
        'page_num' => 2,
      ),
    ),
  ),
  'last_comment_timestamp' => '1286559886',
  'last_comment_name' => NULL,
  'comment_count' => '0',
  'taxonomy' => array(),
  'files' => array(),
  'webfm_files' => array(),
  'menu' => array(
    'link_title' => '',
    'mlid' => 0,
    'plid' => 0,
    'menu_name' => 'navigation',
    'weight' => 0,
    'options' => array(),
    'module' => 'menu',
    'expanded' => 0,
    'hidden' => 0,
    'has_children' => 0,
    'customized' => 0,
    'parent_depth_limit' => 8,
  ),
  'export_display' => '$display = new ;
$display->api_version = 1;
$display->layout = \'\';
$display->layout_settings = \'\';
$display->panel_settings = \'\';
$display->cache = \'\';
$display->title = \'\';
$display->content = array();
$display->panels = array();
$display->hide_title = PANELS_TITLE_FIXED;
$display->title_pane = \'0\';
',
  '#_export_node_encode_object' => '1',
)
quicksketch’s picture

quicksketch
he/him
commented

The screen shoots show that the fieldset "Información General 5" is showing "4 Por favor indique..." which is under fieldset "Información General 4", both in the same page.

That is true, it should only show you items from the previous page and earlier. I'll take a look at your export and see if I can duplicate it.

tskww’s picture

tskww commented

In the "for what it's worth" category, I'd like to add that I can duplicate the problem.

Ken

tskww’s picture

tskww commented

I think I've narrowed it down some - the problem is somewhere in the general vicinity of lines 455-479 in webform.components.inc.

(As a side note, my opinion is that this is probably a completely different problem from the one started on this thread. It's not my call, but it might be worth creating a separate issue for this.)

What I see so far is that the problem occurs in this loop:

  foreach ($node->webform['components'] as $cid => $test_component) {
    // Only components before the pagebreak can be considered.
    if ($test_component['type'] == 'pagebreak') {
      $last_pagebreak_slice = $counter;
    }
    if (isset($component['cid']) && $cid == $component['cid']) {
      break;
    }
    if (webform_component_feature($test_component['type'], 'conditional')) {
      $conditional_components[$cid] = $test_component;
    }
    $counter++;
  }

The array $conditional_components is being built with every element that should appear in the selection box. But fieldsets are being skipped - they don't get added. However, $counter gets added every time through the loop, so $last_pagebreak_slice actually ends up pointing to something after the last pagebreak.

When the $conditional_components gets filtered by the line:

  $conditional_components = array_slice($conditional_components, 0, $last_pagebreak_slice, TRUE);

you then end up with more elements than intended.

I don't know what the proper solution is for this yet. I'll see if I can figure it out and (hopefully) prepare a patch.

Changing this part:

    if (webform_component_feature($test_component['type'], 'conditional')) {
      $conditional_components[$cid] = $test_component;
    }
    $counter++;

to this:

    if (webform_component_feature($test_component['type'], 'conditional')) {
      $conditional_components[$cid] = $test_component;
      $counter++;
    }

superficially appears to resolve the problem, and since $counter isn't used anywhere else, it seems like it might be ok - but someone who knows this code better than me should definitely weigh in on this.

At this point I'm going to step back and wait for someone more skilled / knowledgeable to comment.

Ken

jmrivero’s picture

jmrivero commented

Tested it with the exported node and with a large webform full of conditional fields and it fixed the issue.
I tried to see if this change affects some other funcionality but it looks good while creating, editing or moving components.
Checking the code couldnt see nothing wrong, but it may be that my brain doesnt work early in the morning.
Good work Ken ;)

adx’s picture

adx commented
Title: Missing options in Component selection list in the Conditional Rules fieldset. » Above change still didn't fix it..
Version: 6.x-3.2 » 6.x-3.4

I tried the change to '$counter++' above and that still didn't solve my problem; with the existing form and trying to create a new one.

I'm concerned about one thing though - when I updated the webform module just before this (previously at 3.2), I deleted the webform folder from modules, uploaded the new one and ran update.php. Upon running update.php it didn't list any changes to webform (had 'no updates available' in dropdown), but to some others it did. Then I realised I hadn't disabled the modules before updating them.

Now however in the Drupal update page it lists webform as the latest version. I checked update.php again and the drop down for Webform lists '6322' as the most recent in the list. Does that sound right or could this be my problem?

adx’s picture

adx commented
Title: Above change still didn't fix it.. » Missing options in Component selection list in the Conditional Rules fieldset.

Oh and I checked my webform.info and it does say version = "6.x-3.4". Duno if that means much considering though..

quicksketch’s picture

quicksketch
he/him
commented

Update.php is only needed when database changes are needed. Many updates will require that you update the module code but update.php is not necessary. The version in the drop-down is the *schema* version (basically keeping track of the database state), which is completely separate from the module version.

quicksketch’s picture

quicksketch
he/him
commented
Status: Postponed (maintainer needs more info) » Closed (fixed)

This thread seems to have died out, I'm guessing that the new problem that started in this issue must have been fixed in more recent versions (as there have been 13 new versions since the last update here). Let's start a new thread for this issue if it still seems to be a problem. Reclosing per #11.