PHP session settings to relocate to support multiple sites [#17303]

As discussed in this issue, it would be desirable to move the session settings into /sites/default/settings.php so that subsites can have better control over them. One of the advantages of the site specific settings.php file is that it will never get overwritten during upgrades and having these settings here should prove to be more friendly.

I have created a patch which moves most of the PHP session settings from .htaccess to /sites/default/settings.php with the exception of "session.auto_start" because it must not, as far as I know, be modified.

I have also added 2 additional commented settings which I suspect are often needed as they were in my case.

Comment	File	Size	Author
#16	Drupal.fix-trans-sid.patch	1.04 KB	Gábor Hojtsy
#16
#13	settings-session6.patch	6.8 KB	tangent
#13
#8	settings-session5.patch	6.99 KB	tangent
#8
#7	settings-session4.patch	7.1 KB	tangent
#7
#6	settings-session3.patch	7.13 KB	tangent
#6
#5	settings-session2.patch	2.27 KB	tangent
#5
	settings-session.patch	1.71 KB	tangent

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 14 February 2005 at 09:42

Please, if this gets committed, leave a comment line in the .htaccess file at least, indicating that other session related settings are located in settings.php, so those in the know of current Drupal behaviour will find their way around.

Comment #2

kbahey CreditAttribution: kbahey commented 14 February 2005 at 14:56

This patch is very important. It does what I wanted to do in the other issue pointed to: make Drupal as much as possible independant of the underlying PHP configuration.

Can you please also add ini_set calls for these values to make sure PHPSESSID does not end up in the URL:

php_flag session.use_trans_sid off
php_flag session.use_cookies on
php_flag session.use_only_cookies on

Also, remove the line
+ini_set('session.cookie_lifetime', 2000000);

Since the other patch takes care of this value. Or should we make everything consistent and use just one way to do it (settings.php instead of session.inc)?

Comment #3

tangent CreditAttribution: tangent commented 14 February 2005 at 18:04

I added a line for "session.use_only_cookies" but did not for the others because the values you specified are the defaults. The only reason they should need to be changed is if they are defined elsewhere.

I had to disable "session.use_trans_sid" on my installation so perhaps my webhost enables this in php.ini or elsewhere but I'm not sure if the setting should be included. Anyone else have thoughts?

I see no reason to change the cookie lifetime value from what it has always been. The site admin can change the value in settings.php if they want a different value. I suppose this eliminates the need to set the value elsewhere.

Comment #4

kbahey CreditAttribution: kbahey commented 14 February 2005 at 20:14

I think that your way of doing the session is better, because, as I said in the other thread, it transcends the cookie/session discussion, and makes Drupal independant of how PHP defaults are set (global php.ini, local php.ini, .htaccess, ...etc.), and regardless of how the ISP setup their PHP defaults.

Regardless of whether what the defaults for parameters are, we need to set them, and be sure that for Drupal, they are set this way.

So, go ahead and add everything there, with a comment where appropriate (e.g. for cookie_lifetime, say "This is the number of seconds to keep a user logged in without asking them for a password.", ...etc.)

My only concern is this http://us4.php.net/manual/en/function.ini-set.php (see comment from 14 Sept 2004, and specially this http://us4.php.net/manual/en/ini.php#ini.list). Different PHP versions allow/disallow ini_set for various values. Supporting the various version idiosyncracies in the code with conditionals is a nightmare.

Comment #5

tangent CreditAttribution: tangent commented 14 February 2005 at 21:06

File	Size
settings-session2.patch	2.27 KB

The URL session management settings not mandatory for Drupal to function. Therefore they are optional. That is why I have included them commented out.

Further, the comment that you referenced about the changeability of session.use_trans_sid seems to be misleading. The official documentation states the following version compatibility.

PHP_INI_ALL in PHP <= 4.2.3.
PHP_INI_PERDIR in PHP <= 4-cvs.
Available since PHP 4.0.3.

I also don't think documenting each setting (like cookie lifetime) is necessary because I've added a reference in this patch to the settings documentation. If the admin is not familiar with all the settings they can go read up.

Comment #6

tangent CreditAttribution: tangent commented 15 February 2005 at 07:51

File	Size
settings-session3.patch	7.13 KB

Here's an updated patch which adds a comment to .htaccess noting the relocation of session settings. I also added the settings.php discovery rules which are documented in bootstrap.inc to the file doxygen block since this info will likely be of more use to the admin here.

Finally, it now seems silly to maintain a separate issue for updating the comments from # to c-style (unless this patch is refused) so I've included that change here and will close issue 17288 until this issue is resolved positively.

Comment #7

tangent CreditAttribution: tangent commented 15 February 2005 at 08:07

File	Size
settings-session4.patch	7.1 KB

This patch changes the configuration rule instructions to use "sites/default" instead of a variable since that value is hardcoded and would be more confusing to end users. The instructions are also slightly reworded.

Sorry for the noise. Hopefully this is the final patch.

Comment #8

tangent CreditAttribution: tangent commented 15 February 2005 at 09:35

File	Size
settings-session5.patch	6.99 KB

This patch removes a setting which crept in and probably shouldn't be there.

Comment #9

Dries CreditAttribution: Dries commented 16 February 2005 at 21:55

I guess this is a good thing. Unless someone objects, I'll commit this to HEAD.

Comment #10

(not verified) CreditAttribution: commented 17 February 2005 at 19:40

Isn't the 'session.save_path' directive unnecessary and misleading? Because Drupal uses session.save_handler = user (our sessions are stored in the database), that option won't do anything. AFAIK, save_path only matters when using save_handler = files.

Also, it appears that there are functions we might be able to use instead of ini_set (for shared servers that disable ini_set): session_cache_expire, session_cache_limiter, session_set_cookie_params, session_set_save_handler. session_cache_expire has a minimum version of 4.2.0, so that might be a problem for users of Drupal's minimum PHP version (4.1.0). Perhaps the ini_set would be better for that particular setting.

I think this would be a worthwhile patch... and the more hardcoded PHP settings we remove from .htaccess, the better. Many shared hosts are using CGI-based PHP (rather than mod_php) and configure their settings in a separate place (not .htaccess), which forces end-users to manually copy over the settings they need. Using a runtime-based approach like this would eliminate that problem.

Comment #11

Dries CreditAttribution: Dries commented 17 February 2005 at 23:07

tangent: patch complains about your patch being malformed. I tried fixing it but to no avail. Could you try resubmitting your patch?

Comment #12

tangent CreditAttribution: tangent commented 17 February 2005 at 23:14

Will do. I was planning to look into addressing the concerns of the previous poster with respect to the session functions first. I'll submit an update as soon as I can.

Comment #13

tangent CreditAttribution: tangent commented 18 February 2005 at 06:23

File	Size
settings-session6.patch	6.8 KB

Here's an updated patch.

1. I removed the unnecessary "session.save_handler" setting.
2. I uncommented the "session.use_only_cookies" and "session.use_trans_sid" settings to force cookie based (as opposed to url based) sessions by default. This seems like the most recommended method but perhaps there is a reason it was not in there before?
3. I did NOT replace the ini_set function with the setting specific functions because I'm uncertain what the implications are and I cannot find any documentation that corroborates the anonymous posters arguments. The ini_set function works on my shared server environment so if the anonymous poster (please log in next time so we know who is commenting) cannot clarify his/her concerns then this patch may be final.

Comment #14

kbahey CreditAttribution: kbahey commented 18 February 2005 at 14:15

+1 for this patch as it is.

The setting use_only_cookies is a must. Some time ago my host changed to PHP SuExec (not mod_php) and I started seeing ?PHPSESSID=blah in the URLs, and had to put it manually in .htaccess.

This way we are sure that Drupal handles this explicitly in a way that makes do things our way not how the defaults are set at the hosting server.

Good work!

Comment #15

Dries CreditAttribution: Dries commented 18 February 2005 at 18:45

Great patch. Committed to HEAD. Thanks.

Comment #16

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 19 February 2005 at 12:15

File	Size
Drupal.fix-trans-sid.patch	1.04 KB

Turning on use only cookies, but also turning on trans sid is not logical, since only cookies will be observed for session id information, but session IDs will still be added to the URLs on the page. The attached patch makes trans sid 0, and aligns the setting values.