Last modified: 2010-05-15 15:59:40 UTC
Created attachment 4245 [details] Patch to check for save_handler being set to user instead of !file I do web hosting and use a load balancer to make my life simpler. Unfortunately php sessions don't work by default unless tweaks are made. I've choosen to go the route of using session_mysql http://websupport.sk/~stanojr/projects/session_mysql/. This is an extension to PHP that allows php to save session data in a mysql database transparently to apps. Unfortunately I see that includes/GlobalFunctions.php is explicitly setting the save handler to files for what I assume is ease of use for some users, it is making my life more complicated. And while I can tweak the couple of mediawiki installs that I manage, I have more users who manage their own code which I can't muck with. Given the comments around it, I would assume there are people out their without sane save_handler settings (being set to user globally for example). Unfortunately I don't believe that mediawiki setting it unconditionally to something that works most of the time is the right solution. If sessions are broken on a user's host then they need to fix it. They could easily drop in an ini_set in their Local Settings file to set it to files if their environment is broken. Unless I'm missing something this would make sense to get rid of or at least make non-default at some point. Theoretically if it is set to user, and it was done correctly it would still work as expected. The only cases setting to files fixes are when the user has a broken session setup. Heck, even a check in the install to verify that sessions are properly setup and a warning that they must be working to have media wiki work. My apologies if I'm sounding bitter. I'm just not sure I see the usefulness in mucking with the session settings given to us by the server setup. This also includes the bits I see shortly after this setting the cookie params. Could this become an option that is only turned on if sessions fail miserably? Possibly checking for user being set instead of != files?
I agree with this assessment. I honestly don't know what people are thinking when they release software like this that overrides things that administrators like myself may have setup purposely; we have a fully load balanced system, currently not using memcached, and our sessions *must* be stored in a database or other central location that all our load balanced servers can access. The patch that is provided will not solve the problem universally, as it will not solve the problem for me; we use a session handler 'mysql' with an installed PHP extension that provides the handler itself; neither 'user' nor 'files' will handle the issue. I have provided a new patch which simply removes the entire "else" block from wfSetupSession(); this will allow memcached support to continue as-is, but if memcached is not being used, then do nothing at all with the current session handler. All PHP installs use 'files' by default, this is only changed by people that know what they are doing and have changed it for a good reason. The diff for the patch is below, I will attach it after the post. ---------------- --- includes/GlobalFunctions.php.old Wed Oct 29 17:28:58 2008 +++ includes/GlobalFunctions.php Wed Oct 29 17:37:49 2008 @@ -2428,10 +2428,6 @@ global $wgSessionsInMemcached, $wgCookiePath, $wgCookieDomain, $wgCookieSecure, $wgCookieHttpOnly; if( $wgSessionsInMemcached ) { require_once( 'MemcachedSessions.php' ); - } elseif( 'files' != ini_get( 'session.save_handler' ) ) { - # If it's left on 'user' or another setting from another - # application, it will end up failing. Try to recover. - ini_set ( 'session.save_handler', 'files' ); } $httpOnlySafe = wfHttpOnlySafe(); wfDebugLog( 'cookie',
Created attachment 5484 [details] Patch to not override session.save_handler
I'm fine with this patch. I would tend to agree that setting something absolutely, that had to be configured manually in the first place, is a bad practice. I'm also a user of session_mysql, and the original intent of my patch was to only set the session handler to files *if* the handler was set to user. Granted, this could be just as bad for someone who had taken the time to write a good session handler in PHP. Hopefully we can see this integrated at some point in the near future.
Fixed in r49370.
