Given a page with two views with exposed filters
And those filters has the option Remember the last selection set
And view A has exposed filters set
When I apply filters to view B
Then the filters of view A get lost.

I had a look at view::get_exposed_input(). If there are any get parameters, the values stored in the session are not considered. I sugest, to switch this. Meaning: we should load the values from the session first and merge what is in the get request afterwards.

If there are no objections, I would provide a patch for this but I want to be sure that this does not colide with anything.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

This probably needs to be implemented in 8.x first?

The current code dates back to #267457: Linking feed icon to a feed which considers exposed filters, commit 1fbbb0d1.

johnv’s picture

I guess you have the problem with no caching set? Exposed filters do not work on cached views. See #1055616: Query arguments should be replaced before generating cache ID

corvus_ch’s picture

None of the views in question uses caching so far. So [#1055616: Query arguments should be replaced before generating cache ID] shouldn't be the cause.

dawehner’s picture

This probably needs to be implemented in 8.x first?

I'm happy if people help to fix bugs in 7.x-3.x, porting the patch would cause them to much troubles, especially because not a single view file has the same location.

corvus_ch’s picture

Version: 7.x-3.5 » 8.x-3.x-dev
Assigned: Unassigned » corvus_ch
Status: Active » Needs work

So then I will work on a patch for 8.x first.

miro_dietiker’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.67 KB

Attached patch is a first try to switch the sequence:

  • Start with session exposed input
  • Override it with _GET

This should follow the precedence. However, note this is completely untested.
Also, tests for this precedence issue should really be added to views.
Additionally, also placing multiple views on one page is important for testing.

miro_dietiker’s picture

Version: 8.x-3.x-dev » 7.x-3.x-dev

As 8.x is in core and something different, i'm trying first to address this here.

The last submitted patch, views_1881910_exposed-precedence.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests

The last submitted patch, views_1881910_exposed-precedence.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

Retry. This time with an initial reset.

miro_dietiker’s picture

corvus_ch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. I can confirm this patch solved the problem.

Berdir’s picture

damiankloip’s picture

The fix and tests looks pretty good to me.

+++ b/tests/views_exposed_form.testundefined
@@ -57,14 +57,48 @@ class ViewsExposedFormTest extends ViewsSqlTest {
+  function dtestExposedAdminUi() {

:)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, views_1881910_exposed-precedence_14-test-only.patch, failed testing.

dawehner’s picture

+++ b/tests/views_exposed_form.testundefined
@@ -57,14 +57,48 @@ class ViewsExposedFormTest extends ViewsSqlTest {
+    $view = views_get_view('test_exposed_admin_ui');

It feels wrong to use the exposed_admin_ui view if we actual test the frontend.

Berdir’s picture

Ups. Removed the dtest stuff and other crazy stuff, had to run to catch my bus and no time to check the patch ;). Added a new default view called views_exposed_remember and also added test coverage that changing the stored value still works.

miro_dietiker’s picture

Status: Needs review » Needs work
+++ b/tests/views_test.views_default.incundefined
@@ -218,5 +218,64 @@ function views_test_views_default_views() {
+    3 => 0,
+    4 => 0,

This is not related. Those are local test roles i guess. Also i think it should be fixed in the general export to apply array_filter.

Rest looks just perfect.

Berdir’s picture

Yeah, I know you don't like those ;)

Also uploading a test only patch that has the default view, to see what's actually broken.

Status: Needs review » Needs work

The last submitted patch, views_1881910_exposed-precedence_20-test-only.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Patch passed, so back to needs review.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/includes/view.incundefined
@@ -430,9 +420,17 @@ class view extends views_db_object {
+      foreach ($_GET as $key => $value) {

Nearly wanted to comment that this should use the request object

colan’s picture

Issue summary: View changes

We've recently switched our testing from the old qa.drupal.org to DrupalCI. Because of a bug in the new system, #2623840: Views (D7) patches not being tested, older patches must be re-uploaded. On re-uploading the patch, please set the status to "Needs Review" so that the test bot will add it to its queue.

If all tests pass, change the Status back to "Reviewed & tested by the community". We'll most likely commit the patch immediately without having to go through another round of peer review.

We apologize for the trouble, and appreciate your patience.

johnv’s picture

This is a re-upload of #20.

Status: Needs review » Needs work

The last submitted patch, 25: views_1881910_exposed-precedence_20-test-only.patch, failed testing.

johnv’s picture

Status: Needs work » Reviewed & tested by the community

The tests are OK.

miro_dietiker’s picture

Oh it would be awesome to see this fixed. This created painful headaches.. ;-)

DamienMcKenna’s picture

Seems like a reasonable fix.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks everyone!

Status: Fixed » Closed (fixed)

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

MustangGB’s picture

This has caused a regression to checkbox filters (and maybe more), see #2867734: REGRESSION: Single ajax checkbox filters are broken.

hanoii’s picture

I wonder if now that there's a fix around here (albeit the regression reported), if #1080164: Exposed filters with remember me does not clear if everything is submitted empty could be given a shot again? It was closed as won't fix 6 years ago, but the issue is still valid and I am still keeping the patch up to date because I need it in a project.

MustangGB’s picture