On 1/4/08, Brion Vibber <brion(a)wikimedia.org> wrote: When I did the work on Userrights, I did my best to ensure that the rights-changing code was as modular as possible (it's all abstracted into backend subroutines, which don't make any reference to the form itself). These, therefore, may be very easily moved out into wherever they should be, or you could even instantiate the Userrightsform class and call the methods on that (after making them public). -- Andrew Garrett

On 1/4/08, Roan Kattouw <roan.kattouw(a)home.nl> wrote: 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.

On 1/4/08, Roan Kattouw <roan.kattouw(a)home.nl> wrote: Duplicating logic is less robust, and leads to more bugs. The correct way to do it, which I believe is used elsewhere in the core code now to support the API, is to have error return codes of some kind, not to just check for all the errors in the display logic.

Brion Vibber schreef: Could you be more specific as to how the implementation could be changed to be of acceptable quality? Also, why don't you want changerights in the API? We have move, rollback and all the other good stuff already. Roan Kattouw (Catrope)

Max Semenik schreef: True. With the autopromote system VasilievVV added recently, there's much use for it any more. Roan Kattouw (Catrope)

Roan Kattouw wrote: The more privileged operations are in an undermaintained secondary interface, the more likely we are to have security problems. As such I should warn that I currently would not accept a ChangeRights api module at all, no matter how it's implemented. In theory though, API and UI modules should *both* make clean calls to backend classes. An ideal API or UI module should never touch the database, for instance, nor check permissions. -- brion vibber (brion @ wikimedia.org)