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 |
Comments
Comment #2
donquixote commentedThe 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 commentedIn 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.incis 31dd0540 (The Great Git Migration), also in 2009. So it is all quite historic.Comment #4
donquixote commentedBefore 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:
Comment #5
donquixote commentedHere is a start.
As said, we still need to decide what is the intended behavior. But I think it is ok as a starting point.
Comment #6
donquixote commented