Do not write unchanged sessions to the database

wow - nice work. can't believe we didn't do this earlier. rtbc.

This looks very nice. In most cases, we were already duplicating the session data memory (ie. once for $GLOBALS['user']->session, the second one for $_SESSION), so this doesn't actually increase memory usage.

Really, really nice.

session-write-1.patch queued for re-testing.

great - bot is still happy.

session-write-1.patch queued for re-testing.

Can you quickreroll and put a newline between @param and @return....
Srry that I'm so late with this.

If noone else gets to it, the committers can add the line break (or a follow-up). Lets try not to set important patches to needs work over a single doxygen line break (IMO). Bot is happy so lets take advantage.

Ok attached a new version of patch 1 with a cleanup of the file.
I didn't change anything of the original patch.

Dries or webchick: If you don't like this additional clean up in this patch just use the one in 1.

+++ includes/session.inc	26 May 2010 13:39:50 -0000
@@ -61,12 +61,22 @@
-function _drupal_session_read($sid) {
+function _drupal_session_read($sid, $return_last = FALSE) {

- I like this patch a lot, but I dislike the $return_last parameter that was added to _drupal_session_read(). I think it is cleaner to cache the old value elsewhere. For example, I think it would be cleaner if the $user object kept the original session data. That's how you'd do it in most other programming languages -- I don't think any method in the Java API has a 'return_last' parameter.

- Technically, the extra parameter can be omitted because _drupal_session_read() is only called once, as far as I can see.

- We have an existing pattern for flushing static caches in core. _drupal_session_read() introduces a static cache that cannot be flushed by the test bot. It introduces an inconsistency.

In other words, I'd like to see us ponder some more about the method signature and how we handle caching.

fwiw drupal_static() sharing is also used in path.inc for system paths caching too.

Needs a quick reroll. Sorry!

+  // Store the user that was loaded for comparison in _drupal_session_write().
+  $last_user = &drupal_static('_drupal_session_read', NULL);
+  $last_user = $user;

I would prefer we store only what's really necessary from the $user object (ie. sid and session), and that we renable the '_drupal_session_read' name to something more meaningful ('drupal_last_loaded_session' ?).

I read through the code and can find no issues. I see that Dries and damien's comments have been addressed. This is a huge performance win ... Please wait for green before commit.

+++ includes/session.inc	14 Jun 2010 20:26:42 -0000
@@ -105,109 +106,133 @@ function _drupal_session_read($sid) {
+      // The "secure pages" setting allows a site to simultaneously use both secure
+      // and insecure session cookies. If enabled and both cookies are presented
+      // then use both keys. If not enabled but on HTTPS then use the PHP session
+      // id as 'ssid'. If on HTTP then use the PHP session id as 'sid'.

does not wrap at 80 characters anymore. Re-rolled patch fixes this, everything else is unchanged.

51 critical left. Go review some!

Back to RTBC.

#19: 744384-session-unchanged.patch queued for re-testing.

Grr. This one sat for too long. I wonder why the patch went from 15kn to 10kb between 17 and 19. Anyone available to investigate and reroll?

Thanks for the reroll. Back to RTBC.

+++ includes/session.inc	20 Sep 2010 15:38:01 -0000
@@ -136,80 +150,90 @@ function _drupal_session_read($sid) {
+      if ($is_https) {
...
+        if (variable_get('https', FALSE) && isset($_COOKIE[$insecure_session_name])) {

It looks like we could move the variable_get() into the $is_https condition.

Powered by Dreditor.

#24: session-write-13.patch queued for re-testing.

#24: session-write-13.patch queued for re-testing.

Moshe pinged me about this again tonight.

It looks like Dries was poised to commit this back in June and got blocked by needing a re-roll, so I took a look tonight.

This is a nice performance improvement for sites using a caching server. There is an API change in this patch (it removes the ability to load an anon user by session ID in drupal_anonymous_user()) which we talked about at length on IRC. Moshe's stance is that while this will impact some contrib modules (e-commerce-related stuff, certain voting modules, etc.) there's a simple workaround (just look in $_SESSION the same as you would in a normal PHP script). I'm a little on the fence myself. But since this patch was originally intended to be committed 3-4 months ago by Dries, I decided to let it slide. Obviously if this ends up causing a lot of problems in contrib, we'll need to revisit this decision.

In the meantime, Committed to HEAD. Moshe said he'd follow-up with instructions for contrib modules who are affected for Randy.

This is just a scaling optimization. The API changes for sessions went in a long time ago. That change had us no longer automatically start a session for anonymous users. This is vital for reverse proxies like Varnish. The takeway for module devs is to minimize use of $_SESSION for anon users. If you want lazy sessions for D6, use Pressflow.

One change here which could affect a few modules (i can't think of any) is that the timestamps in the sessions table are no longer accurate up to the second. we now write only once every 3 mins by default. This matches how access timestamps in user table work.

Thanks very much for the info.

I'm not even sure if I understand a change (in this patch) large enough to announce. Just mention that

* Signature change for drupal_anonymous_user()
* $_SESSION is still there, but is not up-to-the-second

True?

All the data in session will be up to the second, but the timestamp won't be.

This will slightly change the behaviour of the who's online block - specifically if it's set to less than 3 minutes online. That might be worth a followup, but since the session writing is a variable too, it might also just be worth ignoring.

I am getting this notice when writing tests for securepages:
Undefined property: stdClass::$timestamp Notice session.inc 176 _drupal_session_write()

It seems that the $user->timestamp is never initialized. From my testing, this notice always occurs - but it's not normally captured. I'm guessing it's evasiveness has something to do with _drupal_session_write() being a "magic" PHP callback, but I'm not sure.

many thanks to catch for helping on IRC tonight. I think $is_changed is FALSE when logging in via HTTPS. If you run user.test on an https:// site, you can see the PHP notices. And this explains why the test bots haven't detected it.

This is closely linked to a much more complex issue, so I'm just rolling a variation of this into #1050746: HTTPS sessions not working in all cases