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.
Comments
Comment #1
BerdirThis 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.
Comment #2
johnvI 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
Comment #3
corvus_ch CreditAttribution: corvus_ch commentedNone 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.
Comment #4
dawehnerI'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.
Comment #5
corvus_ch CreditAttribution: corvus_ch commentedSo then I will work on a patch for 8.x first.
Comment #6
miro_dietikerAttached patch is a first try to switch the sequence:
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.
Comment #7
miro_dietikerAs 8.x is in core and something different, i'm trying first to address this here.
Comment #9
Berdir#6: views_1881910_exposed-precedence.patch queued for re-testing.
Comment #11
miro_dietikerRetry. This time with an initial reset.
Comment #12
miro_dietiker#11: views_1881910_exposed-precedence_12.patch queued for re-testing.
Comment #13
corvus_ch CreditAttribution: corvus_ch commentedLooks good. I can confirm this patch solved the problem.
Comment #14
BerdirTest coverage.
Comment #15
damiankloip CreditAttribution: damiankloip commentedThe fix and tests looks pretty good to me.
:)
Comment #17
dawehnerIt feels wrong to use the exposed_admin_ui view if we actual test the frontend.
Comment #18
BerdirUps. 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.
Comment #19
miro_dietikerThis 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.
Comment #20
BerdirYeah, I know you don't like those ;)
Also uploading a test only patch that has the default view, to see what's actually broken.
Comment #22
BerdirPatch passed, so back to needs review.
Comment #23
dawehnerNearly wanted to comment that this should use the request object
Comment #24
colanWe'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.
Comment #25
johnvThis is a re-upload of #20.
Comment #27
johnvThe tests are OK.
Comment #28
miro_dietikerOh it would be awesome to see this fixed. This created painful headaches.. ;-)
Comment #29
DamienMcKennaSeems like a reasonable fix.
Comment #31
DamienMcKennaCommitted. Thanks everyone!
Comment #33
MustangGB CreditAttribution: MustangGB commentedThis has caused a regression to checkbox filters (and maybe more), see #2867734: REGRESSION: Single ajax checkbox filters are broken.
Comment #34
hanoiiI 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.
Comment #35
MustangGB CreditAttribution: MustangGB commentedAnother regression from this patch #2878837: REGRESSION: Better exposed filter checkboxes not being able to untick all selections.