Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Write database sessions only once. #1163

Closed
wants to merge 8 commits into from

9 participants

@chrisguiney

I didn't see any pull requests in regaurds to issue #215

to only write (database) sessions once.

This would be a huge improvement for sites with large amounts of traffic, that are currently coming up with hacks around this issue.

@it-can

nice...

@JonoB

I think that the only time that this could be an issue is if you echo out an ajax response and then exit - in that case the __destruct method would never get called.

@philsturgeon

There is merit to this approach but there is also a potential issue, if the controller method does not use the output library and just outputs its own content to the butter (echo / exit) then this is going to throw a few errors like when using PyroCMS pages here:

http://d.pr/zWyB

@philsturgeon

A more relevant screenshot http://d.pr/b8XX

chrisguiney added some commits
@chrisguiney chrisguiney typo on variable name dca7c8f
@chrisguiney chrisguiney Seperate the cookie setting from the database write -- this allows th…
…e cookie to bet set prior to any output, but still keeps the database update to one per request
b7272fa
@chrisguiney

I've just put up some updates.

I've seperated out the cookie setting from the database write -- this should still allow the cookie to be updated pre-output,
but will keep the database write in __destruct()

@JonoB - I've been testing with exit()s, and my session data is being written.

@Dentxinho

I have a problem that might be resolved by this PR.
I want to set a message as userdata then unset it in the same request. I always get the error "Cannot modify header information - headers already sent".

@timw4mail

@Dentxinho That's what flashdata is for - http://codeigniter.com/user_guide/libraries/sessions.html

@chrisguiney This looks great. Never realized database sessions were being written twice.

@Dentxinho

@timw4mail Flashdata is only removed after the next request. If you want a flash/userdata only for the current request you need to unset it, then you get that error.

@narfbg
Owner

@Dentxinho If you need something for just the current request - you better use a regular variable. :)

@Dentxinho

@narfbg that's what i'm doing now :D

I'll explain:
My base view should display alert messages. Because flash messages are alive until the next request, they can't be used as alerts on the same request. For that I was trying to display alerts from flash data AND my custom "temp" data then unset that custom data on the end of script execution.

@narfbg
Owner

@Dentxinho Well, that's pointless. :) You shouldn't set session variables (or "flash data", in this case) if you don't need it for the next HTTP request.

Anyway, this is getting too much off-topic. Stick to comments on the pull request please. :)

@chrisguiney

@timw4mail They're being written for every single session variable set/unset...So database writes can get a bit out of control on a larger site.

Hopefully with b7272fa @philsturgeon 's issue with it sending cookie headers after output will be resolved, and this can get merged in

@timw4mail

@chrisguiney I see, so you just delay all the updates until the destruction of the class.

system/libraries/Session.php
((6 lines not shown))
public $CI;
public $now;
+ public $written = FALSE;
@it-can
it-can added a note

Please fix the indentation of the declared vars...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@it-can

What is the status of this pull?

@philsturgeon

Well, it is active up on a staging site I have. When the site has had some extensive testing on that server (we're launching it soon) I will be happy to merge this in.

@it-can

Nice, I will give it also a test run...

@philsturgeon

Ok I have a repeatable bug with this.

I have an AJAX request on the backend that is conflicting with this logic:

{"iTotalRecords": 1264, "iTotalDisplayRecords": 1336, "aaData": [ nice valid json] }<div style="border:1px solid #990000;padding-left:20px;margin:0 0 10px 0;">

<h4>A PHP Error was encountered</h4>

<p>Severity: Warning</p>
<p>Message: Cannot modify header information - headers already sent by (output started at /Users/phil/Sites/pyrocms.com/addons/default/modules/store/controllers/admin_orders.php:293)</p>
<p>Filename: core/Common.php</p>
<p>Line Number: 462</p>

</div><html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>Database Error</title>
<style type="text/css">
blablabla
}
</style>
</head>
<body>
    <div id="content">
        <h1>A Database Error Occurred</h1>
        <p>Error Number: 1054</p><p>Unknown column 'order_status' in 'where clause'</p><p>UPDATE `default_ci_sessions` SET `last_activity` = 1333385142, `user_data` = 'a:7:{s:9:\"user_data\";s:0:\"\";s:8:\"username\";s:12:\"philsturgeon\";s:5:\"email\";s:24:\"email@getoutofit.co.uk\";s:2:\"id\";s:1:\"2\";s:7:\"user_id\";s:1:\"2\";s:8:\"group_id\";s:1:\"1\";s:5:\"group\";s:5:\"admin\";}' WHERE `order_status` != 59999 AND `session_id` =  'a8a20a779e9bc0a659a5c10a1a588bb7' ORDER BY `order_id` desc, `default_ice_orders`.`order_id` desc</p><p>Filename: libraries/Session.php</p><p>Line Number: 565</p>  </div>
</body>
</html>

This only happens when I use this new session library, works fine when I revert back to the main one. Tried multiple times, definitely directly related.

NOW I have no idea what or why or how, but it looks like some where clauses are getting crossed over. any ideas? It might be a fault of my own, but seems odd that it works with the current session library and not with the other.

@Dentxinho

@philsturgeon maybe you missed a where and a order_by around your models?

I had some issues with random "where"s appearing when returning values from model's functions before issuing a get() or forgotting to stop_cache().

@chrisguiney

@philsturgeon sorry for the time delay on this, busy life and all -- but I've updated it to execute a generated query string, so it should circumvent any stale cache

@GDmac

From a comment on the PHP site: Per Persson 09-May-2012 01:57
As of PHP 5.3.10 destructors are not run [when the] "shutdown is caused by a fatal error"

Can someone verify this? @chrisguiney ?

Depending on wether you want the session to be written to the database, even after fatal errors,
you could register the destruct() method as a shutdown function.

@dchill42

A solution to this issue has been implemented in #353, which has now been merged into bcit-ci/CodeIgniter@1e40c21 by @philsturgeon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 11, 2012
  1. @chrisguiney

    Only write sessions once

    chrisguiney authored
  2. @chrisguiney
  3. @chrisguiney

    typo on variable name

    chrisguiney authored
  4. @chrisguiney

    Seperate the cookie setting from the database write -- this allows th…

    chrisguiney authored
    …e cookie to bet set prior to any output, but still keeps the database update to one per request
  5. @chrisguiney
Commits on Mar 15, 2012
  1. @chrisguiney

    removed unused var

    chrisguiney authored
Commits on Jun 29, 2012
  1. @chrisguiney

    Merge remote-tracking branch 'upstream/develop' into session

    chrisguiney authored
    Conflicts:
    	system/libraries/Session.php
  2. @chrisguiney
This page is out of date. Refresh to see the latest.
Showing with 68 additions and 33 deletions.
  1. +68 −33 system/libraries/Session.php
View
101 system/libraries/Session.php
@@ -265,6 +265,13 @@ public function __construct($params = array())
// --------------------------------------------------------------------
+ public function __destruct()
+ {
+ $this->_sess_write_db();
+ }
+
+ // --------------------------------------------------------------------
+
/**
* Fetch the current session data if it exists
*
@@ -396,39 +403,10 @@ public function sess_write()
return;
}
- // set the custom userdata, the session data we will set in a second
- $custom_userdata = $this->userdata;
- $cookie_userdata = array();
-
- // Before continuing, we need to determine if there is any custom data to deal with.
- // Let's determine this by removing the default indexes to see if there's anything left in the array
- // and set the session data while we're at it
- foreach (array('session_id','ip_address','user_agent','last_activity') as $val)
- {
- unset($custom_userdata[$val]);
- $cookie_userdata[$val] = $this->userdata[$val];
- }
-
- // Did we find any custom data? If not, we turn the empty array into a string
- // since there's no reason to serialize and store an empty array in the DB
- if (count($custom_userdata) === 0)
- {
- $custom_userdata = '';
- }
- else
- {
- // Serialize the custom data array so we can store it
- $custom_userdata = $this->_serialize($custom_userdata);
- }
-
- // Run the update query
- $this->CI->db->where('session_id', $this->userdata['session_id']);
- $this->CI->db->update($this->sess_table_name, array('last_activity' => $this->userdata['last_activity'], 'user_data' => $custom_userdata));
-
// Write the cookie. Notice that we manually pass the cookie data array to the
// _set_cookie() function. Normally that function will store $this->userdata, but
// in this case that array contains custom data, which we do not want in the cookie.
- $this->_set_cookie($cookie_userdata);
+ $this->_set_cookie($this->_cookie_data());
}
// --------------------------------------------------------------------
@@ -647,7 +625,6 @@ public function set_userdata($newdata = array(), $newval = '')
$this->userdata[$key] = $val;
}
}
-
$this->sess_write();
}
@@ -738,6 +715,64 @@ public function flashdata($key)
// ------------------------------------------------------------------------
/**
+ * @return void
+ */
+ protected function _sess_write_db()
+ {
+ // Generate a query string to execute, instead of using active record
+ // reference: https://github.com/EllisLab/CodeIgniter/pull/1163#issuecomment-4884026
+ // Run the update query
+ $this->CI->db->query(
+ $this->CI->db->update_string(
+ $this->sess_table_name,
+ array('last_activity' => $this->userdata['last_activity'], 'user_data' => $this->_custom_userdata()),
+ array('session_id', $this->userdata['session_id'])
+ )
+ );
+ }
+
+ // ------------------------------------------------------------------------
+
+ /**
+ * @return string
+ */
+ protected function _custom_userdata()
+ {
+ $custom_userdata = $this->userdata;
+ foreach (array('session_id','ip_address','user_agent','last_activity') as $val)
+ {
+ unset($custom_userdata[$val]);
+ }
+ // Did we find any custom data? If not, we turn the empty array into a string
+ // since there's no reason to serialize and store an empty array in the DB
+ if (count($custom_userdata) === 0)
+ {
+ $custom_userdata = '';
+ }
+ else
+ {
+ // Serialize the custom data array so we can store it
+ $custom_userdata = $this->_serialize($custom_userdata);
+ }
+
+ return $custom_userdata;
+ }
+
+ // ------------------------------------------------------------------------
+
+ protected function _cookie_data()
+ {
+ $cookie_userdata = array();
+ foreach (array('session_id','ip_address','user_agent','last_activity') as $val)
+ {
+ $cookie_userdata[$val] = $this->userdata[$val];
+ }
+ return $cookie_userdata;
+ }
+
+ // ------------------------------------------------------------------------
+
+ /**
* Identifies flashdata as 'old' for removal
* when _flashdata_sweep() runs.
*
@@ -919,7 +954,7 @@ protected function _unescape_slashes(&$val, $key)
{
if (is_string($val))
{
- $val= str_replace('{{slash}}', '\\', $val);
+ $val= str_replace('{{slash}}', '\\', $val);
}
}
@@ -955,4 +990,4 @@ protected function _sess_gc()
}
/* End of file Session.php */
-/* Location: ./system/libraries/Session.php */
+/* Location: ./system/libraries/Session.php */
Something went wrong with that request. Please try again.