Posted by rfay on March 27, 2010 at 4:22am
15 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user system |
| Category: | bug report |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | needs backport to D7, session |
Issue Summary
A drupal_set_message() issued during the logout process is lost and never displayed.
What I did: Created an system_message_action and associated it with the user_logout trigger.
What I expected: A message displayed to the user after logout.
What happened instead: No message was displayed.
I imagine this is because the session is destroyed, and the messages are queued in the session. This is not the expected behavior, though.
Comments
#1
The same problem occurs, for the same reason, I'm sure, at user account creation/login.
1. Create a system message action and associate it with user_insert trigger.
2. Create a user account.
3. No message is displayed.
The trigger fires, the drupal_set_message() is executed. But nothing is displayed.
#2
#3
Session should be regenerated not destroyed. I'm trying to look latter
#4
I believe this is delt with in the (still incomplete #813492: HTTPS sessions use an invalid merge query).
#5
If we can get #721086: Create tests for system actions, clean up token replacement, clean up triggers done and then if #813492: HTTPS sessions use an invalid merge query can make progress, we need to add to it the attached patch, which will properly test.
#6
#813492: HTTPS sessions use an invalid merge query has been committed, so it's time to see if this was resolved.
#7
Temporarily un-duplicating and bumping, to keep this on the radar.
#8
Checked manually (the test modified by #5 seems to be gone):
- messages triggered on logout are still lost in oblivion
- messages triggered on login and user insert work
#9
Downgrading. This only affects logout, and looks by-design to me:
<?php
watchdog('user', 'Session closed for %name.', array('%name' => $user->name));
module_invoke_all('user_logout', $user);
// Destroy the current session, and reset $user to the anonymous user.
session_destroy();
drupal_goto();
?>
We could consider being slightly less heavy-handed here (for example by defining a hook to prevent part of the session from being destroyed). But at least this works as designed.
#10
The test modified by #5 is in #721086: Create tests for system actions, clean up token replacement, clean up triggers and is here as a placeholder in case it can go back into that one later (it was commented out in that one).
#11
Just in case anyone Googling this runs across this issue - drupal_set_message stores the message in the database, tied to session. user_logout destroys the session and does a drupal_goto. Users will get a new session as soon as they load the drupal_goto'd page, but there's no way to link that to the message, which was associated with their old session. To get the drupal_set_message functionality without something like including a querystring and checking for it on the destination page, we need to give them their new session *before* the goto. Here's how I did that:
1) Call session_get_cookie_params to get the current session's attributes
2) Call session_destroy to end the current session
3) Call session_set_cookie_params based on the values returned by 1)
4) Call session_start to start a new session
5) Log the user out with $user = drupal_anonymous_user(session_id()); instead of just $user = drupal_anonymous_user();
6) drupal_set_message followed by drupal_goto will now work.
I don't know if this is the safest/sanest approach, regarding e.g. attacks on session ID, but it at least works for my simple use case. Suggestions for refinement would be appreciated.
(This was in D6, by the way, but the logout code is similar.)
#12
in #11.5 did you mean "Log the user IN...", not "Log the user out..." If not, huh?
#13
Yes, good catch, thanks.
#14
This affects Drupal 6 too. Would like to see it resolved for both D6 and D7 as doing a drupal_set_message on hook_user('logout') is beneficial for the user experience.
#15
@eronarn: I tried out your idea with the following code:
<?php// 1) Call session_get_cookie_params to get the current session's attributes
$p = session_get_cookie_params();
// 2) Call session_destroy to end the current session
session_destroy();
// 3) Call session_set_cookie_params based on the values returned by 1)
session_set_cookie_params($p['lifetime'], $p['path'], $p['domain'], $p['secure'], $p['httponly']);
// 4) Call session_start to start a new session
session_start();
// 5) Log the user out with $user = drupal_anonymous_user(session_id());
// instead of just $user = drupal_anonymous_user();
$user = drupal_anonymous_user(session_id());
// 6) drupal_set_message followed by drupal_goto will now work.
drupal_goto();
?>
Unfortunately it gives the following error:
Fatal error: session_start() [<a href='function.session-start'>function.session-start</a>]: Failed to initialize storage module: user (path: /Applications/MAMP/tmp/php) in /modules/user/user.pages.inc on line 157Going to dive into it some more.
#16
I've resolved the error by changing the session_start part to:
<?phpsession_set_save_handler('sess_open', 'sess_close', 'sess_read', 'sess_write', 'sess_destroy_sid', 'sess_gc');
session_start();
?>
Now to retain the messages.
#17
Here's a patch against Drupal 6.
Patch sponsored by the Siebel Scholars Foundation.
#18
#19
no one-letter variable names. as for the rest, i have no idea
#20
Changed $p to $params.
#21
Patches for both D6 and D7.
#22
The last submitted patch, drupal-logout_session-n754560-20.patch, failed testing.
#23
#24
This is still a problem and the scope is larger than originally outlined. Any other modules that set $_SESSION variables which aren't tied to the login lose their data on logout.
The D7 patch is outdated and needs reworking. Not sure about the D6 patch.
#25
Moving to 8.x
+++ modules/user/user.pages.inc 17 Jan 2011 18:27:54 -0000@@ -170,9 +170,22 @@ function user_logout() {
+ // Obtain the current session's attributes.
This part cares only about session params but other content stored is going to be lost
Powered by Dreditor.
#26
Setting to needs backport
#27
subscribe (sorry if this is the wrong way to do it, but I can't see a monitor/subscribe button anywhere).
#28
Here is #21 rerolled for D8.
#29
Related: #758730: Fatal error on logout.
The route forward seems to be what I asked for in #9: we implement drupal_session_destroy() as described in #758730: Fatal error on logout and we add a hook in there that allow contrib modules to rescue part of $_SESSION.
#30
Adding session tag.
#31
IMO, this is a small problem, not worthy a complex solution. Code that wants to set a message after logout should try to run after the destroy.
#32
Here's a simple solution: don't destroy the session on logout.
Session != login
Drupal seems to make this assumption, however.
#33
Ran into the same problem with Drupal 7.12: after logging out all shopping cart related data (custom module) is gone...
#34
@moshe weitzman:
Could you sketch in how this might work? Sample use case: using Rules, log out a user and present them a message about having been logged out. Currently, this would be done with two actions: (a) set a message and (b) redirect (to user/logout). But if the message is set before redirecting, it is never presented to the user as the session ends. If it is set after redirecting, it is never executed because the page execution (and session) have ended. How could a rule action execute after the destroy?
#35
+++ b/modules/user/user.pages.inc@@ -171,9 +171,22 @@ function user_logout() {
+ session_set_save_handler('sess_open', 'sess_close', 'sess_read', 'sess_write', 'sess_destroy_sid', 'sess_gc');
For D7+ the session calls are renamed, see http://drupal.org/node/224333#session_rename.