Clarification on session handling functions

I haven't had a chance to test the patch yet, but +1 for documentation, as well as the clarity of renaming the functions. My gut feeling is this should be "normal" priority rather than minor, since it introduces documentation to previously undocumented functions, and adds clarity all around.

Looks a huge improvement :)

What about the changes to bootstrap.inc and cache.inc that were in the original patch?

Some minor comments on the documentation:

First a general comment - the documentation for the first 2 functions describes what they do in general, whereas for the last 2 it describes the Drupal implmentation specifically. I've suggested specific changes for better consitency below.

1. Suggest changing

* The session save handlers:

to

* The user-level session handlers

2. I don't understand the documentation for _sess_open() - I thought that http://uk3.php.net/manual/en/function.session-save-path.php is what is used to set the save path, and anyway the save path is not used in Drupal...

3. _sess_close(): Suggest changing

* This function is used to close the current session. Because 

to

* This function will be called by PHP to close the current session. Because 

4. _sess_read(): Suggest changing

* This function retrieves

to

* This function will be called by PHP to get the current user's session data. As well as retrieving the session from the database, it also ...

5. _sess_write(): Suggest changing

* This function writes the current session to the database.

to

* This function will be called by PHP to store the current user's session.

6. Suggest changing

* This function should not be used directly.

to

* This function should not be called directly.

7. Suggest removing

 Session variables should instead be 
* accessed via the $_SESSION superglobal.

from the documentation of _sess_open() and _sess_close().

Also some of the documentation lines are longer than the 80 character limit (coding standards).

Sorry, rather more suggested changes here than I originally intended!

Thanks for update, looking good :)

A few final comments, pretty minor stuff really:

- At the top, maybe replace "The user-level session save handlers:" with "The user-level session storage handlers:" (which is how PHP doc describes them)

- The list of 4 handlers omits 2 - gc and destroy_sid. Obviously the last 2 *can* be called directly.

- "and are called by PHP" -> "and are called automatically by PHP"

- The comment "This function should not be called directly." actually still looks a bit unnecessary to me in the context of _sess_open() and _sess_close() - I mean, calling these directly basically does nothing, is therefore harmless, and also there is no reason that anyone would want to in a Drupal app. Suggest removing these 2 instances.

- "This function will be called by PHP to retrieve the current user's session data from the database. It also loads the current user's appropriate roles into the user object."
--> - "This function will be called by PHP to retrieve the current user's session data, which is stored in the database. It also loads the user's roles into the user object."

- "This function will be called by PHP to store the current user's session to the database." -> "This function will be called by PHP to store the current user's session, which Drupal saves to the database."

Hope this is helpful, have not actually tested the fairly trival code changes but all looks OK.

Holy batman! Very very nice work! Keep going.

Excellent patch! Committed to CVS HEAD. Thanks jscheel and gpk.

Oops, meant to mark them as 'fixed'.

Fantastic, thanks Dries :) and especially jscheel for doing the bulk of the donkey-work :) :)

Sorry to reopen this, but why not marking sess_destroy_sid and sess_gc private? These are also session callback functions registered by session_set_save_handler(), and they should not (and are not) called directly.

Also the PHPdoc of _sess_open() and _sess_close() is not working - can't see why. And the documentation parser is stripping out an opening bracket and leaving session_set_save_handler) in a few places. See http://api.drupal.org/api/file/includes/session.inc/7. Hardly earth-shattering but a bit odd. sess_gc (or _sess_gc) is also missing documentation.

@jsheel: I would like to see sess_gc an sess_destroy_sid marked as private. Those are also callbacks of the PHP session handler and should not be used directly. Instead, you can use the public PHP function session_destroy to kill a session. You should not have to trigger a garbage collection manually.

@jsheel: could you please also fix the last lines of your docbook comments? You are missing the leading space in all of them:

+*/    
+function _sess_close() {

should be:

+ */    
+function _sess_close() {

@23: Thanks Damien, I couldn't see that for the looking!

Here is an extension for this.

This new patch:

- privatize _sess_gc and _sess_destroy_sid
- prefix all public functions with drupal_ to clarify their use. session_regenerate is drupal_session_regenerate, session_count is drupal_session_count, session_destroy_uid is drupal_session_destroy_uid, session_save_session is now drupal_save_session
- fixed the doxygen from session.inc

Makes sense to rename the functions to drupal_ - consistent with similar functions elsewhere in core. The other fixes look fine. No time to run tests now though so could do with another review.

Apparently not all the chunk got in the previous patch.

Committed this to CVS HEAD. Thanks!

I'm marking this as 'code needs work' because we need to update the upgrade instructions in the handbook.

This may have broken the file tests that got in this morning (and used session_save_session).

It did. Patch.

Thanks catch.

Committed. Thanks!

marking back as "code needs work" for the handbook additions Dries requested in #28

Tests no longer broken (I presume...)

http://api.drupal.org/api/file/includes/session.inc/7 looks much more sane now, however the Description section of the PHPdoc needs fixing. Attached patch does that, and also corrects documentation of http://api.drupal.org/api/function/drupal_session_count/7 (it looks as if the patch from #293421: Documentation for sess_count is wrong did not apply properly). Also some further comments cleanup.

Is it worth improving the naming of drupal_save_session() - which despite its name does not actually save the session. How about drupal_set_session_save() (and maybe pair it with a new drupal_get_session_save()) ??

I presume that in #28 Dries was referring to the module update instructions? http://drupal.org/node/224333.

i'll chime in that drupal_save_session() is a pretty bad name for a function that doesn't save the session.

This patch no longer applies because of the REQUEST_TIME changes. Will need a quick re-roll.

Let's deal with the naming of drupal_save_session() after the reroll. It might be a bad name, but I need to investigate where the function is used, and why.

@38: IIRC drupal_save_session() is not used in core (except in http://api.drupal.org/api/function/_sess_write/7 - to check whether to save session data - and in a test).

The PHPdoc of http://api.drupal.org/api/function/drupal_save_session/7 is helpful and the handbook page referenced there (http://drupal.org/node/218104) gives further info.

Ah, so that page will also need updating in due course as the function is no longer called session_save_session() ..!

By the looks at the latest patch a lot has changed since then. Does this need to be fixed anymore?

Much session stuff has changed. Reopen as needed.