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.

Files: 
CommentFileSizeAuthor
#20 views_1881910_exposed-precedence_20-interdiff.txt450 bytesBerdir
#20 views_1881910_exposed-precedence_20.patch6.13 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 1,674 pass(es).
[ View ]
#20 views_1881910_exposed-precedence_20-test-only.patch4.43 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 1,673 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 views_1881910_exposed-precedence_18-test-only.patch1.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 1,669 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#18 views_1881910_exposed-precedence_18.patch6.17 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 1,674 pass(es).
[ View ]
#18 views_1881910_exposed-precedence_18-interdiff.txt6.06 KBBerdir
#14 views_1881910_exposed-precedence_14.patch4.33 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 1,587 pass(es).
[ View ]
#14 views_1881910_exposed-precedence_14-test-only.patch2.64 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 1,586 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 views_1881910_exposed-precedence_12.patch1.69 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 1,627 pass(es).
[ View ]
#6 views_1881910_exposed-precedence.patch1.67 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] 1,621 pass(es), 0 fail(s), and 9 exception(s).
[ View ]

Comments

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.

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

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.

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.

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.

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 1,621 pass(es), 0 fail(s), and 9 exception(s).
[ View ]

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 1,627 pass(es).
[ View ]

Retry. This time with an initial reset.

Status:Needs review» Reviewed & tested by the community

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

Issue tags:-Needs tests
StatusFileSize
new2.64 KB
FAILED: [[SimpleTest]]: [MySQL] 1,586 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.33 KB
PASSED: [[SimpleTest]]: [MySQL] 1,587 pass(es).
[ View ]

Test coverage.

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.

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new6.06 KB
new6.17 KB
PASSED: [[SimpleTest]]: [MySQL] 1,674 pass(es).
[ View ]
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] 1,669 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new4.43 KB
FAILED: [[SimpleTest]]: [MySQL] 1,673 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new6.13 KB
PASSED: [[SimpleTest]]: [MySQL] 1,674 pass(es).
[ View ]
new450 bytes

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.

Status:Needs work» Needs review

Patch passed, so back to needs review.

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