Page MenuHomePhabricator

Factor out a backend from EditPage
Open, MediumPublic

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, DiscussionTools, 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 simulate conflicts, and couples tightly to the form parameters and internal EditPage fields when implementing a new conflict workflow.

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

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
SomeRandomDevelopermediawiki/coremaster+251 -184
SomeRandomDevelopermediawiki/coremaster+437 -172
SomeRandomDevelopermediawiki/coremaster+23 -36
SomeRandomDevelopermediawiki/coremaster+123 -31
SomeRandomDevelopermediawiki/coremaster+6 -89
SomeRandomDevelopermediawiki/coremaster+7 -16
SomeRandomDevelopermediawiki/coremaster+42 -177
SomeRandomDevelopermediawiki/coremaster+227 -21
SomeRandomDevelopermediawiki/coremaster+315 -162
SomeRandomDevelopermediawiki/coremaster+164 -183
SomeRandomDevelopermediawiki/coremaster+140 -142
SomeRandomDevelopermediawiki/coremaster+36 -38
SomeRandomDevelopermediawiki/coremaster+8 -20
SomeRandomDevelopermediawiki/coremaster+0 -11
SomeRandomDevelopermediawiki/coremaster+1 K -660
DannyS712mediawiki/coremaster+13 -5
DannyS712mediawiki/coremaster+185 -56
DannyS712mediawiki/coremaster+3 K -657
SomeRandomDevelopermediawiki/coremaster+2 -2
SomeRandomDevelopermediawiki/coremaster+47 -68
SomeRandomDevelopermediawiki/coremaster+135 -149
SomeRandomDevelopermediawiki/coremaster+495 -183
SomeRandomDevelopermediawiki/coremaster+319 -233
SomeRandomDevelopermediawiki/coremaster+221 -183
Krinklemediawiki/coremaster+6 -6
DannyS712mediawiki/coremaster+30 -16
DannyS712mediawiki/coremaster+1 -1
DannyS712mediawiki/coremaster+41 -19
DannyS712mediawiki/coremaster+38 -12
DannyS712mediawiki/coremaster+14 -14
DannyS712mediawiki/coremaster+43 -19
DannyS712mediawiki/coremaster+12 -87
DannyS712mediawiki/coremaster+125 -0
DannyS712mediawiki/coremaster+1 -1
DannyS712mediawiki/coremaster+4 -7
DannyS712mediawiki/coremaster+304 -65
DannyS712mediawiki/coremaster+244 -197
DannyS712mediawiki/coremaster+14 -14
DannyS712mediawiki/coremaster+46 -44
DannyS712mediawiki/coremaster+37 -33
DannyS712mediawiki/coremaster+13 -11
DannyS712mediawiki/coremaster+29 -11
DannyS712mediawiki/coremaster+2 -9
DannyS712mediawiki/coremaster+12 -11
DannyS712mediawiki/coremaster+56 -23
DannyS712mediawiki/coremaster+60 -43
DannyS712mediawiki/coremaster+170 -7
DannyS712mediawiki/coremaster+444 -48
DannyS712mediawiki/coremaster+25 -15
DannyS712mediawiki/coremaster+412 -69
DannyS712mediawiki/coremaster+15 -15
DannyS712mediawiki/coremaster+0 -8
DannyS712mediawiki/coremaster+53 -37
DannyS712mediawiki/coremaster+722 -29
DannyS712mediawiki/coremaster+83 -19
DannyS712mediawiki/coremaster+482 -30
DannyS712mediawiki/coremaster+59 -74
DannyS712mediawiki/coremaster+31 -62
DannyS712mediawiki/coremaster+1 K -157
DannyS712mediawiki/coremaster+21 -21
DannyS712mediawiki/coremaster+378 -36
DannyS712mediawiki/coremaster+23 -11
DannyS712mediawiki/coremaster+303 -22
DannyS712mediawiki/coremaster+386 -4
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

the biggest part of this will still be blocked by T251588, which hasn't had any progress - can I convince one of the subscribers or teams watching to take that up?

We've had some plans on the MediaWiki-Core-Platform-Team to work on T20654: EditPage.php needs rewrite: Separate DB and UI logic (the parent task for all of this work), but it has been deprioritized for now in favor of finishing the SUL3 work earlier. I am hopeful that we'll come back to it afterwards, but no promises yet.

Well its good to know that the WMF had the plans - are you (collectively) willing to help with code review of more non-trivial patches if I have the time to write the code?

Change #1053993 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Move section-check logic to ExistingSectionEditConstraint

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

Well its good to know that the WMF had the plans - are you (collectively) willing to help with code review of more non-trivial patches if I have the time to write the code?

I can't speak for the team, but I would personally be willing to review - however, the turn-around time will depend a lot on my current work load and involvment in other projects. If it's ok for you to "slow cook" these patches over weeks and months, that would work for me. That's how my own side projects tend to progress as well...

Same here. That said, I think I've been able to stay on top of code review requests recently, so I should be able to keep up with you as well. Thanks for working on it :)

Change #1053989 merged by jenkins-bot:

[mediawiki/core@master] Rename AutoSummaryMissingSummaryConstraint

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

Change #1053992 merged by jenkins-bot:

[mediawiki/core@master] Move AS_REVISION_WAS_DELETED handling into constraint

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

Change #1053993 merged by jenkins-bot:

[mediawiki/core@master] Move section-check logic to ExistingSectionEditConstraint

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

Change #1055582 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Hard deprecate public access to EditPage::internalAttemptSave()

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

Change #1055584 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] EditFilterMergedContentHookConstraint: fix docs typo

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

Change #1055584 merged by jenkins-bot:

[mediawiki/core@master] EditFilterMergedContentHookConstraint: fix docs typo

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

Change #1055617 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] DRAFT: EditPage internalAttemptSaveToConstraint()

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

Change #1055582 merged by jenkins-bot:

[mediawiki/core@master] Hard deprecate public access to EditPage::internalAttemptSave()

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

@matmarex would you mind taking a look at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1055617 which is where I envision this going next - should that patch be split up into multiple smaller steps? Or does it work as a single patch?

Change #1138139 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] editpage: Rename internalAttemptSavePrivate back to internalAttemptSave

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

Change #1138139 merged by jenkins-bot:

[mediawiki/core@master] editpage: Rename internalAttemptSavePrivate back to internalAttemptSave

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

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

[mediawiki/core@master] editpage: Split constraint runner creation into separate methods

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

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

[mediawiki/core@master] editpage: Introduce EditPageStatus

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

Change #1234555 merged by jenkins-bot:

[mediawiki/core@master] editpage: Split constraint runner creation into separate methods

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

Change #1234556 merged by jenkins-bot:

[mediawiki/core@master] editpage: Introduce EditPageStatus

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

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

[mediawiki/core@master] editpage: Move some helper methods to a new service

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

Change #1237600 merged by jenkins-bot:

[mediawiki/core@master] editpage: Move some helper methods to a new service

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

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

[mediawiki/core@master] Convert IEditConstraint to an abstract class

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

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

[mediawiki/core@master] editpage: Use PHP type declarations for save-related attributes and methods

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

Change #1247183 merged by jenkins-bot:

[mediawiki/core@master] editpage: Use PHP type declarations for save-related attributes and methods

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

Change #1246886 merged by jenkins-bot:

[mediawiki/core@master] editpage: Convert IEditConstraint to an abstract class

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

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

[mediawiki/core@master] EditPage: Make two properties non-nullable

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

Change #1251323 merged by jenkins-bot:

[mediawiki/core@master] EditPage: Make two properties non-nullable

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

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

[mediawiki/core@master] Move page saving logic out of EditPage into a separate class

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

FlaggedRevs […] create an EditPage object in order to extract UI components that they wish to reuse.

Maybe it technically uses EditPage only for this, but it relies on the internals of page editing a lot more: it directly accesses a lot of form fields set on editing (including its own checkbox, but also core ones at least to recognize any kinds of undoing edits – revert, rollback, manual revert –, and likely other things I forgot about). Removing the fake requests may break these accesses, so FlaggedRevs should be tested carefully, and fixed as necessary.

I think that comment is outdated because FlaggedRevs doesn't create an EditPage anymore since https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FlaggedRevs/+/1251359, it only uses existing instances from hooks

The code you removed didn’t “extract UI components [it] whish[ed] to reuse”, so either the description is inaccurate, or it meant something else (which either still exists or – more likely – has long been removed).

In any case, my comment is AFAIK up-to-date.

The code you removed didn’t “extract UI components [it] whish[ed] to reuse”, so either the description is inaccurate, or it meant something else (which either still exists or – more likely – has long been removed).

Yes, my point was primarily that it is outdated because FlaggedRevs isn't creating an EditPage anymore at all.
Based on the age of this task and the last comments I wouldn't expect anything to be up-to-date anyway.

In any case, my comment is AFAIK up-to-date.

In that case, feel free to update the description so it won't get buried beneath other comments :)

Change #676794 abandoned by Hashar:

[mediawiki/core@master] EditPage::importFormData should set context request

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

Change #1055617 abandoned by Hashar:

[mediawiki/core@master] DRAFT: EditPage internalAttemptSaveToConstraint()

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

Change #1055617 restored by Thcipriani:

[mediawiki/core@master] DRAFT: EditPage internalAttemptSaveToConstraint()

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

Change #676794 restored by Thcipriani:

[mediawiki/core@master] EditPage::importFormData should set context request

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

Change #649916 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] Factor out a backend from EditPage

Reason:

Outdated patch, conflicts with ongoing EditPage refactoring – sorry

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

Change #1055617 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] DRAFT: EditPage internalAttemptSaveToConstraint()

Reason:

Outdated patch, conflicts with ongoing EditPage refactoring – sorry

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

Change #676794 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] EditPage::importFormData should set context request

Reason:

Outdated patch, conflicts with ongoing EditPage refactoring – sorry

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

Change #1249476 merged by jenkins-bot:

[mediawiki/core@master] Move page saving logic out of EditPage into a separate class

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

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

[mediawiki/core@master] PageEdit: Introduce ISaveAttemptDataHolder

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

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

[mediawiki/core@master] PageEdit: Remove UnicodeConstraint and move check to EditPage instead

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

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

[mediawiki/core@master] PageEdit: Remove tempUserCreateActive check when updating watchlist

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

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

[mediawiki/core@master] PageEdit: Use a MessageSpecifier for the submit button label

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

Change #1283816 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Remove UnicodeConstraint and move check to EditPage instead

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

Change #1283819 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Remove tempUserCreateActive check when updating watchlist

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

Change #1283820 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Use a MessageSpecifier for the submit button label

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

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

[mediawiki/core@master] PageEdit: Replace getContextTitle() with a PageReference

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

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

[mediawiki/core@master] PageEdit: Only require a PageIdentity instead of an Article

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

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

[mediawiki/core@master] [WIP] PageEdit: Provide defaults for most input parameters

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

Change #1283975 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Replace getContextTitle() with a PageReference

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

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

[mediawiki/core@master] EditPage: Move minor edit and bot flag logic to PageEdit

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

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

[mediawiki/core@master] Rename EditPageStatus to PageEditStatus and move to PageEdit namespace

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

Change #1286527 merged by jenkins-bot:

[mediawiki/core@master] Rename EditPageStatus to PageEditStatus and move to PageEdit namespace

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

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

[mediawiki/core@master] PageEdit: Move wpAntispam check from edit constraint back to EditPage

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

Change #1284011 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Only require a ProperPageIdentity instead of an Article

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

Change #1284048 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Provide defaults for most input parameters

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

Change #1285935 merged by jenkins-bot:

[mediawiki/core@master] EditPage: Move minor edit and bot flag logic to PageEdit

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

Change #1287501 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Move wpAntispam check from edit constraint back to EditPage

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

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

[mediawiki/core@master] EditPage: Streamline missing/blank summary logic

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

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

[mediawiki/core@master] PageEdit: Remove summary from result

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

Change #1303000 merged by jenkins-bot:

[mediawiki/core@master] PageEdit: Remove summary from result

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

Change #1302317 merged by jenkins-bot:

[mediawiki/core@master] EditPage: Streamline missing/blank summary logic

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

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

[mediawiki/core@master] EditPage: Move section anchor logic to PageEdit

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

Change #1303002 merged by jenkins-bot:

[mediawiki/core@master] EditPage: Move section anchor logic to PageEdit

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

Change #1283110 abandoned by SomeRandomDeveloper:

[mediawiki/core@master] PageEdit: Introduce ISaveAttemptDataHolder

Reason:

In favor of 1306004

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

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

[mediawiki/core@master] [WIP] PageEdit: Introduce PageEditAttemptState

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