Last modified: 2010-05-15 15:59:40 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 11613 - session.save_handler being over-ridden
session.save_handler being over-ridden
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
1.11.x
All All
: High normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-09 15:25 UTC by Nathan Gordon
Modified: 2010-05-15 15:59 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch to check for save_handler being set to user instead of !file (592 bytes, patch)
2007-10-09 15:25 UTC, Nathan Gordon
Details
Patch to not override session.save_handler (709 bytes, patch)
2008-10-29 17:49 UTC, Allen
Details

Description Nathan Gordon 2007-10-09 15:25:28 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?
Comment 1 Allen 2008-10-29 17:48:22 UTC
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',
Comment 2 Allen 2008-10-29 17:49:20 UTC
Created attachment 5484 [details]
Patch to not override session.save_handler
Comment 3 Nathan Gordon 2008-10-29 18:04:28 UTC
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.
Comment 4 Chad H. 2009-04-10 14:53:54 UTC
Fixed in r49370. 

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links