[drupal-devel] [bug] PHP session settings to relocate to support
multiple sites
Goba
drupal-devel at drupal.org
Sat Feb 19 12:15:55 UTC 2005
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: tangent
Updated by: Goba
-Status: fixed
+Status: patch
Attachment: http://drupal.org/files/issues/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.
Goba
Previous comments:
------------------------------------------------------------------------
February 14, 2005 - 08:51 : tangent
Attachment: http://drupal.org/files/issues/settings-session.patch (1.71 KB)
As discussed in this issue [1], 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.
[1] http://drupal.org//node/2974
------------------------------------------------------------------------
February 14, 2005 - 10:42 : Goba
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.
------------------------------------------------------------------------
February 14, 2005 - 15:56 : kbahey
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)?
------------------------------------------------------------------------
February 14, 2005 - 19:04 : tangent
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.
------------------------------------------------------------------------
February 14, 2005 - 21:14 : kbahey
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.
------------------------------------------------------------------------
February 14, 2005 - 22:06 : tangent
Attachment: http://drupal.org/files/issues/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 [2] 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.
[2] http://www.php.net/manual/en/ini.php#ini.list
------------------------------------------------------------------------
February 15, 2005 - 08:51 : tangent
Attachment: http://drupal.org/files/issues/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 [3] until this
issue is resolved positively.
[3] http://drupal.org//node/17288
------------------------------------------------------------------------
February 15, 2005 - 09:07 : tangent
Attachment: http://drupal.org/files/issues/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.
------------------------------------------------------------------------
February 15, 2005 - 10:35 : tangent
Attachment: http://drupal.org/files/issues/settings-session5.patch (6.99 KB)
This patch removes a setting which crept in and probably shouldn't be
there.
------------------------------------------------------------------------
February 16, 2005 - 22:55 : Dries
I guess this is a good thing. Unless someone objects, I'll commit this
to HEAD.
------------------------------------------------------------------------
February 17, 2005 - 20:40 : Anonymous
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.
------------------------------------------------------------------------
February 18, 2005 - 00:07 : Dries
tangent: patch complains about your patch being malformed. I tried
fixing it but to no avail. Could you try resubmitting your patch?
------------------------------------------------------------------------
February 18, 2005 - 00:14 : tangent
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.
------------------------------------------------------------------------
February 18, 2005 - 07:23 : tangent
Attachment: http://drupal.org/files/issues/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.
------------------------------------------------------------------------
February 18, 2005 - 15:15 : kbahey
+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!
------------------------------------------------------------------------
February 18, 2005 - 19:45 : Dries
Great patch. Committed to HEAD. Thanks.
--
View: http://drupal.org/node/17303
Edit: http://drupal.org/project/comments/add/17303
More information about the drupal-devel
mailing list