Page MenuHomePhabricator

Clean up Article::doRollback
Closed, ResolvedPublic


Author: ayg

Nonfunctional beginnings of a patch

Article::doRollback is very messy right now, with several problems:

  1. It provides no way for scripts to override permissions checks.
  1. It uses nonstandard permissions checks and inadequate error return codes, which a) duplicate code in Title::getPermissionsErrors (probably, in some respect, incorrectly), and b) force Article::rollback to check permissions anyway using Title::getPermissionsErrors so it can display a proper error. Combined, this code duplication probably results in more than one scenario in which you can rollback through the API but not the standard UI, and vice versa.

Proposed fix:

  • Break into two functions, doRollback() and commitRollback() (or whatever -- name suggested by BrokenArrow). doRollback() should perform all permissions checks and then call commitRollback() to do the actual work. commitRollback() should only ensure that the database is not read-only.
  • Remove the pass-by-reference $resultDetails (this isn't C(++) here), and change the return value to an array of errors, like some of the editing logic already does.

The very beginnings of a (not yet functional) patch are attached.

Version: 1.12.x
Severity: enhancement




Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:07 PM
bzimport set Reference to bz12615.


I'll probably get around to this by Friday.

Done in r28903. The API still has to be updated, and other functions have to be refactored similarly. I'm on that.