Last modified: 2014-09-24 01:12:34 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.
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.
(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.
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.
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.
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.
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.
This patch appears to be something to do with section edit link formatting.
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.
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Removing "patch" keyword since it seems there is no patch attached here currently.
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.
