r58313 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r58312‎ | r58313 | r58314 >
Date:14:19, 29 October 2009
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
Switch to default skin if user session found without persistent creds. Bug 19048
Modified paths:
  • /trunk/phase3/includes/User.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/User.php
——@@ -3575,6 +3575,7 @@
35763576 }
35773577
35783578 protected function loadOptions() {
 3579+ global $wgCookiePrefix;
35793580 $this->load();
35803581 if ( $this->mOptionsLoaded || !$this->getId() )
35813582 return;
——@@ -3603,6 +3604,11 @@
36043605 $this->mOptionOverrides[$row->up_property] = $row->up_value;
36053606 $this->mOptions[$row->up_property] = $row->up_value;
36063607 }
 3608+
 3609+ //null skin if User::mId is loaded out of session data without persistant credentials
 3610+ if ( !isset( $_SESSION['wsToken'] ) && !isset( $_COOKIE["{$wgCookiePrefix}Token"] ) )
 3611+ $this->mOptions['skin'] = null;
 3612+
36073613 }
36083614
36093615 $this->mOptionsLoaded = true;

Follow-up revisions

RevisionCommit summaryAuthorDate
r60814Reverted r58313, pointless, does not fix the bug.tstarling23:36, 7 January 2010

Comments

#Comment by Tim Starling (talk | contribs)   02:45, 5 January 2010

I think you're misunderstanding that bug. I don't think you've fixed it, I think you've just made a second one instead.

Did you actually reproduce it before you tested the patch? Because we already have code to prevent this from happening, and I suspect that code is broken on Jidanni's wiki due to a hard-to-reproduce local configuration issue. For instance, if Jidanni deleted all his cookies (despite his claim that he did not) and then visited a cached page, this could occur.

#Comment by Freakolowsky (talk | contribs)   12:58, 5 January 2010

Well then i've had the same hard-to-reproduce local configuration issue, because i've had the same problem (and i too claim i didn't drop cookies), so yes it was reproduced, however i didn't think much of it before reading this bug report. After patch the skin plays nice for me and parsertests went trough 100% ... so i don't what bug i've made and i guess that code you had that does all this wasn't working for me.

So atm i don't know what should i fix ...

#Comment by Tim Starling (talk | contribs)   23:24, 5 January 2010

Well, if you can reproduce it, then you can answer the questions I asked Jidanni on the bug report.

If you think it works just fine, then I'd like to hear your explanation of how that is possible. Where do you think this phantom skin comes from, and why are no other preferences affected? $_SESSION['wsToken'] is always set when the user has a valid session, even if they don't have "remember me" checked, that seems to be the only reason this patch doesn't disable the skin preference for everyone who logs in without "remember me". But with CentralAuth, the user logs in purely from the global session, and wsToken will always be unset.

#Comment by Freakolowsky (talk | contribs)   16:42, 6 January 2010

g.d. ... had whole novel written then by mistake closed the window :S

Anyhow ... it has been a while since that patch was written and i tried it now without this patch on current revision ad it works normaly as far as i can tell. I tried reverting to a revision prior to this patch but i'd have to downgrade DB and i don't have the time to do that at this moment. As far as i can remember i tried to figure out why the skin remained while all other preferences remained unaffected but after a day i had no clue about it. I did however notice that somehow unlike Token, UserID was set to previous user when preferences were being loaded (loadOptions call) and that's why i was checking for it. The preferences then somehow got reverted to defaults but no until skin value got used. So in that case wiki skin got displayed incorrect, but if i did a print_r of preferences the value of the skin parameter was correct.

So i'd love to bu i just can't give you any insight on why or how. I'd gladly anwser your question because i dislike voodoo like this and in current state i'm unable to reproduce the error but again at that time on my setup it existed just as described and was mended by this patch ...

i'll remove this patch ...

#Comment by Freakolowsky (talk | contribs)   17:52, 6 January 2010

spoke too soon ... reproduced ... will post on bug page tomorrow.

Status & tagging log