When using a text field as an exposed grouped filter with the checkbox to allow multiple selections, I get the following error when trying to use the pager to go to the next page of results:

An illegal choice has been detected. Please contact the site administrator.

This shows up in the watchdog as:

Illegal choice 0 in field_presenter_last_name_value element.

Using Better Exposed Filters with the exposed form in a block.

I've attached screenshots and the display export.

Comments

rollsd’s picture

Confirming this bug. It still exists in 7.x-3.5, and on 7.x-3.x-dev branch.

This not only affects pagination links, but anything that uses the
views->raw_exposed_filters property to generate links. Table click-sort links are also affected by this.

To reproduce this error one can do the following on a test site:

  1. Install the Devel generate module and go to /admin/config/development/generate/content
  2. Select Article content type only and generate some nodes (eg 50).
  3. Create a new view by importing: test-search-exported-view.txt
  4. Try click on the column sorting for title on the view or pagination options.

You should see something like the following:
An illegal choice has been detected.

rollsd’s picture

StatusFileSize
new766 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,652 pass(es).
[ View ]

The problem is that pagination and quicksort links are being created from a set of inputs that does not understand that checkboxes set to zero should not be sent as part of the request.
Have found a fix that uses array_filter to remove values that have a value of zero, if it is a multiple exposed checkbox.

function views_exposed_form_submit(&$form, &$form_state) {
  foreach (array('field', 'filter') as $type) {
    $handlers = &$form_state['view']->$type;
    foreach ($handlers as $key => $info) {
      $handlers[$key]->exposed_submit($form, $form_state);
    }
  }
  $form_state['view']->exposed_data = $form_state['values'];
  $form_state['view']->exposed_raw_input = array();

  $exclude = array('q', 'submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', '', 'reset');
  $exposed_form_plugin = $form_state['exposed_form_plugin'];
  $exposed_form_plugin->exposed_form_submit($form, $form_state, $exclude);

  foreach ($form_state['values'] as $key => $value) {
    if (!in_array($key, $exclude)) {

      // If we have a multiple checkbox with values set to zero, we don't want
      // these included in the exposed_raw_input array as it breaks pagination.
      if (is_array($value) && $form[$key]['#type'] === 'checkboxes') {
         $form_state['view']->exposed_raw_input[$key] = array_filter($value);
      } else {
        $form_state['view']->exposed_raw_input[$key] = $value;
      }
    }
  }
}

rollsd’s picture

Version:7.x-3.7» 7.x-3.x-dev
Status:Active» Needs review
rollsd’s picture

Note: better exposed filters introduces a complication here as you have the option to move fields into a field-set named secondary (ie by making them a secondary filter). The above fix doesn't work in this case as it would need to check $form['secondary'][$key] for the type.

rollsd’s picture

StatusFileSize
new1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 1,652 pass(es).
[ View ]

Have modified this patch so it checks any children on the form (such as fieldsets) if it cannot find the $key directly on $form

function views_exposed_form_submit(&$form, &$form_state) {
  foreach (array('field', 'filter') as $type) {
    $handlers = &$form_state['view']->$type;
    foreach ($handlers as $key => $info) {
      $handlers[$key]->exposed_submit($form, $form_state);
    }
  }
  $form_state['view']->exposed_data = $form_state['values'];
  $form_state['view']->exposed_raw_input = array();

  $exclude = array('q', 'submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', '', 'reset');
  $exposed_form_plugin = $form_state['exposed_form_plugin'];
  $exposed_form_plugin->exposed_form_submit($form, $form_state, $exclude);

  foreach ($form_state['values'] as $key => $value) {
    if (!in_array($key, $exclude)) {

      $form_state['view']->exposed_raw_input[$key] = $value;

      // If we have a multiple checkbox with values set to zero, we don't want
      // these included in the exposed_raw_input array as it breaks pagination.
      if (is_array($value)) {
        $field = isset($form[$key]) ? $form[$key] : NULL;

        if ($field === NULL) {
          // The field may be burried in a fieldset somewhere so find it.
          foreach (element_children($form) as $child) {
            if (isset($form[$child][$key])) {
              $field = $form[$child][$key];
              continue;
            }
          }
        }

        if (isset($field) && $field['#type'] === 'checkboxes') {
          $form_state['view']->exposed_raw_input[$key] = array_filter($value);
        }
      }
    }
  }
}

Darren Oh’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

The patch in comment #5 works.

markocall’s picture

The patch in #5 worked for me as well, thanks a lot!

dawehner’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

To be honest, this amount of change is to awkward to be just committed without some serious test coverage, I am sorry.

rreiss’s picture

The patch in #5 worked for me. Thanks @rollsd !

enjaygee’s picture

Thanks @rollsd -- the patch in #5 worked for me as well.

rmathew’s picture

The patch in #5 resolves the "illegal choice" issue for me, however the pager URL is not quite right. eg:

http://[...]=All&search_api_views_fulltext=digital&&&page=1

Note the "&&&" - due to two checkbox values being ignored by the patch.

Crell’s picture

I just ran into this issue as well. I am seeing the &&& in the URL even without this patch, so that appears to be a separate issue.

Fendo’s picture

#5 worked for me too. only thing I wasn't aware need to logout and login again sessions to see changes.

mr. chips’s picture

I had to change one line in #5 patch:
from this
if (isset($field) && $field['#type'] === 'checkboxes') {
to this
if (isset($field['#type']) && $field['#type'] === 'checkboxes') {
or I would get this:
Notice: Undefined index: #type in views_exposed_form_submit() (line 2079 of ..... httpdocs/sites/all/modules/views/views.module)

I have several exposed filters with some as checkboxes, filtering entities from a custom table. I get the notice whether or not I use hook_form_alter. I can't duplicate that notice by filtering on Body of content type Article.

Other than that it worked for me. Paging was my problem before - if a checkbox group was left un-checked it would throw an error and check ALL the boxes in that group when going to the next page of results.

fietserwin’s picture

Same as #14, patch works but is giving undefined index notices. So that case should be handled as in #14.

BTW: BEF does not have this problem, as it leaves the #type to select, but changes the theme, and only in the theme function it renders checkboxes instead of a select.

rcodina’s picture

Patch on #5 works for me too.

Darren Oh’s picture

Issue tags:-Needs tests

I can understand holding up a feature until it has tests, but continuing to release this module without a bug fix that passes existing tests makes no sense.

fietserwin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
Test request sent.
[ View ]

Reroll of #5 (line number offset) + #14

pthurmond’s picture

I am going to agree with #17, this is getting to be irresponsible at this point. You have had a valid fix for this, on that is passing unit tests and peer review, and refuse to add the patch. Despite what was said in #8 this isn't that big a change. If you call that a big change then I don't want to know how you manage to code in a team environment. That is piddly at best.

So the question remains, when will this patch (or a better one if you have it) going to be applied?

mbopp’s picture

While it certainly isn't ideal, if you want to 'patch' the views module without actually modifying the code you can use hook_form_alter to do so. This would at least make upgrading the views module a bit easier, as this is something you will most likely want to do as time goes on.

An example of this in a custom module would be something like...

/**
* Implements hook_form_alter().
*/
function yourmodule_form_alter(&$form, &$form_state, $form_id) {
  if (isset($form['form_id']['#value']) && $form['form_id']['#value'] == 'views_exposed_form') {
    // dpm($form['#submit'], 'FORM');
    $form['#submit'] = array('yourmodule_exposed_form_submit');
  }
}

Then the submit handler... which is the modified views exposed form submit handler.

/**
* Submit handler for exposed filters
*/
function yourmodule_exposed_form_submit(&$form, &$form_state) {
  dpm('#called');
  foreach (array('field', 'filter') as $type) {
    $handlers = &$form_state['view']->$type;
    foreach ($handlers as $key => $info) {
      $handlers[$key]->exposed_submit($form, $form_state);
    }
  }
  $form_state['view']->exposed_data = $form_state['values'];
  $form_state['view']->exposed_raw_input = array();

  $exclude = array('q', 'submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', '', 'reset');
  $exposed_form_plugin = $form_state['exposed_form_plugin'];
  $exposed_form_plugin->exposed_form_submit($form, $form_state, $exclude);

  foreach ($form_state['values'] as $key => $value) {
    if (!in_array($key, $exclude)) {
      $form_state['view']->exposed_raw_input[$key] = $value;
     
      // If we have a multiple checkbox with values set to zero, we don't want
      // these included in the exposed_raw_input array as it breaks pagination.
      if (is_array($value)) {
        $field = isset($form[$key]) ? $form[$key] : NULL;
     
        if ($field === NULL) {
          // The field may be burried in a fieldset somewhere so find it.
          foreach (element_children($form) as $child) {
            if (isset($form[$child][$key])) {
              $field = $form[$child][$key];
              continue;
            }
          }
        }
     
        if (isset($field['#type']) && $field['#type'] === 'checkboxes') {
          $form_state['view']->exposed_raw_input[$key] = array_filter($value);
        }
      }
    }
  }
}

deggertsen’s picture

Status:Needs review» Reviewed & tested by the community

@pthurmond. Don't be so surprised. Views is a big project that goes through a lot of quality control. There have been some tests here, but status is still Needs review and only a few comments ago (#14) a bug in the patch was discovered which was just fixed in #18. There have been very few test since #18, which a maintainer would probably look at and say there needs to be more tests. That said, I think the change in #18 is minor enough that all the other tests are still valuable and I agree that it's at least time for a maintainer to take a good look at the patch. Thus I'm marking to RTBC to get their attention. Obviously that doesn't necessarily mean it will pass quality control, but at least we can get somebody looking at this who can make a decision.

The patch in #18 looks good to me. Perhaps a maintainer can weigh in?