On 7/9/07, Andrew Garrett andrew@epstone.net wrote:
Hi all,
After a brief planning discussion with Tim a few days ago, I'm re-working the Title::userCan method. . . .
Definitely a good thing. But while you're at it, I have another proposal related to it:
[070706 14:52:03] <Simetrical> brion-office, by the way, I've decided I hate Title::userCan and want to scrap it in favor of User::hasPermission. So instead of $title->userCan( 'edit' ), $wgUser->hasPermission( 'edit', $title ). This reads better, allows it to be called on non-$wgUser users, and allows it to be called with different things for different permissions (e.g. $user1->hasPermission( 'block', $user2 ) ). [070706 14:52:07] <Simetrical> Sound good? [070706 14:53:26] <robchurch> Why not just pass a User to Title::userCan() ? [070706 14:53:36] <robchurch> User::hasPermission() sounds too much like User::isAllowed() [070706 14:53:47] <Simetrical> Er, that's what I meant. [070706 14:53:52] <Simetrical> Just add a second parameter to that. [070706 14:54:00] <Simetrical> I never could remember its bloody name. [070706 14:56:23] <robchurch> Simetrical: What about backwards compatibility with all the existing uses of Title::userCan(), specifically by hooks? [070706 14:56:33] <Simetrical> robchurch, obviously it's just going to be deprecated, not removed entirely. [070706 14:57:01] <robchurch> What specific use case is prompting you to want this change? [070706 14:57:14] <Simetrical> A discussion last night. [070706 14:57:23] <brion-office> Simetrical: hmmmmmmm [070706 14:57:35] <Simetrical> Also, I always hated the call syntax for that function, it's completely backwards and harder to understand at first glance. [070706 14:57:49] <Simetrical> You can't say that for $wgUser->isAllowed( 'edit', $title );
Note that on reflection, I figured it would be best to use a different function name to preserve reverse compatibility, although it's always a shame when a nice name like User::isAllowed has to be thrown out.
I had actually been pondering exactly the same sort of change to return values for errors as you propose, too, as part of the permissions cleanup. A couple of thoughts:
1) Returning something that evaluates to true on an error is sketchy. I thought about this and concluded it would probably be best, for readability, if a separate function were used to retrieve errors. Then the call syntax would be something like
if( !$user->isAllowed( 'edit' ) ) { $wgOut->addHtml( $user->errorMessage( 'edit' ) ); }
instead of the more inscrutable
if( ($error = $user->isAllowed( 'edit' ) ) !== true ) { $wgOut->addHtml( $error ); }
or whatever. (You can tell I like Python over Perl, can't you?) This also potentially saves callers who are, e.g., bots or maintenance scripts from having the error messages calculated when they might not need to read them.
2) I don't see that you're accounting for the fact that there may be multiple errors preventing the action. It's kind of annoying if you, e.g., try to edit and get told "Anonymous users don't have permission to edit, please log in", then log in and get told "Your IP address is blocked". This should be fixed while you're at it. A meta-message saying something like "You cannot do the action you have requested for multiple reasons. <ul><li>..." should be used when multiple errors occur.