Page MenuHomePhabricator

Factor out a backend from EditPage
Open, NormalPublic

Description

Currently, EditPage contains the web UI tightly integrated with edit authorisation and edit conflict merging. ApiEditPage wraps EditPage by making a fake web UI request. And VisualEditor and LiquidThreads wrap ApiEditPage by making a second fake request.

I propose introducing a new backend containing edit authorisation, automatic conflict merging, and the call through to WikiPage::doEditContent(), separated from the web UI. EditPage, ApiEditPage, VisualEditor, SemanticForms and LiquidThreads would call this backend.

The interface would be similar to EditPage::internalAttemptSave(), except with relevant request state injected instead of being fetched from global or class static data. updateWatchlist() would be moved to the UI layer.

A code comment notes:

@todo FIXME: This interface is TERRIBLE, but hard to get rid of due to various error display idiosyncrasies. There are also lots of cases where error metadata is set in the object and retrieved later instead of being returned, e.g. AS_CONTENT_TOO_BIG and AS_BLOCKED_PAGE_FOR_USER. All that stuff needs to be cleaned up some

So we would try to fix that.

The rationale for this work is to encourage further development of editing customisations and extensions. It may be possible to make MCR editing (T107595) easier to implement.

Usage survey in WMF deployed extensions

SemanticForms (a.k.a. PageForms) creates an EditPage object in order to execute the backend EditPage::internalAttemptSave().

JsonConfig and LiquidThreads create an EditPage object in order to execute the combined frontend and backend EditPage::edit(), with hooks to customise its operation.

ProofreadPage subclasses EditPage in order to override the UI and content object creation.

VisualEditor and LiquidThreads invoke ApiEditPage to perform an edit.

The following extensions hook the backend part of EditPage (EditFilter, EditFilterMergedContent, EditPage::attemptSave, EditPage::attemptSave:after):

  • AbuseFilter
  • ConfirmEdit
  • EventLogging
  • Gadgets
  • JsonConfig
  • ProofreadPage
  • Scribunto
  • SpamBlacklist
  • TitleBlacklist
  • Translate
  • UploadWizard
  • WikiEditor
  • Wikidata

FlaggedRevs and VisualEditor create an EditPage object in order to extract UI components that they wish to reuse. MassMessage calls an EditPage static method for the same reason.

TwoColConflict subclasses EditPage in order to override the conflict UI.

Request access deeper in the stack

The proposed backend is not useful if the caller still has to set up a fake request to fool deeper parts of the stack. As such, the following methods that access RequestContext or WebRequest are concerning:

  • The context title is accessed from WikitextContent::isCountable(), which is called during edit with $title = null
  • Various things access the IP address from the request context, such as User::getBlockedStatus(), User::pingLimiter(), RecentChange::checkIPAddress() and AbuseFilter.
  • User::getBlockedStatus() also accesses the request cookies and X-Forwarded-For header
  • Probably other things

Event Timeline

tstarling created this task.Feb 9 2017, 3:48 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 9 2017, 3:48 AM

I would love to see this happen. BTW, the "FlaggedRevs and VisualEditor create an EditPage object in order to extract UI components that they wish to reuse." is one of the most disgusting pieces of our architecture and I would love for the ability to get the displayed checkboxes and their labels as data, not HTML strings – but I imagine that would involve breaking the current EditPageBeforeEditChecks hook.

I would love to see this happen. BTW, the "FlaggedRevs and VisualEditor create an EditPage object in order to extract UI components that they wish to reuse." is one of the most disgusting pieces of our architecture and I would love for the ability to get the displayed checkboxes and their labels as data, not HTML strings – but I imagine that would involve breaking the current EditPageBeforeEditChecks hook.

For additional context: VE does this because FlaggedRevs uses a hook to inject an extra checkbox. If it was just the minor edit and watchlist checkboxes from core, we'd be happy to duplicate a little bit of code in VE (which we in fact did until we found the FlaggedRevs problem).

TTO awarded a token.Feb 13 2017, 2:05 AM
Ebe123 added a subscriber: Ebe123.Feb 17 2017, 3:01 AM

I've made an attempt to refactor EditPage in https://gerrit.wikimedia.org/r/#/c/338276/.
This goes a bit beyond factoring out a backend as I've also rewritten the UI part based on an MVC pattern.

The backend for saving is the EditAttempt class.
The Edit class models an edit, the Editor class controls the editing and the EditView class contains the UI elements.
The EditFacilitator class transforms the form data into an Edit object and mediates the call to EditAttempt for an Editor object.

The API only needs to make an Edit object and inject it into an EditAttempt object to make the save. Same for extensions needing only backend access.

I've also:

  • passed the hook status as edit status rather than having a hook error
  • required hooks to pass an error when returning a fatal status
  • removed the wpUltimateParam cutover.
tstarling triaged this task as Normal priority.Apr 20 2017, 5:22 AM