On Tue, Aug 12, 2008 at 8:34 AM, Andrew Garrett <andrew(a)epstone.net> wrote:
Generally speaking, the permissions code does need to
be smoothed
over. It's currently a mishmash of older code which uses the old
sequence of checking read-only, blocks, rights individually, some
newer code which does the same, code which does some subset of the
above, and code which uses getUserPermissionsErrors, with various
hacks and so on to make it work (such as the count() stuff you
removed). Certainly, if I decided today to make the changes to
permissions code that I made back last year, I would have done them
differently (and perhaps heeded Simetrical's advice to move the checks
away from being methods of the Title object to being methods of the
User object, with the title becoming optional).
Another ugly thing is the errors format, with arrays of arrays. Tim
has pointed out that a much cleaner interface would be an associative
array of key => params. Then we could use if( isset( $errors['foo'] )
) instead of some ungodly mess like $condition = false; foreach(
$errors as $error ) if( $error[0] == 'foo' ) { $condition = true;
break; } if( $condition ). Similarly we can use array_merge to merge
errors instead of having to roll our own function, it's easier to
unset errors, etc.
Alternatively, we might create a custom error class, but I'm not sure
what the added utility of that would be in any immediate sense. What
do other software projects do for reporting when multiple errors
occur? Other than just reporting the first, of course, or giving a
generic error message. For that matter, what kind of return format do
they use for permissions checks altogether?
Maybe we should consider introducing another set of functions and
using those. The current interface is remarkably terrible for the
amount of discussion it received.
getUserPermissionsErrors, for compatibility with the
older userCan
interface, returns a permissions error on Special-page titles.
Which makes no sense, generally speaking. We should probably change
this so that it only returns errors for known bad operations like edit
or move. We don't need to preserve old interfaces if they're stupid.
:) Of course, someone somewhere might be relying on the fact that
attempting 'squizzle' on special pages will fail, but I doubt it. At
worst it should fail when the operation is actually attempted, if it
really makes no sense. There's not much you *can* do to a special
page, since it has no presence in the database and that's the only
thing that's changeable persistently across requests.
If another set of functions is introduced, though, it might be a good
idea to *not* try to rewrite the current functions as wrappers for it.
That way we avoid crufty nonsense like this. Build a good interface
first, and only then consider whether and how to rewrite the old
stuff. Maybe I'll try it this time -- I just rewrote another crufty
old interface. :) (Maybe sometime I'll get back to writing features
and fixing bugs, too . . .)
This is true, but it's always checked along with
permissions (and was,
IIRC, in the original userCan method before I touched it, so
backwards-compatibility is relevant), so it makes sense to put it in
the getUserPermissionsErrors method, so a separate check doesn't have
to be done every time we check permissions.
It's not always checked with permissions. If a user doesn't have
permission to do something, that means that for policy reasons, they
are not allowed to do it. If the wiki is read-only, the user *does*
have permission to do the action, just it's impossible at the moment
(but should become possible shortly). For examples of where this
makes a difference, consider links like "edit" or "block". We should
and do display those if the database is read-only, but not if the user
doesn't have permission. Similarly, if a user tries to save an edit,
a permissions error results in a simple error page -- a read-only
error returns a preview page with a message at the top asking the user
to resubmit. It makes no sense to lump the two types of errors
together.