views_handler_filter::exposed_form() is messed up. [#2704545]

I found the following code in views_handler_filter::exposed_form():

    // Build the form and set the value based on the identifier.
    if (!empty($this->options['expose']['identifier'])) {
      $value = $this->options['expose']['identifier'];
      $this->value_form($form, $form_state);
      $form[$value] = $form['value'];

      if (isset($form[$value]['#title']) && !empty($form[$value]['#type']) && $form[$value]['#type'] != 'checkbox') {
        unset($form[$value]['#title']);
      }

      $this->exposed_translate($form[$value], 'value');

      if (!empty($form['#type']) && ($form['#type'] == 'checkboxes' || ($form['#type'] == 'select' && !empty($form['#multiple'])))) {
        unset($form[$value]['#default_value']);
      }

      if (!empty($form['#type']) && $form['#type'] == 'select' && empty($form['#multiple'])) {
        $form[$value]['#default_value'] = 'All';
      }

      if ($value != 'value') {
        unset($form['value']);
      }
    }

Let me explain how this is meant to work. At least this is from my observation with search_api.
- $form contains the entire exposed form in its current "under construction" state. It may already contain filter widgets from previous calls to the same method, on different filter handlers.
- $value contains the key for the new widget / filter. This is a poorly chosen variable name, but whatever.
- $this->value_form($form, $form_state) creates a new widget under $form['value'].
- This new widget is then copied to $form[$value].
- Now some finishing operations are applied to the widget.

I think the idea is that views should check whether the widget is #multiple, and which #type it has.
The correct way to do this would be to look into e.g. $form[$value]['#type'].
Instead, views looks into $form['#type'] and $form['#multiple'].

I found this because sometimes the #default_value of a select filter widget turned out to be an array.

Comment	File	Size	Author
#5	views-7.x-3.x-2704545-5-views_handler_filter-exposed_form.patch	3.62 KB	donquixote
#5	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 210 pass, 1 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

12 April 2016 at 10:16

donquixote created an issue. See original summary.

Comment #2

donquixote CreditAttribution: donquixote as a volunteer commented 12 April 2016 at 10:43

The problem is we cannot just "fix" this in a straightforward way, because it is likely that existing code already depends on the misbehavior.

Comment #3

donquixote CreditAttribution: donquixote as a volunteer commented 12 April 2016 at 11:16

In my case, $this->exposed_translate($form[$value], 'value') removes the '#multiple', and replaces 'checkboxes' with 'select'.
But the #default_value is still an array.
The code in exposed_form() is meant to detect this and clean up the #default_value. But it fails, because it looks at $form['#multiple'] instead of $form[$value]['#multiple'].

Btw, inside exposed_translate(), the variable $form stands for the widget element at $form[$value], not the entire exposed form.
This poor choice of variable names could be a reason why such bugs happen in the first place.
E.g. maybe the code was once copied from a place where $form had a different meaning.

git blame shows 31dd0540 in 2009. But this commit was just copying stuff around. The original commit with git blame 2647afe6^ handlers/views_handler_filter.inc is 31dd0540 (The Great Git Migration), also in 2009. So it is all quite historic.

Comment #4

donquixote CreditAttribution: donquixote as a volunteer commented 12 April 2016 at 12:14

Before I do anything here (e.g. post a patch), we need to decide what is the intended behavior.
The #default_value needs to be cleaned up after exposed_translate(). But how?

Cases to consider:

multiple-value select or checkboxes:
#default_value needs to be an array.
The existing code, if we fix it, would simply unset the #default_value. This might be intended, or maybe not.
If the #default_value is already an array, we could simply leave it alone. If only for BC reasons.

single-value select:
#default_value needs to be a string or integer.
The existing code, if we fix it, would set the #default_value to 'All'. This seems wrong, because 'All' might not be a valid value at all.
Also in some cases, #default_value might already be in the correct format. In this case we could simply leave it alone.
Either way we need to check if the value is in #options. If not, we should unset it.

----

Local variable renaming:

I would suggest to rename $value to $value_key.
Instead of directly copying $form['value'] to $form[$value], I would suggest a local variable $element = $form['value'].
Do all the operations on $element, then set $form[$value] = $element.
Instead of checking isset($element['#type']) && $element['#type'] == 'select', we could introduce a local variable $type = isset($element['#type']) ? $element['#type'] : NULL.
'==' and '!=' should be replaced with '===' and '!=='.