Feature "Remember the last selection" for views exposed filters doesn't work anymore

Can you identify the Drupal release that caused the regression?

I think I can reproduce this but I am not a Views expert. It will be really useful to determine what release introduced this.

The fastest way to pin this down would be to perform a git bisect on 9.2.11 vs 9.3.0.

I figured that was one of the ways this could break. So we just need to find the commit that did this by checking the release notes, looking at the commit log, diffing 9.2.11 and 9.3.0 (and then git blame), or git bisect.

Magnificent. I will comment on that issue to alert them to this one.

We recently ran into this issue as well. I checked the views module and only noticed 2 lines using $_SESSION. After updating those to use the Symfony session, remembering the filters works again.
I included a patch file containing the fix.

#15 works for me in Drupal 9.3.9. Thanks Maarten!

Thanks @maarten_plessius and @bserem for working on this. In order to make the tests pass, please wrap the whole code block into the following condition:

if ($this->getRequest()->hasSession()) {
  // mess with the session here.
}

Tidied up the Request and Session checks as suggested in #24.

lind101 made their first commit to this issue’s fork.

Thanks @lind101 for the work. It seems to me that the merge request now contains a couple of unrelated hunks. Would you mind minimizing the changes? The result should look like the patch from #15 but with an additional check around the new code as pointed out in #24.

Thanks for the feedback @znerol. The extra chunks are just me repositioning the conditionals to make the code a bit more readable and then wrapping all of the exposed input logic in a condition checking for the existance of a request (which we can't do anything without). Same code, just tidied up a bit!

If that explination doesn't suffice, just let me know and I'll dial it back as requested. :)

Cheers!

Drupal::hasRequest() is only necessary in very few edge cases. In fact grepping through the 10.0.x branch today turns up only two occurrences:

$ git grep Drupal::hasRequest
core/includes/errors.inc:  if (\Drupal::hasRequest() && \Drupal::request()->isXmlHttpRequest()) {
core/lib/Drupal/Core/Utility/ThemeRegistry.php:    $this->persistable = $modules_loaded && \Drupal::hasRequest() && \Drupal::request()->isMethod('GET');

I'd be very surprised if it would be necessary to add it here.

In order to make the job of the core committers easier, it is advisable to reduce the changes to the minimum which is necessary to fix a bug. This will increase the chance that a patch actually makes it into core in a timely manner.

Setting to needs work for the reason stated above.

Done!

I've left in the change of moving the $display_id variable declaration inside the Session conditional.

I've left in the change of moving the $display_id variable declaration inside the Session conditional.

That makes sense. Thanks a lot @lind101.

RTBC +1

RTBC +1 Works like a dream.

Forgive me if I'm wrong here, but seeing how this worked before and got broken, and given that 9.4 has not been released yet, I believe we can target 9.4 and not wait for an absurd amount of time to fix a regression. Right?

Thanks for filing this bug report and for fixing the issue. Bug fixing is very valuable. In order to commit a bug fix, we need an automated test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal, see the following links:

1. https://www.drupal.org/docs/testing/phpunit-in-drupal/phpunit-javascript...
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/9.3.x

This is especially true on this bug as we broke this unknowingly in #2473875: Convert uses of $_SESSION to symfony session retrieved from the request

Surprising there was not a test for this in the first place! +1 on the patch resolving it for embedded views

#15 Works. Thank you!

+1

Given a try to fix unresolved threads of MR, please review. Still needs work for tests.

Not directly related, but it appears that caching of the view will also cause problems with the "remember last selection": #2742273: Views with "remembered" filters are missing the use cache context

I have applied the patch in #15 and the selection is remembered in terms of the results of the view, but the actual value in the exposed filter form resets to the default (or the first in the list if no default is set), so the value in the excposed filter form is incorrect and is not what the view is actually displaying.

Patch #15 doesn't appear to work anymore. Just applied it to 9.4.9 and the exposed filters do not save.

Interesting. This is working fine here with merge request MR 2102 rebased on a fresh 9.4.x branch. Repro:

1. drush site-install
2. drush gent --bundles=tags 20
3. drush genc 50
4. Login as admin and edit the content overview view
5. Disable cache
6. Enable the remember option on all exposed filters
7. Save the content overview view
8. Navigate to the content overview and set filters
9. Switch back and forth between content overview and the comment / files tab
10. Result: Filters controls are set to the corrrect value and results are correctly filtered

This issue still needs tests. A good starting point is ./core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php.

I will help with advice and reviews if anybody wants to take up the task.

Yeah, MR 2102 works, but the patch on #15 doesn't when applied. I ended up manually doing the work that MR 2102 does locally to get it working. Now just trying to get it into a localized patch until it can get fixed in Drupal core since this we have an emergent need to use this.

I took a shot at adding a simple test. I can add more to it if there are more cases it should cover.

Thanks @davisben, the test provides coverage for the bug. I did verify locally that the test fails if the fix from MR 2102 isn't applied.

./vendor/bin/phpunit --filter=testRememberSelected core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php

Committer feedback from #37 was addressed in #41 and a test was added in #49, thus all green IMHO.

Yeah, MR 2102 works, but the patch on #15 doesn't when applied. I ended up manually doing the work that MR 2102 does locally to get it working. Now just trying to get it into a localized patch until it can get fixed in Drupal core since this we have an emergent need to use this.

@jfurnas: There is a way to retrieve a diff from a gitlab merge request. Just add .diff to the end of the MR URL. E.g.:

https://git.drupalcode.org/project/drupal/-/merge_requests/2102.diff

Also note that it is also possible to add .patch to the end of the URL. In that case the response contains the equivalent of git format-patch. E.g.:

https://git.drupalcode.org/project/drupal/-/merge_requests/2102.patch

Nice to see this working and with a test! It now just needs to be on 10.1.x, where the fix will be applied first.

Thanks!

If I'm not completely mistaken, fixing regressions is allowed within the release cycle: https://www.drupal.org/about/core/policies/core-change-policies/allowed-...

@quietone this is a regression that has been bugging people for a long time. I don't feel we should move it two versions in the future.
Given that 9.5.x/10.0.x are due tomorrow, it is ok we target that. But 10.1.x is too far away.

This issue needs a rebase. There is a chapter in the issue forks docs about that process.

I will help with advice and reviews if anybody wants to take up the task.

Thank you @rpayanm. I did a diff between MR 2102 and the new MR 3132. I did not notice any change in functionality.

Also took the new MR to a manual test-drive on a fresh 9.5.x, works beautifully. MR 3132 is good to go.

@bserem, @quietone is a Drupal core release manager, which means she has final decision-making authority over which branches will receive a change and all other issue metadata. Therefore, when she makes a decision on the branch setting of the issue, please don't revert it.

Furthermore, regardless of the fact that this change may be eligible for backport to the production 10.0.x and 9.5.x branches as a non-disruptive bugfix, the issue must still be committed to 10.1.x first under the core backport policy. Therefore, a patch that applies to 10.1.x or a version of the MR that has 10.1.x as its base branch is still required before we can commit it to 10.0.x or 9.5.x.

Finally, due to limitations of the GitLab integration, it can be difficult to get merge requests against multiple base branches to be tested properly, and currently the easiest way to get the respective MRs to be tested properly against the correct branch is by changing the version setting on the issue. So, @quietone was doing a helpful thing in that regard as well.

I closed the outdated merge request against 9.4.x (as well as one that had what looked like a bad merge), and hid the 9.3.x patch.

We still need a 10.1.x merge request here to proceed. I've left the passing 9.5.x merge request open as that will still be useful when it comes time to commit the backport (because I agree this issue is probably backportable as a non-disruptive bugfix).

Thanks!

I created a patch for 10.0.x and 10.1.x branches, please review.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Taking a look at the MR for 10.1.x

Applied the fix locally
Verified tests fail without the fix and obviously pass with it.

Manually test on Drupal 10.1.x standard profile
Edited the content view content type filter to remember last selection
Went to the content page and filtered by article
Went to another admin page
Went back to content page verified article is selected
Repeated X3

Code looks good so +1 from me.

The IS is missing a proposed resolution, as @smustgrave pointed out. I added it for posterity.

@xjm thanks for the clarifications. Completely understandable

OK, this looks now. Thanks everyone! Adding credit for helpful reviews, research, and improvements to the patch and merge request.

Committed to 10.1.x, 10.0.x, and 9.5.x. Thanks!

Ah - this was backported to 9.5.

Hi All,

For your information - There seems to be an issue when working with the "Remember the last selection" feature and Better Exposed Filters - https://www.drupal.org/project/better_exposed_filters/issues/3359182.

drupal 9.4.15 and 9.5.9

I have been scratching my head for a while on this issue.

If any one has any thoughts, welcome.

Drupal 10.3: if your filters are not saved, needs to disable cache for this view

Amending attribution.