Page MenuHomePhabricator

Remove error constants from IEditObject and use bad statuses without a result instead
Open, Needs TriagePublic

Description

Currently, if an EditConstraint (or certain code in EditPage that hasn't been converted to a constraint yet) fails, it returns a StatusValue with ok set to false, an error/warning message and a result that is one of the error constants in IEditObject.

However, this poses multiple issues:

  • If an edit constraint fails, two different error codes are returned: The IEditObject constant and an error message. This is unnecessary duplication and requires additional logic when creating the StatusValue object.
  • It is not easily possible to just "bubble up" statuses produced by other code because the result value has to be populated based on what the status actually is. This was a problem in this patch where I simplified the code so AuthorizationConstraint directly returns a PermissionStatus, which is then used in EditPage to throw an exception or show an error. A PermissionStatus is not supposed to hold a value, so the approach I had to use (silence phan and set a value) is quite hacky.
  • Logic that checks the result value of the returned status shouldn't be necessary. Instead of running all edit constraints at once and then using if/else or switch statements to perform various actions based on the result value, the edit constraints should instead be grouped into different constraint runners. If one of the constraints checked by a specific runner fails, ideally there should only be a one way to handle the failure (e.g. show an error/warning box, prevent the edit etc).

Apart from hook errors (which I still have to look into), the values are currently mainly used for the following:

  • EditPage::handleStatus: Throwing exceptions (PermissionsError, ReadonlyError etc) based on status values. The edit constraints could instead just throw the exceptions themselves.
  • ApiEditPage: Some errors are mapped to messages, which can be created in the constraints and mapped in ApiMessageTrait instead.
  • Also in ApiEditPage: AS_BLOCKED_PAGE_FOR_USER and AS_READ_ONLY_PAGE cause errors to be thrown. These errors are also thrown in EditPage, but according to the comment, These two cases will normally have been caught earlier, and will only occur if something blocks the user between the earlier check and the check in EditPage (presumably a hook). I haven't looked into the exact cause yet, but I think this would also be resolved by throwing the errors in the edit constraints instead.

There are also constants that indicate success (e.g. AS_SUCCESS_NEW_ARTICLE). I think it makes sense to keep those, and they can just continue to be returned by good statuses, e.g. in internalAttemptSave.

Event Timeline

(Not claiming the task yet since I currently work on other changes related to EditPage and edit constraints. If others think this sounds feasible and is a good idea, I plan to implement it though)