Problem/Motivation

The views exposed filter form adds the filter values to $view->exposed_raw_input in \Drupal\views\Form\ViewsExposedForm::submitForm(). I found what I consider to be a bug in the way the views exposed form handles multivalue filters.

Steps to reproduce:

  • Edit the admin/content overview
  • Edit the 'type' filter
    • Select 'article' as default value
    • Check 'Allow multiple selections'
  • Create enough content to allow a pager

The following now happens:
In the pager, &article=article is added to the URL. This is supposed to be &type[]=article. When you submit the exposed filter first and click the pager after that, you get both in the URL &type%5B0%5D=article&article=article. This is because the $view->exposed_raw_input data is not correct.

Proposed resolution

The issue is in \Drupal\views\Form\ViewsExposedForm::submitForm(). The code for checkboxes does not use the key of the filter field. I think this should be fixed.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 2943163-16.patch708 bytesd.novikov
#10 interdiff-7-10.txt6.79 KBseanB
#10 2943163-10.patch4.29 KBseanB
#7 interdiff-3-6.txt633 bytesseanB
#7 2943163-6.patch3.88 KBseanB
#7 2943163-6-test-only.patch3.19 KBseanB
#3 2943163-3.patch3.88 KBseanB
#3 2943163-3-test-only.patch3.18 KBseanB
#2 2943163-2.patch708 bytesseanB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB at ezCompany commented
Status: Active » Needs review
FileSize
2943163-2.patch708 bytes

Patch is attached, please review!

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB at ezCompany commented
Title: Wrong URL parameters added to views pager for multivalue filter field » Wrong views exposed raw input values for multivalue filter field
Issue summary: View changes
FileSize
2943163-3-test-only.patch3.18 KB
2943163-3.patch3.88 KB

Now with tests. Also changing the IS and title since the issue about the wrong values in $view->exposed_raw_input, the pager is just how I noticed it.

The last submitted patch, 3: 2943163-3-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 2943163-3.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

Looking nice already.

  1. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
@@ -357,4 +357,90 @@ public function testFormErrorWithExposedForm() {
+  public function testExposedRawInput() {

    This can actually be a kernel test like this, maybe add it to ExposedFormRenderTest or something? 

  2. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
@@ -357,4 +357,90 @@ public function testFormErrorWithExposedForm() {
+    // Alter the identifier of the filter to a random string.

    Copy/paste left over, needs update to current test.

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB at ezCompany commented
Status: Needs work » Needs review
FileSize
2943163-6-test-only.patch3.19 KB
2943163-6.patch3.88 KB
interdiff-3-6.txt633 bytes

Ok missed 1 small thing, this should do it.

The last submitted patch, 7: 2943163-6-test-only.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs review » Needs work

Nice! Back to 'needs work' for #6

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB at ezCompany commented
Status: Needs work » Needs review
FileSize
2943163-10.patch4.29 KB
interdiff-7-10.txt6.79 KB

We seem to have cross posted so I missed #6. Thanks for the review. Updated the patch.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs review » Reviewed & tested by the community

Fix looks good, tests look good, feedback has been addressed, @seanB++

  • catch committed a71163c on 8.6.x 
    Issue #2943163 by seanB, Lendude: Wrong views exposed raw input values...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 971ce94 on 8.5.x 
    Issue #2943163 by seanB, Lendude: Wrong views exposed raw input values...

Status: Fixed » Closed (fixed)

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

d.novikov’s picture

d.novikov CreditAttribution: d.novikov as a volunteer commented
Version: 8.5.x-dev » 8.6.x-dev
FileSize
2943163-16.patch708 bytes

#10 does not work for filters with named keys. E.g., views date filter utilizes max and min keys for date fields. For example, we get the following structure for imaginary event date field:

[field_event_start_date_value] => Array
        (
            [0] => 2018-10-20
            [1] => 2018-11-02
        )

Instead, it should be:

[field_event_start_date_value] => Array
        (
            [min] => 2018-10-20
            [max] => 2018-11-02
        )

As a side-off, it breaks views pager links when using date filters.

Proposed solution attached.

mpolishchuck’s picture

mpolishchuck CreditAttribution: mpolishchuck at Alpha Web Group for Dazzle commented

The patch #16 works for me with Drupal 8.6.7.
URLs look consistent for filters and for filters+pager links.
I'm wondering why it's not posted to some open issue and not considered to merge. Maybe I've missed something?

gcb’s picture

gcb
he/him
CreditAttribution: gcb at ThinkShout commented

This issue still exists, and #16 resolves it for me (core 8.6.13). It is as d.novikov describes: in my case, we have a view using location proximity filtering, and the named keys are getting munched by this bug when building the pager, resulting in a pager that removes filters.

This issue should be re-opened, or a new issue created referencing this one.

smaz’s picture

smaz
Primary language English
Location England, UK
CreditAttribution: smaz commented

r.e. #16, #17 & #18:

It looks like that issue was resolved (for 8.7) as a result of https://www.drupal.org/node/2987598., as the issue over there has made the same as #16,