- 🇨🇴Colombia jedihe
Patch #4 works on Drupal 9.5.8. We were having a problem where hitting /user/logout triggers the "To log in to this site..." message, after the user is properly logged out. A closer look into Drupal core showed that Drupal/Core/Session/SessionManager::destroy() calls session_destroy() instead of Session::invalidate() due to unwanted side-effects (details there); due to this, I also checked and found that both the session record and the browser-session-cookie are correctly cleared, which means the patch doesn't cause redundant session rows or bad impacts on caching.
I also ran some testing in a simplytest.me instance (vanilla 9.5.10-dev) and found:
- Chrome set to block cookies gets to show the "To log in to this site..." error message after submitting /user/login with valid login credentials.
- Chrome with cookies allowed: on logout, everything is properly cleared: browser-cookie as well as session record (checked this using views_sessions module).
Given all of the above, I think #4 is in great shape. Hopefully we can get some tests written, so it can be reviewed for merging.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Since Drupal 9 will be EOL soon, I think this needs to target D11 and get a backport to D10
- First commit to issue fork.
- Status changed to Needs review
7 months ago 5:11am 1 December 2023 - Status changed to Needs work
7 months ago 5:26am 1 December 2023 The Needs Review Queue Bot → tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- Status changed to Needs review
7 months ago 2:56pm 3 December 2023 - Status changed to Needs work
7 months ago 3:12pm 3 December 2023 - 🇺🇸United States smustgrave
Previously tagged for tests. When changing to review please take a look at the issue summary and tags to make sure it’s ready.
- 🇬🇧United Kingdom andrew.farquharson
andrew.farquharson → made their first commit to this issue’s fork.
- 🇬🇧United Kingdom andrew.farquharson
Merge request !5634 has patch #4 applied.
- 🇬🇧United Kingdom andrew.farquharson
The proposed fix at #18 seems the best solution. It fixes the original issue as strictly described. I have tested site instalaltion using drush site:install (Drupal 11.x) command. The bug does not occur. I believe that fix #18 since it targets the file that controls site installation done via the Drupal UI. Fix at #4 is far more general in its action and could have side-effects.
@smustgrave I am not sure how to test the installation via the Drupal UI? Is it possible to create a test that tests the equivalent of a Drupal UI installation? If a test is not feasible, it is simple to re-create the issue using the summary and testing manually.
There is a more general issue, possibly, as described at #22. But that should be dealt with as a new issue.
- Status changed to Needs review
3 months ago 7:29pm 9 April 2024 - 🇬🇧United Kingdom andrew.farquharson
I have applied the one liner fix at #18. I have not added any test because I do not know how a fresh installation using the UI is possible in, say, a kernel test. Unless someone knows how to create such a test, please review and test the fix manually.
To see how the issue is isolated to installations via the UI, try a site install using drush and note that the issue does not arise.
- Status changed to Needs work
3 months ago 7:50pm 9 April 2024 - 🇺🇸United States smustgrave
Still putting in NW as tests will be needed.
But does anyone have a suggestion for #41
Know can do a fresh install using functional tests.
