Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added Session driver with native PHP sessions and original-flavor CI cookie sessions #353

Merged
merged 30 commits into from
@dchill42

This is a refactoring of the original CodeIgniter Session library as a driver library. It adds a native PHP session driver for those who prefer that route, while keeping all the neat features of original CI sessions and a couple new features suggested on UserVoice. This code has been thoroughly tested and running stable on St. Petersburg College servers for many, many moons.

Bon Appetit!

@ktomk

Man, what should I say :) I was just yesterday thinking about something very similar as we're integrating a project into CI which is using native PHP sessions. Maybe the native driver could be called php so it's more clear that it is bound to the PHP session configuration? And the ci session driver could be split into session and database probably. That was part of the idea I had yesterday. Would that make sense?

@kylefarris

Without looking too far into this, it sounds like an awesome idea. I like that you implemented this using drivers.

@dchill42

@ktomk - I'm glad this came at a convenient time for you. I think the two supplied drivers are pretty well set the way they are, but you are completely free to write a new one to plug into this Session driver library if you like. You should also be able to override either of these drivers with a local replacement or extension if you just want to make minor changes to the way they work. That's the beauty of the driver system, and why I implemented it for sessions. Your name suggestion is a good one, but I'm not sure I want to change it at this point, and I wouldn't change the original CI session implementation other than abstracting it out to interface and driver like I already have.

@kylefarris - Thanks! it's been working very well so far. My team has been using this driver for about 9 months, since we wanted to use native PHP sessions instead of the CI cookie solution. I figured there were enough strong features in the original implementation, though, that keeping the CI_Session interface and being able to swap out storage solutions seemed like a good idea.

@ktomk

@dchill42: Yeah that were merely some thoughts, maybe not really practical or a burden. For the moment it's even better this stays pretty close so it's easy to integrate which I'm currently doing against 2.0.2. I run into a problem that the driver loader is expecting the Session directory to be lowercase, I'm fiddling around that somehow. I'll let you know if I get it working.

@dchill42

@ktomk - I'll take a look at that tomorrow. Like I said, this code is running on our server, but perhaps something has changed in the core, or got hosed when I carried it over from bitbucket. Thanks for the heads up, as well as the suggestions - I welcome both!

@ktomk

@dchill42: Found the culprit, an ucfirst missing in the 2.0.2 code that moved into CI_Driver_Library::load_driver. I'm currently porting your changes manually so have not copied over everything.

@ktomk

Got it to work.

Just some issue probably informative: If session was accessed like:

 $this->session->userdata['myvalue'];

which is not documented, it results in the try to load the Session_userdata driver. So I just fixed our user-code with the wrong accessors to solve this.

@ktomk

I'm playing with this:

        array_unshift($params, $name, '');
        $params['lifetime'] = time() - 42000;
        call_user_func_array('setcookie', $params);

For support of the additional parameters (the secure and httponly one). When setting the cookie further on top this could as well be reflected, maybe an additional configuration option.

@ktomk

@dchill42: I got this to work with CI 2.0.2. Only a slight change in the Driver Loader (the uppercase problem) and switching in autoload from session library to session driver. And that's it. For a better integration I've added the drivers into the application directory to keep the changes of the CI code files minimal for the beginning. Works great!

system/libraries/Driver.php
@@ -45,8 +66,8 @@ class CI_Driver_Library {
$child_class = $this->lib_name.'_'.$child;
// Remove the CI_ prefix and lowercase
- $lib_name = ucfirst(strtolower(str_replace('CI_', '', $this->lib_name)));
- $driver_name = strtolower(str_replace('CI_', '', $child_class));
+ $lib_name = strtolower(preg_replace('/^CI_/', '', $this->lib_name));
+ $driver_name = strtolower(preg_replace('/^CI_/', '', $child_class));

I have about 3 different versions of these two lines hanging around. The ucfirst seemed to be causing problems, so I put in the version that's been working here at SPC.

@ktomk
ktomk added a note

I actually need that ucfirst on my linux based file-system because the driver base directory is named Session. Changing it to all lowercase will throw the loader a warning that the driver could not be loaded.

So... this was not the ucfirst I was looking for? Does something need to be changed in Loader to make this work?

@ktomk
ktomk added a note

I needed to add ucfirst in /system/libraries/Driver.php to get it working with 2.0.2 in:

    // Remove the CI_ prefix and lowercase
    $lib_name = ucfirst(strtolower(preg_replace('/^CI_/', '', $this->lib_name)));
    $driver_name = strtolower(preg_replace('/^CI_/', '', $child_class));

Which is now about line 69 in that file but I shifted lines a bit. Please keep in mind that I did not merged your changes in via git but applied changes manually.

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

I've used this (in its pre-github incarnation) on a couple membership sites, where php sessions facilitated integration other software the client was using, and worked great. One thing - shouldn't it default to CI's cookie-based sessions?

@dchill42

@anaxamaxan: It's funny - I was looking over the code the other day and asked myself the same question. Certainly, it's easy enough to change the default if anyone else agrees.

@ktomk

It's more worthy to spend the time in updating the docs probably because the default should just be a configuration option. Additionally I think the PHP native driver should support all PHP configuration options that PHP offers across all available versions to make it useful for system integrators.

@ktomk

@dchill42: Just some feedback if you're interested: We're running into problems integrating with the native session driver in a legacy codebase as code uses session_... functions as well (e.g. name, id etc. pp.). Everything worked fine w/o that, right now I try to make the session lazy starting which would be a nice feature, especially as sessions are automatically ignored if the application does not need them ;).

Some basic cookie and config handling could be put into the base class, like getting/merging the settings and getting the cookie parameters / setting the two year default (I found it useful as well that the class knows under which session_name it was registered, I wish it's possible to change it programatically, but that's another thing). It's just that there is some duplicate code.

Another thing which could be considered is to store the active time, user-agent string and ip into a subarray so this won't conflict with userdata set that easily. I think the original session driver does this in some kind (prefixing), in native session this could be done with a sub-array keyed at '__' or so.

@ktomk

@dchill42: To make the driver a bit more flexible, it would be good if the concrete driver (or even global) could signal the base driver class to re-bind the userdata. This is needed for $_SESSION if you start a new session. The base driver still refers to the zval container of the old session. $this->session->userdata is then disconnected from the current session store. I just added a public method for some tests:

final class Session extends CI_Driver_Library {
...
    public function bind_userdata()
    {
        $this->userdata =& $this->current->get_userdata();
    }
...
}

It's called in the constructor as well. This way, the reference is still private and the concrete driver is still in control.

@dchill42

@ktomk: Thanks for the suggestions - I'll look into implementing them soon.

@ktomk

@dchill42: Just seeing this, the userdata member in core Session is a public variable. I think the base session driver should reflect this accordingly and have it public as well.

@skunkbad

+1

I really like the idea of CI having a native PHP $_SESSION usage option. In my own apps, I end up using $_SESSION instead of the CI session library. There may be some scalability issues, but it really should be a choice.

@cnizzdotcom

I actually just started writing a new plugin that acts the same as the native CI session library (same method names, parameters, and return values). It would be nice if CI just implemented this themselves and had a config for using database, cookie, or PHP style file sessions. I've noticed problems with how CI implements its sessions when using VM boxes over at browsercam.com for testing. Ever CI application/site I've tested drops sessions including my own, bambooinvoice, and ones I picked from this list at random: http://codeigniter.com/wiki/Applications_Using_Code_Igniter

@skunkbad

Ideally we should be able to use both, not just one or the other. CI sessions are nice for flash session usage.

@mattrobenolt

Flash messages should actually be stored in a cookie separate from actual session data. Separate that out, then leave actual session data to a driver. Another example of a driver would be memcache or a hybrid cache and database. These are highly important in large scale deployments. I really hate the idea of slinging around 3KB of cookie data, and database only is a terribly slow alternative.

@ktomk

Hybrid MySQL / Memcached storage ships with MySQL5 .6: http://schlueters.de/blog/archives/152-Not-only-SQL-memcache-and-MySQL-5.6.html

@dchill42

@mattrobenolt - Separating out the flashdata isn't a bad idea, but is there really a good reason to store it separately from the rest of the items? The original flashdata implementation in CI_Session stored those values right along with the rest, so I have maintained that mechanism - including the original, well-tested code - in this driver implementation. I think one of the advantages of flashdata storage in PHP sessions is that there is no size limit.

I have also added a parallel feature to this library called tempdata, which works very much like flashdata, but sweeps old values based on an expiration time instead of request count. This could also be achieved with cookies, but what if you want to store sensitive data in such a manner? Why encrypt data and pass it back and forth on every request when you can just keep it locally in session data (or whatever other storage mechanism you want to write a driver for) and retrieve it when you need it?

I am very much open to suggestions for improvements for this library, but you'll have to do some more convincing if you really believe flashdata should be stored separately.

@dchill42

@skunkbad - Thanks for your feedback. Actually, you CAN use both drivers with this library - they are interchangeable on-the-fly. Once you load the library, you can alternate between $this->session->native->userdata() and $this->session->cookie->userdata() (or any other such calls), specifically identifying which driver to use for each call. While there are configuration options to specify which driver to load by default, they will also load on-demand.

So, normal usage is to configure a driver and just call (e.g.) $this->session->userdata() directly, but specifying a driver will ensure it is loaded and explicitly route an operation through it. This means you can store some data in cookies, and others in PHP sessions - even if you use the same key in both, they will not conflict. Also, if you wrote, for example, a file-based driver, you could load and alternate between all three within the same request. There is no limit to how many different drivers you can manage with this library.

@mattrobenolt

@dchill Flash data is used to just Flash messages. They're one time use, and how much data are you really shoving into it? You really should not be shoving more than 4KB of data into a flash message. You typically just need to set, "Success" or some other message quickly, and remove it. Having flash messages stored in a cookie only eliminates the need for technically needing sessions at all in some edge cases. Why introduce a session if it isn't needed? And for performance sake, sessions consume clock cycles and disk I/O or whatever overhead the session driver introduces. It's just unnecessary for most use cases. Throw it into a cookie and call it a day.

Overall, I don't think it's a huge deal either way. I've typically implemented flash messages in cookies for simplicity and some other frameworks do the same. I think it just comes down to preference.

What I am opposed to is bloating up cookies with a big serialized PHP object. That's just clunky and a mess. There's no need for that. Most people aren't configuring cookie-less domains for their static assets, so your large serialized cookie is being slung around for every single asset when it could simply be: "message=Saved+successfully!" like a normal cookie.

@mattrobenolt

Also to harp on being opposed to a serialized object: it makes it very difficult to interface with other languages. The PHP serialization protocol isn't a standard, so implementations are a hit or miss in other languages. Say I have a cookie on: *.example.com. Now I have a separate service running on a.example.com in Python or Node.js, but I still want access to the session cookie, it makes it nearly impossible. Or even to access the cookie via Javascript.

This is also a reason to support multiple session backends. In our product, we're storing sessions in Memcache being backed by MySQL, and that session data is accessed both by Python/Django and Node.js. Python/Django manages the total session, and Node.js queries memcache just to read the data. (I know it's volatile, but in our case, the chance of memcache being down isn't that major of a problem)

@dchill42

@mattrobenolt - If you want to store a flashdata item in a cookie, then use the cookie driver instead of the PHP session driver - both are included in this library, and can be used interchangeably, as noted in my post to skunkbad. Regarding the serialization in the cookie driver, that is legacy code from the original (current) CI_Session library, and I am not inclined to change it. I have included that driver so that this library can be a drop-in replacement for the former one, with all of the previous functionality operating just as it always has (other than the abstraction of the driver layer).

If you prefer a more simplistic, open-structured cookie storage mechanism, you are welcome to add a driver that does exactly that. It would then be interchangeable with the two drivers I have included here. In fact, if there is a reasonable level of demand for such a driver, I'll even write it myself. However, the original cookie driver is not something I want to re-engineer for the reasons given above.

@chonthu

+1

@skunkbad

@dchill42 - good job. I have a feeling that the only thing holding this back from being pulled in would be somebody with a negative opinion of $_SESSION. I do hope to see this in a future version of CI.

@mattrobenolt

@dchill42 I agree with the concerns, I'm more backing the idea of making the session engine a driver so we have control and can write our own handler. That would satisfy everyone's needs. It would be a major backwards compatibility issue converting how sessions are natively stored. It would need to be reserved for a 3.0 release at this point.

@dchill42

@mattrobenolt - The native storage mechanism is the part that's up to each driver with this library. The session variables are basically handled as an associative array with the variable name as a key (plus modifiers for flashdata and tempdata names) to the value. For the legacy cookie driver, that array gets serialized into a cookie whenever an element changes. For the PHP session driver, the whole array is simply mapped to $_SESSION, so changes happen directly and immediately. New drivers could be implemented in a variety of creative ways, depending on the back-end in use (file, database, shared memory, etc.). Most drivers you could think of should be very easy to write and plug in, and can be stored in the application path or a package path, if not included in the core libraries.

Obviously, I have no idea whether this library will get picked up before 3.0 or not, but all the features are here, well-tested, and working smoothly. Perhaps this recent flurry of activity will draw attention to it and help promote adoption. Thanks for your +1, as well as your comments above - it's great to see people interested and offering feedback.

@dchill42

@skunkbad - Thanks again. :)

I suspect the biggest reason this hasn't been addressed yet is just that the Reactor team has been very busy with their top priorities. Surely negative opinions of $_SESSION do exist, but I would hope that they wouldn't hold up the pull since nobody would be forced to use that driver. It's just a new option for those of us who prefer to use it. I would not be surprised, however, to get an official request to make the legacy cookie driver the default, as suggested by @anaxamaxan a couple months ago. That change, along with a simple tweak to load CI_Driver_Library when a library is loaded with CI_Loader->library() and found in its own subdirectory, would ensure complete backwards compatibility with existing applications using the older session library.

@skunkbad

@dchill42 - I wish they would at least comment. It's been months since you first issued this pull request.

@dchill42

It looks as though we can all effectively vote on PHP Session support in this CI survey. Hurry, though - I don't think it will be up for very long!

@skunkbad

Yes, I already voted, but I don't understand why, since your pull request allows for both, and since it won't break existing code, it hasn't been commented on by the CI dev team. Not even, "Hey, we're busy, but this looks good/bad".

@dchill42

@skunkbad - I agree, acknowledgement would be most excellent. Maybe if we wave our arms around real big, they'll notice...

@kylefarris

I agree too... I did the survey... but the native PHP session aspect of it is just a driver. If no one wants to use it, they don't have to. Not a lot of things have been pulled recently, though--I wouldn't take it personally. I'm guessing the Reactor team is overwhelmed with the number of requests, unenthusiastic (doubt that)/unsure about where the project is going, or just busy with their own projects (like the rest of us).

I wish there was someone at Ellislab who's only job was to handle the open-source side of CI. It can only benefit them seeing as EE is written using it. EllisLab? You hear me? I'll do it for the right price. :-)

@philsturgeon

You don't have to wave your arms, but if you put a @philsturgeon in there you'll get a reply from me, @ericbarnes, @pkriete and @gaker can be poked too.

So, this looks great and I think I remember seeing it a while back but at the time it was too big for 2.1 so it was put to one side. Something should have been said but there are f**kload of pull requests and issues to get through, sometimes we forget - and sadly nobody is getting paid! :p

The code is now somewhat out of date as changes have been made to the driver library, drivers can be autoloaded now in the core and changes have happened to the session library. If you could update this code we can talk about getting it in, especially now that the other guys have been poked.

@dchill42

I will get this up to snuff this week. Thanks for taking the time to comment, Phil - especially on a holiday. Hopefully the others will weigh in soon.

@it-can

This is nice! Would be very nice if this gets included in CI...

@HipsterJazzbo

What's the status of this? Would be a fantastic addition to CI. @dchill42?

@philsturgeon

Hey @dchill42 you you update this codebase so we can try again? I'd love to move session storage over to Memcache and you've got a bunch of code here that can avoid me needing to write anything! :)

Also, if you can update any unit tests and write some new ones possibly, it would be hard for anyone to think up a reason to keep this out.

@chonthu

+1

@tkaw220

+1

@dchill42

@philsturgeon, I'll see how quick I can get that accomplished.

And, thanks to others for encouragement - it's nice to see that there's some demand for this. I'd love to see it finally included.

@it-can

Yeah the sooner the better!

@tkaw220

@dchill42 Can't wait to see this great addition in next release.

@philsturgeon

@dchill42 still needs updating with other 3.0 changes! :)

@dchill42

Sorry - busy, busy, busy, busy! I'm about to have some free time to dedicate to that update, though. I'll do it ASAP.

@dchill42

@philsturgeon, this needs to be in sync with CodeIgniter/develop? (as opposed to master or something)
[Edit] Scratch that - dumb question. Working on bringing this into sync - should be ready soon (as soon as I get my head on straight).

@philsturgeon
@dchill42

@philsturgeon we are in sync. Let me know what else (if anything) needs to be done with this.

system/libraries/Session/Session.php
((170 lines not shown))
+ public function sess_regenerate($destroy = false)
+ {
+ // Just call regenerate on driver
+ $this->current->sess_regenerate($destroy);
+ }
+
+ /**
+ * Fetch a specific item from the session array
+ *
+ * @param string Item key
+ * @return string Item value
+ */
+ public function userdata($item)
+ {
+ // Return value or FALSE if not found
+ return (!isset($this->userdata[$item])) ? FALSE : $this->userdata[$item];

Can you put spaces around any !'s and this should return NULL Instead of FALSE now (we changed stuff). I'll get this off to EllisLab to see what they say.

Do you have any unit tests for this btw?

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

@philsturgeon Done and done. Sorry, I don't have any unit tests at this point. :(
I know unit testing was (relatively) recently added. Are they required for acceptance at this point? Are most contributors including them? I see the the tests directory in the tree, with modules derived from CI_TestCase - is this how/where it's done?

@philsturgeon
@dchill42

I'll see what I can do. Those may take a little bit to work up, though.

@philsturgeon, This unit test framework really isn't ready for drivers, and seriously needs more documentation, but I'm trying to piece something together anyway. I can tell you I've already had to modify tests/mocks/autoloader.php to get it off the ground. I hope that doesn't ruffle any feathers when it gets committed to this branch.

@dchill42

@philsturgeon We now have unit tests. Driver autoloading added to unit test framework, several bugs caught and killed, and fully in sync with develop as of now. Read: ready when you are. :)

tests/codeigniter/libraries/Session_test.php
((21 lines not shown))
+ }
+
+ // Start with clean environment
+ $this->cookie_vals = $_COOKIE;
+ $_COOKIE = array();
+
+ // Establish necessary support classes
+ $obj = new stdClass;
+ foreach (array('config' => 'cfg', 'load' => 'load', 'input' => 'in') as $name => $abbr) {
+ $class = $this->ci_core_class($abbr);
+ $obj->$name = new $class;
+ }
+ $this->ci_instance($obj);
+
+ // Attach session instance locally
+ $config = array('sess_encrypt_cookie' => FALSE, 'sess_use_database' => FALSE, 'sess_table_name' => '',

Trivial I know, but you can you newline each array item?

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

@philsturgeon Done throughout, including inline arrays carried over from legacy Session code. Also added userdata resync on regenerate, as noted by @ktomk above.

@dchill42

@philsturgeon Dare I ask, at what point do we worry about documentation updates?

@philsturgeon
@dchill42

Duly noted. I'll see if I can get a head start adding the driver features to the existing Session docs.

It may be worth noting (esp. to internals) that this library should act as a drop-in replacement for the former one, without any adverse effects or required changes to users' legacy code. Loader->library() will find it in the Session subfolder during its last-ditch effort, and the default driver is cookie. Obviously, using Loader->driver() is faster, but nothing should break from replacing the lib with this driver.

@GDmac

This is a very nice addition, chapeau @dchill42, some questions after testing this afternoon.

I had to explicitly load->library('driver'), otherwise the load->library('session') generated an php error:
Class 'CI_Driver_Library' not found in ... system/libraries/session/Session.php on line 41
Loading Driver for session should probably be in the docs. (ps: does the unit-test autoload drivers?)

Session_native doesn't set userdata[session_id]. This might be nitpicking, but
some apps [citation_needed :)] might use it, or might check against session->userdata[session_id]

Session_native doesn't generate a new session_id when the "sess_time_to_update" expires.
Is there a specific reason not to generate a new session ID, (just asking) ?

@dchill42

@GDmac You're right - I had forgotten that the regular library loader doesn't backstop the library with the Driver base class. I'll have to go in and wire that up, if @philsturgeon agrees. I would really like to see this have full backward compatibility with the previous Session library. At very least, as you point out, documentation will be requisite.

As for your points about Session_native - you've raised a couple good ones. Setting session_id should be very simple to implement, and I'll have to take a look at the time_to_update feature. Thanks for your input!

@GDmac

in the session_native driver, you could,
add 'sess_time_to_update' to the $prefs array in the initialize() method, and after the if($destroy) block,
(before updating the last_activity) add a check for regenerating the session_id.

    if (($_SESSION['last_activity'] + $config['sess_time_to_update']) <= $now)
    {
        $this->sess_regenerate(FALSE); // don't destroy
    }
@GDmac

1.) I have been scratching my head over this, while testing the library.
Changing the sess_driver from native to cookie with database=true,
results in an error Invalid driver requested: CI_Session_CI_Session_cookie
the problem seems to occur somewhere in the select_driver() method.

2.) also the CI_Driver_Library is still not found (autoloaded),
i had to change in loader (line 914) to trim slashes from $subdir:

// if (strtolower($subdir) == strtolower($class) && ! class_exists('CI_Driver_Library'))
if (trim(strtolower($subdir),'/') == strtolower($class) && ! class_exists('CI_Driver_Library'))
{
    // We aren't instantiating an object here, just making the base class available
    //require BASEPATH.'libraries/Driver.php';
    require_once BASEPATH.'libraries/Driver.php';
}

3.) session_id is lost for native session.
When setting sess_driver to native and also throwing $this->session->cookie->set_userdata() in the mix,
then the native session_id changes on every request. Data is not retained anymore?

//controler
$this->foo = $this->session->native->userdata('foo');
$this->foo = $this->foo + 1;
$this->session->cookie->set_userdata('bar', 'hello'); // comment this line and foo updates again
$this->session->native->set_userdata('foo', $this->foo);
echo $this->foo;
@dchill42

@GDmac Good catches - chapeau to you, too! Thanks for your testing and feedback, you found things I hadn't had the time to test myself. If you're curious, #3 was a cookie conflict between the two drivers - Cookie was storing data in the same place Native was looking for a session name (oops).

@philsturgeon What's our status with this? Is anyone talking or thinking about including it? Are we in the queue, or held up by something else going on, or being summarily ignored? We have unit tests, we have integration testing (thanks to GDmac), we have docs, and we have full backward compatibility.

@philsturgeon

@dchill42 it looks like you've fixed the reported issues with this so far. Anybody else have any other issues?

This has the go-ahead from Wes Baker at EllisLab, so we can merge it in if @GDmac confirms those bugs are fixed and the tests are still passing :)

@dchill42

@philsturgeon Excellent! I am stoked! I hadn't realized @GDmac was testing in an official capacity. (Thanks again for thorough testing and constructive feedback.)

Do we need anything else in the unit tests? I think the Session tests cover all the key features of the library itself (other than ones we can't really test under PHPUnit), but I was thinking of adding driver() and driver as library() tests to the Loader unit. Similarly, @GDmac had asked about autoloading, which should also be covered in the Loader test, but currently isn't. I'm not sure this is the right branch for mucking about with the Loader unit test, though. Any thoughts?

@philsturgeon
@GDmac

Tested, and all seems well.
@philsturgeon is #1163 still on the radar? that pull-request related to this pull-request:

  • aren't php-native sessions affected by file-locks? (and should thus be ok and safe)
  • cookie-session data, does the last process to finish win the cookie? (but server data may be inconsistent, hmm)
  • database-session data, without some lock mechanism during processing, possible overwrites and race conditions? (hmm^2)

edit: The latter two might be preventable by planning the webapp but makes me uncomfortable.
Article about php-session file blocking http://konrness.com/php5/how-to-prevent-blocking-php-requests/

@dchill42

@philsturgeon and @GDmac I'm working on the Loader unit test, and found a couple minor bugs - hence that last commit. Unlike the other Loader calls, driver() was returning FALSE when an array of drivers was passed. That's usually ignored, but I don't like inconsistency.

Also, when _ci_load_class() made its last-ditch effort to locate a library in a subdirectory (which we rely on for our backwards compatibility), it was failing to pass on $object_name, so the user couldn't attach it to the framework under a different handle. This is also something of a corner case, but I thought should be fixed while on the radar.

Both of these situations will be covered by the updated Loader unit test.

@dchill42

@GDmac Some answers:

  • Yes, the default native session file module uses flock() on each session file.
  • Yes, the last process gets the final say on cookie data contents.
  • As for database overwrites - yes, but race conditions - not really. All the db calls are atomic, so there isn't an actual race, but much like the cookie data, a later asynchronous process may overwrite previous data.

I'm not sure it's really worth adding semaphore acquisition before each cookie data write, though. In my mind, that's overkill and basically unnecessary overhead on a process that's already getting slow complaints, according to #1163. I think if a developer knows he's writing a highly per-client concurrent application, it's up to him to employ extra safety measures.

So, I'd say to a developer: if your app spams the server with overlapping AJAX requests (one starting before the previous one finished) that are each going to update your cookie based session data, then add your own semaphores or use the native driver (or write a new one). That's my personal perspective on it, but if there's any consensus that a cookie locking feature is necessary or appropriate, I'll add one.

Also, you're correct about #1163 - what he's trying to do needs to be done in a registered shutdown function. @philsturgeon, if that feature is desired, I'll be very happy to add a proper implementation to this driver.

@GDmac

@dchill42 IMO maybe first get this pull-request in.

  1. i played a bit with a register shutdown function but am unsure if during coding, when -cough- fatal errors sometimes do occur, you want to write out data. Maybe only session_id and last_activity etc. (ps. if you do register a __destruct() method as a shutdown function, it gets called twice, once during object destruction and once at shutdown).
@dchill42

@GDmac Yeah, it's about time to stop mucking with this so it can get pulled. I'd only consider that delayed database write if it were inevitable to be included anyway. I'd just rather take the idea and implement it myself now than have someone else hacking it into the driver later.

@philsturgeon

@dchill42 I think we should probably address this at the same time? It's most likely going to cause some conflicts if we try smashing the two together after-the-fact and his implementation had a bug that I reported which seemed to get fix with a hack.

@dchill42 dchill42 Minor session test improvements
Signed-off-by: dchill42 <dchill42@gmail.com>
08c8304
@dchill42

@philsturgeon OK. From what I saw in #1163, the change is about separating the database calls to make one update at the end of the request.

Given the structure of my drivers here, I think a two-pronged approach might fit best. I'm thinking of calling a database update method upon sess_destroy(), which will set a flag saying it's written, and also registering a shutdown function which will call the same method, but only do the update if the flag has not been set. That way, it won't be left until the very end unless the end comes unexpectedly (like in a fatal error). Does that sound about right?

[Edit] No, that's not right - I forgot that sess_destroy() kills the session. It's not a cleanup function like a destructor. Perhaps just registering the shutdown function is the best approach. Can you think of any reason why that won't work?

@philsturgeon
@dchill42

OK, coding that up. I might just install a destructor in the main library anyway and give each driver a shutdown function for any final cleanup. I could see that being useful in other scenarios, too. I'll check all that in as soon as it's ready.

@dchill42

@philsturgeon An implementation of #1163 is installed, and I cleaned up the cookie code a bit. It's a little more DRY now, and easier to read. All unit tests continue to pass, but at present we are not testing the database functionality. Is there a recommended way to test against a data source in the unit tests yet?

Meanwhile, @GDmac if you happened to test the cookie driver with the database option, I sure wouldn't be mad. :)

@GDmac

Test failed
1. Session_cookie.php#L302 method sess_save() should be:
if ($this->sess_use_database === TRUE) (not FALSE)

2. Session and data not stored
It helps for array_diff_key and intersect_key if there actually are some keys to compare ;)

    protected $defaults = array(
        'session_id'    => null,
        'ip_address'    => null,
        'user_agent'    => null,
        'last_activity' => null,
    );

Remarks.

  • the shutdown() and __destruct() functions for driver_library are IMO unnecessary and something the driver probably should take care of itself, the registered shutdown function will be called anyway.
  • Might i suggest to register the shutdown() method of the driver instead of update_db(). Check in shutdown for sess_use_database and dirty data. (i was pondering at first why you would want to check sess_use_database inside an update database method). After removing the destruct from the library and registering shutdown all works well and is called once.
@GDmac

I had to google 'semaphore acquisition' but it is a problem with the cookie and database session class. Consider the following jquery on a page, and the update method storing $foo + 1 into the session. Only for native php sessions is $foo updated twice (without resorting to a timeout).

function ajax_test(x){
  $.ajax({
    url: '/welcome/update/' + x,
    success: function(data) {
    $('#result').append(data);
  }
});

$('#clicky_the_button').click(function(e){
  $('#result').html('');
  ajax_test('random');
  ajax_test('dummy');
  // setTimeout( "ajax_test('baz')", 100 ); // timeout needed for session cookie
});

I don't suggest implementing 'semaphore acquisition' but wanted to make note of the issue.
(some further googling) hmm, sadly php semaphore support is not enabled by default http://php.net/manual/en/book.sem.php

edit: wouldn't a simple (blocking?) database lock on the table row work similar as flock?

@dchill42

@GDmac Doh! I didn't even recognize that I wasn't comparing keys to keys in the diff functions - just silly. Obviously, it was getting late when I finished this last night.

You're right about the shutdown feature - it's just redundant. The same thing can be more readily accomplished by just adding a destructor to the driver. And, in this case, that would run just barely before the shutdown call, so I scrapped that whole route. I was the one talking about overkill yesterday, wasn't I?

As for the database check inside _update_db, it's just extra safety since that's technically a public function. If some yahoo calls it directly or something, it still does what it should. It only runs once per request, so I don't think the extra comparison cost is significant.

Regarding the AJAX scenario, I definitely understand the issue. Yes, a database lock would achieve the same effect, but what if database isn't being used? Also, there is no consistent, reliable, cross-DB way to lock a single row. We wouldn't want to lock rows for other sessions. My suggestion for code like your sample would simply be to queue one request after the other, something like this:

function ajax_test(x, next){
  $.ajax({
    url: '/welcome/update/' + x,
    success: function(data) {
      $('#result').append(data);
      if (next) ajax_test(next);
    }
  });

$('#clicky_the_button').click(function(e){
  $('#result').html('');
  ajax_test('random', 'dummy');
});

I think you have an absolutely valid concern about the cookies, but I'm not sure there's a reasonable and foolproof solution to be built into the library for it. IMO, it's just up to the developer to use these tools sensibly.

Having said that, I guess my best suggestion would be a locked tempfile named after the session id. But, I would make it strictly configurable so those who are concerned about speed or those running on a system where locked tempfiles aren't an option (it could happen!) aren't stuck using that feature.

I also think that feature should probably be discussed as a separate add-on after this gets committed. I'm already nervous about all the changes causing hangups with the folks at Ellis. Maybe @philsturgeon will weigh in on this idea.

@philsturgeon
@dchill42

LOL @ tl;dr!

@GDmac is concerned about the fact that concurrent requests to the cookie driver for the same session will step on each others' data. (Same behavior as the former lib had.) It's a valid concern, but I'm not sure now is the time to try fixing it.

I made a relatively simple suggestion at the end of my last post (the paragraph before your name), but it might be worth getting more opinions before dropping in a new solution like that.

@philsturgeon
@dchill42

@philsturgeon Thanks. I bet you post that last comment from your iPhone 4S (like a BOSS) - I recognize the classic DYAC in "dealt" = "death". ;)

@GDmac As per Phil's suggestion, I'd like to hold off on resolving the concurrency issue. Please don't drop it - I think it's worthy of discussion, I'd just like to see it hashed out a little more and implemented as a separate pull.

@dchill42

Also, thanks again @GDmac for your continued testing and input - you've been an invaluable part of this process.

@philsturgeon

So, merge?

One last +1 from another Reactor guy. We have tests, and a few users confirming this all works nicely.

@pkriete @ericbarnes @katzgrau @seejohnrun

@alexbilbie

I've just updated our test install from work and it's working great here, so +1 from me

@philsturgeon philsturgeon merged commit 1e40c21 into from
@philsturgeon

Good work to everyone involved. This is merged now, and its looking good.

Thanks for keeping up with this @dchill42, it took a while but we got there.

If you have follow-up pull requests please make them and @ anybody you think is relevant, referencing any commits and pull requests (this one) that you need to.

@it-can

Is there a way to autoload the session library, cannot get it to work currently...

$autoload['drivers'] = array('session'); // not working

$autoload['libraries'] = array('session'); // working

@dchill42

@IT-Can Either of those should work. I just retested the drivers autoload to be sure nothing got broken, and it worked fine for me. Are you using a complete pull from dchill42/CodeIgniter@88b636b or bcit-ci/CodeIgniter@1e40c21?

There are changes to the Loader (including 'drivers' support) and the Driver libraries that are required for the Session driver to work properly. You can't just grab the Session library folder by itself.

If that's not the problem, I'll need a bit more detail to help.

@it-can

I used the latest develop of CI... I will test some more... But what I see, library works, drivers (autoload) cannot access $this->session

@dchill42

I just pulled bcit-ci/CodeIgniter@ff1c125 and made these changes locally:

application/config/autoload.php:

$autoload['drivers'] = array('session');

application/config/config.php:

$config['encryption_key'] = 'foobar';

application/views/welcome_message.php:

<div id="body">
    <p>Foo: <?php echo $foo; ?></p>
    <p>The page you are looking at is being generated dynamically by CodeIgniter.</p>

application/controllers/welcome.php:

public function index()
{
    $foo = $this->session->userdata('foo') + 1;
    $this->session->set_userdata('foo', $foo);
    $this->load->view('welcome_message', array('foo' => $foo));
}

...and it worked just fine. I also tried it with $config['sess_driver'] = 'native'; and that worked, too.

Are you getting an error message, or any other indication of what's going wrong?

@it-can

ok, I will try to reproduce my error...

@it-can

Ha found the problem... I use Sparks, it uses their own Loader.

@appleboy appleboy referenced this pull request
Closed

PHP session to CI session #1754

@nickl- nickl- referenced this pull request from a commit in kotive/CodeIgniter
@nickl- nickl- Unit tests load and subclass Session #353.
Unit tests to reproduce the problem raised in #353.
In addition to testing load by libary or load by driver we are also extending by prefixed sub-class.
Added new mack application folder.
Added Session Subclass mock.
Fix bootstrap to point APPPATH to the mock application folder.
Including IC_Session class more than once in the same unit test requires runInSeparateProcess and preserveGlobalState disabled
which also causes all knowledge of bootstrap to dissapear so we need to cater for that.
7102c63
@nickl- nickl- referenced this pull request from a commit in kotive/CodeIgniter
@nickl- nickl- Fix #353 Prefixed subclass Session extended.
Support for case sensitive FS
Allows for extended class to be in library or library/Session folders.
Solves problems extending Session as identified in #353.
22e3549
@nickl- nickl- referenced this pull request from a commit in kotive/CodeIgniter
@nickl- nickl- Fix #353 Allow Session extend with or without driver.
To extend the Session and drivers you would add same named drivers to the valid_drivers collection.
This allows you to still fall back to the standard session drivers eves thaugh we extended CI_Session.
Also helps solving problems discovered in #353.
898805b
@nickl-

#1769 fixes all issues and also improves flexibility

Complete support for extending CI_Session driver with custom application prefixed class.
Supports subclass loading through:

<?php

$CI->load->library('session'); 

/** or */ 

$CI->load->driver('session');

Allows for custom subclass from application folders libraries or libraries/Session
Complete support for case sensitive filesystems.
You can extend only the CI_Session class or provide same named drivers added to the CI_Session::valid_drivers collection or as collection in config as you wish. If no valid drivers are found for your subclass we can fall back to the default Session drivers.

Patch also removes the tracking of files included "loaded" instead now using PHP built in support through

<?php

get_included_files();

Complete with unit tests.

nJoy!

@dchill42

#1744 puts the library vs. driver testing in the Loader unit test, where I believe it belongs. For proper isolation in unit testing, the Session test shouldn't be trying to check Loader functionality.

@nickl-

@dchill42 Referring to your comment:

1744 puts the library vs. driver testing in the Loader unit test, where I believe it belongs. For proper isolation in unit testing, the Session test shouldn't be trying to check Loader functionality.

I am not sure if this is in response to my post as you are being sufficiently vague enough to not be of any use other than open up a foir amount of assumption like:

  • that it is a response to my post
  • that you are saying the pull request I proposed is redundant because 1744 already does this
  • That there isn't a problem to be addressed

Several users have already mentioned on the issue which mind you I agree has totally lost any kind of thread but never the less others including myself have experienced the same problems extending the new CI_Session. The current cod cannot work with prefixes because it is not considering them at all. This does not appear to be a problem with other drivers just the Sessions so I would assume we test for it there but if you suggest that it must be moved please give adequate instruction because I have no clue what you are trying to suggest.

What it really all boils down to:

  • Do you agree that there is a problem
  • Do agree that this proposed fix properly addresses the problem
  • Is there anything you would like to see added removed or changed.

If we can get this fixed as a matter of urgency as it effectively renders the v3.0 codebase useless then you would go a long way to show that this is a serious project and that you are serious about getting it ready Then we may just look at other areas where improvements are warranted or issues that may need some help. But if you really couldn't be bothered please let us know and we can make our efforts somewhere else where they are appreciated.

@dchill42

@nickl- Let's see what we can do with this.

  • Yes, my post relates to your post - nobody else is adding Loader tests to the Session test
  • The primary use in my post is establishing the relationship between the two requests so people responsible for merging can see that they are related
  • The secondary use in my post is pointing out why that test belongs where it does
  • You won't get very far telling people their contributions are of no use
  • Yes, part of your pull request is redundant
  • I do not agree that #1744 properly addresses the problem. There are a number of issues with the proposed code there, some of which will break other functionality

The 3.0 codebase is not useless, nor is the Session driver. You may have a problem using the driver the way you want to, which can certainly be addressed, but I'd recommend a more cooperative approach if you want your issue dealt with. If you think I'm not serious about CI or couldn't be bothered, you obviously haven't looked over my contributions, nor the length of time I've been working to help improve the framework.

So, if we're done with the personal attacks, maybe we can discuss the details of the problem you need fixed. I gather it's related to subclass prefix extensions. Are you trying to extend the library itself, or the driver classes, or both?

I'm guessing from the code you want to add to the Session driver copy of load_driver() that you're trying to address the driver class naming convention. That is not the place to do it. If you will describe the problem in more detail, I will work with you on an appropriate solution.

@nickl-

Hi @dchill42, you misunderstood, Yes the conversation started asking you if this was a response to my post, still think it was rather unclear but I did not mean to do any personal attacks. My apologies in any case. I was ending the conversation directed towards everybody concerned, I can see how that may be misinterpreted.

The tests are quite clear about what needs to happen on my side, I ran into another problem since where the Driver, that gets hacked in if not present a few lines before never gets loaded.

What happens is either the new Session folder gets added for the subclass and not for Session or the prefix is missing. Ideally it should also allow the subclass to remain in the library folder. I also doubt that anyone would care to extend both the drivers too, this may be a nice to have but most of the time you'll want to use the normal Cookie implementation etc.

If you know of a better location to address the issue I'd be keen to see that, this is just where it falls over and the most obvious place for me to start applying duct tape =)

Tx for the detailed response, I think there is going to be a lot of breaking still before things start coming right as I see lots of sticky spaghetti all over the show. Is there no specific rules for the autoloader, it's going to have a hard time if it just tries to do whatever.

What do you want to do next?

@dchill42

@nickl- It seems to me that the base Driver library needs to be aware of subclassing. I think it should search for drivers with the subclass name first (e.g. - MY_Session_cookie), and proceed to the base driver if that is not found (e.g. - Session_cookie). That would fix loading drivers when the library class gets extended, and would apply to all driver libraries.

The other key item you seem to have addressed is finding the driver library extension in the main libraries folder (e.g. - libraries/MY_Session.php) instead of the driver name subdirectory (e.g. - libraries/Session/MY_Session.php). On one hand, I think this shouldn't be necessary, as the extension for a driver lib should be placed in the subdirectory just like the base class. On the other hand, I want Session in particular to be completely backwards compatible, which means supporting legacy extensions that may be located in the libraries directory just like the former Session library was. So, that change does seem reasonable.

Regarding your replacement of _ci_loaded_files with get_included_files(), I'm not sure that's a good idea. _ci_loaded_files explicitly tracks libraries and drivers that have been loaded, whereas the PHP function will report all files included. Also, I'm not sure get_included_files() will return compatible file paths to be searched on all PHP installations. A mismatch due to variant directory separators would ruin the functionality. I expect this is why the Loader was implemented the way it was in the first place.

I see you added the '@' in front of the setcookie() call - was there a circumstance in which that made a difference? It doesn't hurt, but it didn't seem necessary, either.

And we have already discussed the unit tests. Obviously, I would prefer to have #1744 (which I mis-referenced for #1769 in my previous post) merged to handle those tests.

Did I miss anything? If there's some other aspect of your solution I have not mentioned here, please let me know.

If you don't object, I would like to implement the changes as outlined above in a new pull request, which I would reference from here and #1769. I would include unit tests for the Driver class to cover the load_driver() changes, and add tests to the Loader unit to cover the driver extension scenario. You could test it and make sure it fixes the problems you have encountered, and we can submit it in place of #1769. What do you say?

@nickl-

@dchill42 I am not sure I follow exactly what you want to do which might become clear after looking at your proposed code.

Some points motivators

  • +1 on Session backward compatibility/drop in replacement
  • +1 on whatever gets things resolved the quickest iow I am not married to my code and would support any viable solution.
  • -1 on always and forever, if PHP does it natively do it with PHP so you should rather ask how do we use get_included_files instead. The DIRECTORY_SEPARATOR should be used in any case, won't you agree? I also don't see how you would get anything else from realpath or stream_resolve_include_path that would not be compatible with get_included_files, how is this an issue? Since this is used to check if file was included before including it its safe to argue that you need all the information required. Keep in mind other loaders may also be employed in tandem and we should know the whole picture.
  • +1 on the drivers, instead of searching it looks in collection provided so you need to specify your sub classed driver but otherwise that is exactly how I've implemented the fix, you would see.

The @ sign is just another way of doing if (!headers_sent()) which certainly comes into play when running unit tests and there isn't any reason why the app should fail at this point, or do you disagree?

Shout if you want me to test anything or help with anything else...

@narfbg
Owner

Since I do receive multiple emails a day from this thread, I might as well clear up something - CI does NOT need to track included files. An included file doesn't necessarily represent a single library and vice-versa - a library doesn't represent a single file in 100% of the cases. require_once() and include_once() are way better than checking for file paths/names returned by get_included_files(). Why we need to know if a library is loaded or not is so that we don't try and look for it in 3+ different places every single time.

@nickl-

Hi @narfbg glad I triggered a response =) you probably should still read the other mails though.

You will find that special caching provisions have been provided for include and require of late which makes using the *_once methods less productive. Ideally stream_resolve_include_path should be used, for the same reasons, instead of realpath and the use of file_exists should definitely be avoided, so should any function that requires stat for that matter.

Therefor a definitive -1 on using *_once, bad idea.

@narfbg
Owner

@nickl- A quick glance at your and @dchill42's comments here shows that you're mostly discussing other pull requests here, which is hard to follow (consider this a suggestion to move the discussion :)) and the only new thing I can comment on is again +1 for @dchill42 - putting @ in front of setcookie() is pointless and only causes a few more CPU cycles to be wasted.

Otherwise, to be honest - I don't get why is there discussion here on file paths at all. I'd like to get more info on those special caching provisions that you're talking about, but as I said - we need to track loaded libraries and class names, not file paths.

@nickl-

@narfbg I responded at #1769 as requested

@ivantcholakov

@dchill42

There are all_userdata() and all_flashdata() methods, that are convenient at least for debugging. Could you also add a similar method all_tempdata() ? Within a controller context it is to be $this->session->all_tempdata().

Thank you, nice work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 30, 2011
  1. @dchill42
Commits on Aug 31, 2011
  1. @dchill42
  2. @dchill42

    Whitespace cleanup

    dchill42 authored
  3. @dchill42

    Better style guide compliance

    dchill42 authored
  4. @dchill42
  5. @dchill42
  6. @dchill42

    Missed whitespace on Driver

    dchill42 authored
Commits on Sep 1, 2011
  1. @dchill42
  2. @dchill42
Commits on Sep 12, 2011
  1. @dchill42
Commits on Jul 23, 2012
  1. @dchill42
  2. @dchill42
Commits on Jul 24, 2012
  1. @dchill42
Commits on Jul 30, 2012
  1. @dchill42
Commits on Jul 31, 2012
  1. @dchill42
  2. @dchill42
  3. @dchill42
  4. @dchill42
  5. @dchill42
Commits on Aug 8, 2012
  1. @dchill42
  2. @dchill42
Commits on Aug 12, 2012
  1. @dchill42
  2. @dchill42
Commits on Aug 13, 2012
  1. @dchill42

    Minor doc fixes

    dchill42 authored
Commits on Aug 27, 2012
  1. @dchill42
  2. @dchill42
Commits on Aug 28, 2012
  1. @dchill42
  2. @dchill42

    Minor session test improvements

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Commits on Aug 29, 2012
  1. @dchill42

    Extracted cookie database saves to shutdown and cleaned up code

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
  2. @dchill42

    Fixed defaults and database check, reverted redundant shutdown feature

    dchill42 authored
    Signed-off-by: dchill42 <dchill42@gmail.com>
Something went wrong with that request. Please try again.