Page MenuHomePhabricator

Move human-readable error messages for edit attempts to the EditConstraint classes
Open, Needs TriagePublic

Description

As noted here by @SomeRandomDeveloper: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1090544/comment/47886e8c_81e16373/

Many of the IEditConstraint classes do not set the human-readable error messages in the StatusValue objects they return from their getLegacyStatus() methods, or set them incorrectly (with missing or wrong message parameters).

This works because the messages are not used in these cases. EditPage recognizes some specific error codes (set in the StatusValue object's value), and some specific IEditConstraint classes, and generates its own error messages from scratch.

I think the constraint classes should always generate the error messages, and EditPage should just display them instead of duplicating this code. There are some comments in EditPage suggesting that this was the original vision of the refactor, just not quite implemented:

EditPage will still need to special-case some errors (e.g. edit conflicts, or errors that can be supressed like BrokenRedirectConstraint), but the "default" error message should be generated by the constraint class, not by EditPage. This would let us also remove a third duplicate of this code in ApiEditPage:

(BTW, getLegacyStatus() should be renamed to just getStatus(). It's not "legacy" in the usual meaning of the word, I don't understand where that name came from.)

Event Timeline

Change #1113225 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] editpage: Make PageSizeConstraint provide its own error message

https://gerrit.wikimedia.org/r/1113225

I wrote a patch to do one of the constraints, as an example of what I mean, and added a few notes about the task in the code. I'm not planning to write more patches here at the moment (although I would be happy to review any).

I picked a rather simple one, and the patch is still gnarly. This will be tedious work. Fair warning. :)

Change #1113875 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Make BrokenRedirectConstraint provide its own error message

https://gerrit.wikimedia.org/r/1113875

Thanks for creating this task and the example patch. I checked out the patch to understand what these changes do exactly. To see what this refactoring would look like for other edit constraints, I created a dependent patch to move the warning message for BrokenRedirectConstraint to the constraint class as well.

Based on my understanding, there is less to refactor in this case: For example, in contrast to $tooBig, the $brokenRedirect property in EditPage has to stay since it is being used for adding wpIgnoreBrokenRedirects to the html. Also, there is no implementation for this message in ApiEditPage.

Change #1113225 merged by jenkins-bot:

[mediawiki/core@master] editpage: Make PageSizeConstraint provide its own error message

https://gerrit.wikimedia.org/r/1113225

Change #1113875 merged by jenkins-bot:

[mediawiki/core@master] editpage: Make BrokenRedirectConstraint provide its own error message

https://gerrit.wikimedia.org/r/1113875

Change #1176231 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] Merge all redirect constraints into one

https://gerrit.wikimedia.org/r/1176231

Change #1176231 merged by jenkins-bot:

[mediawiki/core@master] editpage: merge all redirect constraints into one

https://gerrit.wikimedia.org/r/1176231

Change #1176774 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Make DefaultTextConstraint provide its own error message

https://gerrit.wikimedia.org/r/1176774

Change #1177331 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Make MissingCommentConstraint provide its own error message

https://gerrit.wikimedia.org/r/1177331

Change #1176774 merged by jenkins-bot:

[mediawiki/core@master] editpage: Make DefaultTextConstraint provide its own error message

https://gerrit.wikimedia.org/r/1176774

Change #1177331 merged by jenkins-bot:

[mediawiki/core@master] editpage: Make MissingCommentConstraint provide its own error message

https://gerrit.wikimedia.org/r/1177331

Change #1225181 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Make section edit constraints provide their own error messages

https://gerrit.wikimedia.org/r/1225181

Change #1225184 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] RedirectConstraint: use warnings instead of errors

https://gerrit.wikimedia.org/r/1225184

Change #1225181 merged by jenkins-bot:

[mediawiki/core@master] editpage: Make section edit constraints provide their own error messages

https://gerrit.wikimedia.org/r/1225181

Change #1225184 merged by jenkins-bot:

[mediawiki/core@master] RedirectConstraint: use warnings instead of errors

https://gerrit.wikimedia.org/r/1225184

Currently, getLegacyStatus either duplicates logic that is also present in checkConstraint or reads a status that was set by checkConstraint.

I think we could get rid of the method altogether after this task is done and have checkConstraint return a StatusValue instead of the two strings (CONSTRAINT_PASSED/CONSTRAINT_FAILED). We could then just check StatusValue::isOK/isGood and there wouldn't be a need to temporarily store the status int or the result in the class anymore.

Change #1225706 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Move article recreation error logic to edit constraint

https://gerrit.wikimedia.org/r/1225706

Change #1226380 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Use error message from PermissionStatus for rate limit errors

https://gerrit.wikimedia.org/r/1226380

Change #1225706 merged by jenkins-bot:

[mediawiki/core@master] editpage: Move article recreation error logic to edit constraint

https://gerrit.wikimedia.org/r/1225706

Change #1226380 merged by jenkins-bot:

[mediawiki/core@master] editpage: Reuse error message and exceptions from AuthorizationConstraint

https://gerrit.wikimedia.org/r/1226380

Change #1228241 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Merge getLegacyStatus into checkConstraint

https://gerrit.wikimedia.org/r/1228241

Currently, getLegacyStatus either duplicates logic that is also present in checkConstraint or reads a status that was set by checkConstraint.

I think we could get rid of the method altogether after this task is done and have checkConstraint return a StatusValue instead of the two strings (CONSTRAINT_PASSED/CONSTRAINT_FAILED). We could then just check StatusValue::isOK/isGood and there wouldn't be a need to temporarily store the status int or the result in the class anymore.

I've decided to implement this now rather than later since other changes would take longer otherwise due to the code duplication.

Change #1228241 merged by jenkins-bot:

[mediawiki/core@master] editpage: Merge getLegacyStatus into checkConstraint

https://gerrit.wikimedia.org/r/1228241

Change #1229201 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Display error messages produced by PageUpdater

https://gerrit.wikimedia.org/r/1229201

Change #1229207 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Introduce RevisionDeletedConstraint

https://gerrit.wikimedia.org/r/1229207

Change #1229201 merged by jenkins-bot:

[mediawiki/core@master] editpage: Display error messages produced by PageUpdater

https://gerrit.wikimedia.org/r/1229201

Change #1229207 merged by jenkins-bot:

[mediawiki/core@master] editpage: Introduce RevisionDeletedConstraint

https://gerrit.wikimedia.org/r/1229207

Change #1236797 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] editpage: Use warnings instead of errors where applicable

https://gerrit.wikimedia.org/r/1236797

Change #1236797 merged by jenkins-bot:

[mediawiki/core@master] editpage: Use warnings instead of errors where applicable

https://gerrit.wikimedia.org/r/1236797