Posted by gansbrest on September 22, 2010 at 3:48pm
8 followers
| Project: | No Anonymous Sessions |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
| Issue tags: | Drupal 6, lazy sessions patch |
Issue Summary
I having problems with drupal_set_message (which uses $_SESSION to store messages). Basically if I use drupal_set_message inside form submit handler (let say to display status message like 'Item was added to your shopping cart'), then it will not be displayed.
Comments
#1
Here is my thoughts after some more investigating:
drupal_set_message inside submit handler doesn't work because there is a redirect after form submit and the actual message was suppose to be stored in the $_SESSION and displayed on the next pageview (right after redirect):
Looking into the sessions.inc file of the module I was thinking maybe it's better to:
1) Remove from sess_read()
// Modification for no_anon module: turn off session cookies
ini_set('session.use_cookies', 0);
so most of the anonymous requests will fall into next condition and basically will construct anon user object with any sessions data
2) Inside sess_write() just keep
if (!session_save_session() || ($user->uid == 0 && empty($value))) {
return TRUE;
}
so anon sessions will not be saved to db if SESSION is empty
Here is another part with goes right after snippet above
if (!empty($user->session))
$value = '';
This will remove any sessions data from sessions table (let say it will hide notification messages if any)
Lets assume following scenario:
Anon user put an item in the shopping cart and saw a notification message about that action. Because of his action the row was created in the sessions table with uid = 0 and notification message stored in session field. After user redirect (after submit) session_read() will construct user object with message in session property which will be displayed on the page and then removed from sessions table by sess_write()..
Basically there will be the same limitations like in Pressflow installation (no modules should write to SESSION if user is anon) but drupal_set_message will work as expected + there actually will be no need for the module and the only required thing is that include file.
#2
Try your approach and see if it works, then submit working and tested patches. If others verify that they are working, I will include them in the module.
For an alternate approach, check no_anon.module, check the no_anon_form_alter() function. See how we set back cookies to enabled? Perhaps you can use something like that and put it in the module?
#3
Just created the patch to illustrate my thoughts.
Basically the idea behind the patch is very simple: If any module requires $_SESSION for anon users and website uses that module - then the row will be created in the sessions table as usual. That should allow any existing module to work with the new (patched) sessions include.
If someone wants to reduce the number of rows and obviously number of queries to sessions table then he must eliminate (or disable) any module that stores data inside $_SESSION for anon users. (no pressure though :)
The benefit of using patched sessions include is "lazy" session creation (Pressflow like) - if no need for the $_SESSION (user just browses the site) no entry will be created in the sessions table. If user submits the form and that form uses drupal_set_message from the form submit handler (user initiated the action) the row will be created in the sessions table, but on the next pageviews there will be no access to sessions table since $_SESSION will be cleared after message was presented to the user + on the cron run sessions with uid = 0 will be eventually garbagecollected.
Again the difference between original no_anon session include is that patched version don't force you to disable session cookie and there is no need for the module itself, the only file we need is that patched session file + drupal_set_message will work in patched version.
I'm using it on my site and it looks good so far, so if anyone is interested in testing - go for it! :)
#4
I had a problem with comments because a moderation message wasn't shown when an anonymous user submits a comment. As gangsbrest said, there is a redirect after form submit so the expected message is not shown. After a deep debug I realized that no_anon was on the problem.
The patch at #3 worked for me. Thank you very much!
#5
#3 patch is perfect! thanks!!
#6
Subscribing.
#7
Will this also have implications for, say, anonymous shopping carts in Ubercart?
#8
If Ubercart stores data in $_SESSION for anonymous users - then you probably will need a patch, otherwise it's not going to play nicely with 'No Anonymous Sessions' module. But keep in mind that you will get a row in sessions table for every anonymous cart, which is fine for this specific use case I guess (since user initiated the action).
#9
subscribing
#10
Subscribing.
#11
I really like this patch. We are using it in a custom session include, and don't even need the module.
Our sessions table now has about 100 entries and has been stable for the past few days. Previously it fluctuated between 5000 and 10,000 records.
#12
So I rolled a patch for the session.inc in core (6.22) based on the patch in #3. The only thing that's different is that in line 23 of the patch from comment #3, the if statement checks if $user->session is empty, whereas in the stock version of session.inc from Drupal 6.22, the if statement checks if $_COOKIE[session_name()] is empty... I left the if statement the way it was in the stock file. Not sure if this is correct or not.
If that is correct, though, I wonder if the if statement in line 28 of the patch from comment #3 (and line 17 of the attached patch) should use $_COOKIE[session_name()] instead of $user->session.
This patch certainly applies more to core than to this module, but I figured this would be the best place to continue the discussion.
#13
I just checked core session include from 6.22 and the problem with your approach / patch from #12 is that $_COOKIE[session_name()] will always create a row in session table for anonymous users, since there is unconditional session_start() statement in the bootstrap as opposed to conditional session handling in Pressflow.
I created slightly modified version of my initial patch to session-no-anon.inc which removes notification messages handling since drupal does that by default (it removes messages from session once displayed). So we don't need that part any more.
The other addition I made is I added $user->status == 1 condition like in 6.22 to start anonymous session for blocked / inactive users.
New patch attached.
#14
Yes I applied the patch by hand as I noticed the core file had changed too.
I too removed the $_COOKIE[session_name()] part, and in my case put the isEmpty($user->session) back in. It is basically what gansbrest has posted in #13 and it seems to work fine.
#16
Great, thanks to gansbrest and mr.j.
So is there a point to using this module with gansbrest's patch applied, or does patching core's session.inc have the same effect?
#17
There is no need for the module. All you need is that patched session include file. You can replace drupal stock session file from settings.php like this:
$conf['session_inc'] = 'includes/session-no-anon.inc';or you can take changes from the latest patch from #13 and modify core session.inc file. If you do that, make sure you produce the patch file after you done, so you could reapply the patch in the future if you need to (after core updates for example).
But generally speaking fivefrank was right about moving latest patch into D6 core, since this thread is not really belongs to no_anon module. Maybe we should start that process somehow.
#18
I've attached a patch for core's session.inc based on gansbrest's patch in #13, just for the sake of completeness. Not counting changes to comments, it's basically a one line change.
I doubt this patch will ever get included in core, though, since it breaks throttle and the "who's online" block, and possibly other things.