Page MenuHomePhabricator

Fix Content validation - something to replace Content::isValid()
Open, Needs TriagePublic

Description

There are two validation functions for Content objects, Content::isValid() which returns a boolean, and Content::prepareSave(), which returns a Status object. And the default implementation of prepareSave just calls isValid() and returns a generic error. Throughout the editing process, the first time either is called is in WikiPage::doModify(), which is at the very end of the save process. Most problematically, is that functions like Content::preSaveTransform() and Content::getParserOutput() will have already been called on the possibly-invalid Content.

If you look at JsonContent, you'll see that it skips doing anything in those functions if isValid() returns false - that's silly, it shouldn't have to do that. Furthermore, the EventLogging extension (which many others have copied) uses an EditFilterMerged hook to be able to display errors to users, even though that validation code is inside the Content object

Instead, I propose we continue to use Content::prepareSave(), but call it much earlier, before PST is invoked for the first time. It would probably go after the EditPage::toEditContent() calls in different code paths (preview, diff, save).

Event Timeline

After looking at this again, prepareSave() won't work because the parameters it wants are not easily accessible in EditPage...

So I think a new function should just be passed the current Title (or LinkTarget) and return a Status object.

@Bawolff also pointed out to me that throwing a MWContentSerializationException exception in the ContentHandler::unserializeContent function can also be used to display errors.