r78104 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78103‎ | r78104 | r78105 >
Date:23:18, 8 December 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Added name and sessionId methods to user object - now anonymous users have an automatically generated persistent (for 30 days at a time via a cookie) session identifier. Use mediaWiki.user.sessionId() to get it.
Modified paths:
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/Resources.php
——@@ -326,6 +326,7 @@
327327 'mediawiki' => array(
328328 'scripts' => 'resources/mediawiki/mediawiki.js',
329329 'debugScripts' => 'resources/mediawiki/mediawiki.log.js',
 330+ 'dependencies' => array( 'jquery.cookie' ),
330331 'debugRaw' => false
331332 ),
332333 'mediawiki.util' => array(
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -225,7 +225,62 @@
226226 * User object
227227 */
228228 function User() {
 229+
 230+ /* Private Members */
 231+
 232+ var that = this;
 233+
 234+ /* Public Members */
 235+
229236 this.options = new Map();
 237+
 238+ /* Public Methods */
 239+
 240+ /*
 241+ * Generates a random user session ID (32 alpha-numeric characters).
 242+ *
 243+ * This information would potentially be stored in a cookie to identify a user during a
 244+ * session. It's uniqueness should not be depended on.
 245+ *
 246+ * @return string random set of 32 alpha-numeric characters
 247+ */
 248+ function generateSessionId() {
 249+ var id = '';
 250+ var seed = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXTZabcdefghiklmnopqrstuvwxyz';
 251+ for ( var i = 0, r; i < 32; i++ ) {
 252+ r = Math.floor( Math.random() * seed.length );
 253+ id += seed.substring( r, r + 1 );
 254+ }
 255+ return id;
 256+ }
 257+
 258+ /*
 259+ * Gets the current user's name.
 260+ *
 261+ * @return mixed user name string or null if users is anonymous
 262+ */
 263+ this.name = function() {
 264+ return mediaWiki.config.get( 'wgUserName' );
 265+ };
 266+
 267+ /*
 268+ * Gets the current user's name or a random session ID automatically generated and kept in
 269+ * a cookie.
 270+ *
 271+ * @return string user name or random session ID
 272+ */
 273+ this.sessionId = function () {
 274+ var name = that.name();
 275+ if ( name ) {
 276+ return name;
 277+ }
 278+ var sessionId = $.cookie( 'mediaWiki.user.sessionId' );
 279+ if ( typeof sessionId == 'undefined' || sessionId == null ) {
 280+ sessionId = generateSessionId();
 281+ $.cookie( 'mediaWiki.user.sessionId', sessionId, { 'expires': 30, 'path': '/' } );
 282+ }
 283+ return sessionId;
 284+ };
230285 }
231286
232287 /* Public Members */

Follow-up revisions

RevisionCommit summaryAuthorDate
r78344Fixed issue in r78104 where jquery.cookie wouldn't load properly. Changed the...tparscal23:51, 13 December 2010
r78347Fixes r78104 much better than r78344 did. By adding jquery.cookie to the load...tparscal00:02, 14 December 2010
r78365Followup r78104: set the cookie unconditionally, so its expiry is renewed eve...catrope11:09, 14 December 2010
r830371.17wmf1: Merge revs to mediawiki.js required by ArticleFeedback: r78104, r78...catrope19:23, 1 March 2011

Comments

#Comment by Nikerabbit (talk | contribs)   08:09, 9 December 2010

I hope this doesn't mean every user will get a cookie just by viewing a page. It was the most annoying thing when I was still filtering cookies.

#Comment by Catrope (talk | contribs)   15:26, 9 December 2010

No, it's done on-demand.

#Comment by P858snake (talk | contribs)   12:16, 9 December 2010

I vaguely remember a discussion somewhere but don't remember the outcome of it, where iirc we were discussing about not wanting items like cookies unless they were needed, What benefit would it even bring for a anon to have a cookie?

#Comment by Catrope (talk | contribs)   15:38, 9 December 2010 
+				$.cookie( 'mediaWiki.user.sessionId', sessionId, { 'expires': 30, 'path': '/' } );

If you want this to really work properly, you should renew the cookie every time it's used, i.e. do the $.cookie call unconditionally.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:40, 9 December 2010

Well, there are really 2 use cases here...

1. Giving anonymous users an ID of some sort so we can treat them more like logged in users (ArticleFeedback use case) 2. Giving anonymous users an ID of some sort so we can cluster together information about a single page view (ClickTracking use case) 3. Maybe something in between?

I think the sessionId probably should be more of a #2 use case, so maybe we could support other use cases here as well - ideas?

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:25, 9 December 2010

We are already using code on every page that implements this on it's own. What I'm trying to avoid is having multiple simultaneous implementations of the same thing. As it is, the sessionId is only generated and stored when it's needed (as Catrope said, it's on demand). This can replace code that's been written in ClickTracking and ArticleFeedback, and prevent this from being written yet again in many other systems to come.

#Comment by Catrope (talk | contribs)   21:42, 13 December 2010

As Krinkle pointed out on IRC, this revision causes $.cookie is undefined errors, because mediawiki is a magical module that can't depend on anything but jquery. This behavior should probably be documented somewhere.

#Comment by Catrope (talk | contribs)   21:51, 13 December 2010

It gets worse: for some reason the fact that mw depends on cookie causes cookie to never be loaded. Presumably this is because something, somewhere sets cookie to ready state without actually loading it. Krinkle confirms this is fixed when that dependency is removed.

IRC log for chatting about a workaround: 

[22:49] <Krinkle> RoanKattouw: What do you suggest, wrapping a mw.using() call in mw.user.sessionId() ?
[22:49] <Krinkle> mw.loader.using *
[22:49] <RoanKattouw> Probably a good idea, yes
[22:49] <Krinkle> k
[22:49] <Krinkle> I'm currently working on documention though
[22:49] <RoanKattouw> But that means sessionId() can't be called before the loader is properly set up
[22:49] <Krinkle> will see later or Trevor could do it
[22:49] <RoanKattouw> So you'd have to document that it won't work in that case
[22:49] <RoanKattouw> I'll paste this log onto CR
[22:50] <Krinkle> k
#Comment by Trevor Parscal (WMF) (talk | contribs)   22:12, 13 December 2010

Hmm, maybe we should just use the browser's own cookie stuff - it's not really that complex to support all browsers.

#Comment by Catrope (talk | contribs)   22:15, 13 December 2010

That'd just duplicate code. Wrapping in using() and documenting the fact that the function will explode when called from within the mediaWiki module itself sounds easier to me.

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:50, 13 December 2010

Agh - sorry missed your brilliant idea there... Will do.

#Comment by Trevor Parscal (WMF) (talk | contribs)   23:51, 13 December 2010

Well, using won't work cause it is callback-based, and I need to return the value... I used load() though, which works just fine.

#Comment by Trevor Parscal (WMF) (talk | contribs)   00:03, 14 December 2010

r78347 fixes this by using mediaWiki.loader.load just after declaring mediaWiki - adding it to the initial payload.

#Comment by He7d3r (talk | contribs)   19:20, 17 February 2011

Just to be sure: this isn't live on Wikimedia projects is it?

I was tring to use mw.user.name() but it seems to be undefined.

#Comment by Catrope (talk | contribs)   15:03, 18 February 2011

No, it's not live.

Status & tagging log