Last modified: 2014-09-24 01:12:34 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T12663, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 10663 - Linker::userToolLinks and friends should not require a user ID
Linker::userToolLinks and friends should not require a user ID
Status: REOPENED
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: code_quality
  Show dependency treegraph
 
Reported: 2007-07-22 20:40 UTC by Aryeh Gregor (not reading bugmail, please e-mail directly)
Modified: 2014-09-24 01:12 UTC (History)
4 users (show)

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


Attachments
Proposed patch (7.01 KB, patch)
2007-07-22 20:40 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
Details
Proposed patch (much simpler than previous, no less efficient) (12.82 KB, patch)
2008-01-24 03:45 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
Details

Description Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-07-22 20:40:51 UTC
Created attachment 3941 [details]
Proposed patch

Currently the series of functions related to Linker::userToolLinks (userLink, userToolLinks, userToolLinksRedContribs, userTalkLink, blockLink) accept two arguments: a user ID and a user name.  This is ridiculous, because not a single one of them actually needs the user ID, and getting it typically requires an extra database query.  This makes it unnecessarily difficult to use them in new places like on log pages (bug 10655).  They should be changed to accept a User object, and the User object should be made more efficient so isAnon() doesn't require a database query if a username is provided.

A couple of notes on the patch:

1) I've just converted the calling convention for the existing functions completely.  This may be confusing; personally I prefer it to an Obj syntax like we have for makeLink and friends, but that could be used instead if desired, and the existing versions deprecated.

2) The User changes require that User::isIP be accurate.  I assume it is, because no users are allowed to register if it returns true, so there should be no rows in the database if it's true, so the current way of checking the database to see if the user is an IP is pointless for User::isLoggedIn.

3) I declared a bunch of @private functions public, because some of them are in practice, and the rest are useful to other classes, so there's no reason for them to be private.

I've tested the patch reasonably, browsing around histories and RC and so forth, making sure I got a mix of anons, registereds, people with/without talk pages, etc.
Comment 1 Rob Church 2007-07-22 20:57:41 UTC
What's the point in accepting a User object that's much heavier than a string and a boolean, which is how we are treating the identifier. I consider it would be much more effective to just transform the identifier parameter into a boolean. As it stands, this would maintain backwards compatibility with callers.
Comment 2 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-07-22 21:45:10 UTC
(In reply to comment #1)
> What's the point in accepting a User object that's much heavier than a string
> and a boolean, which is how we are treating the identifier.

It's not *much* heavier, since it's never initialized.  A little bigger in memory, but not so much as to make an actual difference (it's not actually initialized after all).  The reason, in any case, is that it's more consistent to use User objects everywhere, since then the caller can generate the object once and use it for everything, rather than having to keep strings and id's both handy where they're needed, and probably a User object too.  That's part of the clarity of using objects, passing around information coherently instead of in bits and pieces that vary from function to function.  The only reason to do the contrary is to maintain more obvious backward compatibility, but that's also a code style issue, and I don't think it's so terribly important (the documentation is pretty clear).

Note that the ID parameter is unused in two of the five functions regardless (userTalkLink, blockLink).  That's the kind of pointless and confusing thing that normally generates compiler warnings.  Using User objects instead of boolean/name would also fix that.
Comment 3 Matt 2007-07-23 08:02:06 UTC
The boolean method wouldnt look very good in cases like the following:

Linker::userToolLinks($name.contains("."), $name)

If it's just a boolean, we dont even need a parameter - we can just do a anon check immediately. The user object is still a lot more robust - we can extract much more information while keeping the parameters the same.
Comment 4 Brion Vibber 2007-07-23 15:31:47 UTC
Function exists for use in long lists of DB info; User object is currently relatively heavyweight, geared to live-user info, and not suitable for this.
Comment 5 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-07-23 19:54:44 UTC
Some benchmarking shows the object-based method takes possibly twice the time as the current method, and it's unacceptably slow already: 3-4 s for 5000 runs of the current userToolLinks.  I've committed the efficiency fixes to User.php but will still tweak this as Rob suggests, making the first parameter a boolean instead of an int.
Comment 6 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-01-24 03:45:39 UTC
Created attachment 4573 [details]
Proposed patch (much simpler than previous, no less efficient)

Here's a patch that just deprecates the parameter and does nothing else.  I'm not going to bother committing it unless I can establish that I can actually avoid database queries somewhere, however.
Comment 7 Brion Vibber 2008-02-01 21:35:43 UTC
This patch appears to be something to do with section edit link formatting.
Comment 8 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-02-01 21:39:06 UTC
Comment on attachment 4573 [details]
Proposed patch (much simpler than previous, no less efficient)

Brilliant, and it doesn't even have the desired changes anywhere in it.  I guess I uploaded the wrong file.
Comment 9 p858snake 2011-04-30 00:09:48 UTC
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Comment 10 Sumana Harihareswara 2011-11-09 03:14:49 UTC
Removing "patch" keyword since it seems there is no patch attached here currently.
Comment 11 Daniel Friesen 2011-11-09 06:08:12 UTC
Please note that it seams that the reason we have a user and user id separate in these methods rather than trying to use other methods to determine what is anon. Is because on imported edits where we have a username and an id of 0 these entries are supposed to link to contribs rather than to the user's userpage even if that user was later created since they didn't really make the edit.

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


Navigation
Links