On 1/4/08, Roan Kattouw roan.kattouw@home.nl wrote:
That's exactly what I did. What Brion couldn't really know is that the code in ApiChangeRights duplicated code only added to Userrights *later*. Anyway, I've re-added ApiChangeRights, which now handles disallowed groups more gracefully.
I think the sort of code duplication Brion is referring to is things like this. ApiChangeRights.php lines 51 to 59:
if(is_null($params['user']))
$this->dieUsage('The user parameter must be set', 'nouser');
$uName = User::getCanonicalName($params['user']);
$u = User::newFromName($uName);
if(!$u)
$this->dieUsage("Invalid username ``{$params['user']}''", 'invaliduser');
if($u->getId() == 0) // Anon or non-existent
$this->dieUsage("User ``{$params['user']}'' doesn't exist", 'nosuchuser');
This more or less entirely duplicates the content of UserrightsPage::fetchUser, and poorly. It doesn't support foreign users, at the very least. Instead it needs to be replaced with some call like $ur->fetchUser( $username );, followed by an error message if that's null. Similarly, lines 80 to 85 in ApiChangeRights.php are probably unnecessary:
if(empty($params['addto']) && empty($params['rmfrom']))
$this->dieUsage('At least one of the addto and rmfrom parameters must be set', 'noaddrm');
if(is_null($params['token']))
$this->dieUsage('The token parameter must be set', 'notoken');
if(!$wgUser->matchEditToken($params['token'], $uName))
$this->dieUsage('Invalid token', 'badtoken');
That should all be handled by the backend logic, which should just return whether it's successful or not. All of the saving logic, in particular, should be identical between the two. The API's entire processing routine should be something along the lines of
$ur = new UserrightsPage();
$dbw = wfGetDb(DB_MASTER);
$dbw->begin();
$ur->saveUserGroups($params['user'], $params['rmfrom'], $params['addto'], $params['reason']);
$dbw->commit();
or something not too different from that. The rest should all be display logic.