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 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

ProjectBranchLines +/-Subject
mediawiki/coremaster+13 -5
mediawiki/coremaster+3 K -658
mediawiki/coremaster+12 -87
mediawiki/coremaster+125 -0
mediawiki/coremaster+1 -1
mediawiki/coremaster+4 -7
mediawiki/coremaster+304 -65
mediawiki/coremaster+244 -197
mediawiki/coremaster+14 -14
mediawiki/coremaster+46 -44
mediawiki/coremaster+37 -33
mediawiki/coremaster+13 -11
mediawiki/coremaster+29 -11
mediawiki/coremaster+2 -9
mediawiki/coremaster+12 -11
mediawiki/coremaster+56 -23
mediawiki/coremaster+60 -43
mediawiki/coremaster+170 -7
mediawiki/coremaster+444 -48
mediawiki/coremaster+25 -15
mediawiki/coremaster+412 -69
mediawiki/coremaster+15 -15
mediawiki/coremaster+0 -8
mediawiki/coremaster+53 -37
mediawiki/coremaster+722 -29
mediawiki/coremaster+83 -19
mediawiki/coremaster+482 -30
mediawiki/coremaster+59 -74
mediawiki/coremaster+31 -62
mediawiki/coremaster+1 K -157
mediawiki/coremaster+21 -21
mediawiki/coremaster+378 -36
mediawiki/coremaster+23 -11
mediawiki/coremaster+303 -22
mediawiki/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

Change 633001 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Introduce EditContraint system, migrate unicode check to constraint

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

The phase 1 from the comment above looks like a good idea. Please don't create a massive patch to do everything all at once, let's take baby steps:

  • Patch to establish the infrastructure for the constraints
  • Patch to migrate one or two constraints as a demo

After that we can evaluate again how it looks in the code. Will likely need to have some level of integration testing for the migrated code prior to migrating to the constraint system.

Okay, the first patch is one of the two simplest - the unicode check. Once next once after that will probably be the antispam check that fails if $request->getText( 'wpAntispam' ); is something other than '' - that patch will introduce the factory service for constraints that need dependencies injected, in this case a logger.

Change 633001 merged by jenkins-bot:
[mediawiki/core@master] Introduce EditConstraint system, migrate unicode check to constraint

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

Change 635334 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add EditConstraintFactory, migrate SimpleAntiSpamConstraint

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

Change 635334 merged by jenkins-bot:
[mediawiki/core@master] Add EditConstraintFactory, migrate SimpleAntiSpamConstraint

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

Change 635586 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: Move spam checks to SpamConstraint

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

Change 635588 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: add EditConflict log channel

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

Change 635588 merged by jenkins-bot:
[mediawiki/core@master] EditPage: add EditConflict log channel

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

Change 635586 merged by jenkins-bot:
[mediawiki/core@master] EditPage: Move spam checks to SpamRegexConstraint

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

Change 635598 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Reorder some of the checks in EditPage

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

Change 635598 merged by jenkins-bot:
[mediawiki/core@master] Reorder some of the checks in EditPage

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

Change 635783 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: Migrate to EditPermissionsConstraint

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

Change 635783 merged by jenkins-bot:
[mediawiki/core@master] EditPage: Migrate more checks to constraint

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

Change 636384 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Reorder some of the code in EditPage::internalAttemptSave

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

Change 636384 merged by jenkins-bot:
[mediawiki/core@master] Reorder some of the code in EditPage::internalAttemptSave

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

Change 636700 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add PageSizeConstraint and ChangeTagsConstraint

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

Change 636788 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Clean up EditPageConstraintsTest

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

Change 636788 merged by jenkins-bot:
[mediawiki/core@master] Clean up EditPageConstraintsTest

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

Change 636700 merged by jenkins-bot:
[mediawiki/core@master] Add PageSizeConstraint and ChangeTagsConstraint

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

Change 636965 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Tweak logging for EditConstraintRunner

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

Change 638060 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: Prep for EditConflictManager refactor

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

Change 636965 merged by jenkins-bot:
[mediawiki/core@master] Minor tweaks to edit constraints

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

Change 638061 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: Factor out more constraints

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

Change 638061 merged by jenkins-bot:
[mediawiki/core@master] EditPage: Factor out more constraints

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

Change 638060 abandoned by DannyS712:
[mediawiki/core@master] EditPage: Prep for EditConflictManager refactor

Reason:
going a different route

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

Change 639654 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage::runPostMergeFilters should never start with a hook error

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

Change 639658 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Make EditConstraintRunner reusable

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

Change 639654 merged by jenkins-bot:
[mediawiki/core@master] EditPage::runPostMergeFilters never starts with a hook error

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

Change 639661 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Move EditPage::runPostMergeFilters to a constraint

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

Change 640196 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Rename MissingSummaryConstraint to NewSectionMissingSummaryConstraint

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

Change 640196 merged by jenkins-bot:
[mediawiki/core@master] Rename MissingSummaryConstraint to NewSectionMissingSummaryConstraint

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

Change 640492 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: Move $sectionHeadingToCheck handling to SpamRegexConstraint

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

Change 639661 merged by jenkins-bot:
[mediawiki/core@master] Move EditPage::runPostMergeFilters to a constraint

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

Change 640494 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add AutoSummaryMissingSummaryConstraint

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

Change 640492 merged by jenkins-bot:
[mediawiki/core@master] EditPage: Move $sectionHeadingToCheck handling to SpamRegexConstraint

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

Change 640494 merged by jenkins-bot:
[mediawiki/core@master] EditPage: Add two more constraints

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

Change 640747 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add AccidentalRecreationConstraint

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

Change 640747 merged by jenkins-bot:
[mediawiki/core@master] Add AccidentalRecreationConstraint

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

Change 642439 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: misc cleanup related to eventual backend

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

Change 644497 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Move more logic into edit constraints

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

Change 642439 merged by jenkins-bot:
[mediawiki/core@master] EditPage: misc cleanup related to eventual backend

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

Change 644499 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: use TextFormatter in ::newSectionSummary()

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

Change 644497 merged by jenkins-bot:
[mediawiki/core@master] Move more logic into edit constraints

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

Change 644502 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditManager: create initial service for editing backend

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

Change 644499 merged by jenkins-bot:
[mediawiki/core@master] EditPage: use TextFormatter in ::newSectionSummary()

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

Change 644503 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove EditPage::$unicodeCheck

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

Change 644504 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] UserRateLimitConstraint: move detection of content model change to constraint

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

Change 644503 abandoned by DannyS712:
[mediawiki/core@master] Remove EditPage::$unicodeCheck

Reason:
Breaks unrelated tests, not important

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

Change 644648 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: cleanup mergeChangesIntoContent

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

Change 644504 merged by jenkins-bot:
[mediawiki/core@master] UserRateLimitConstraint: move detection of content model change to constraint

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

Change 644648 merged by jenkins-bot:
[mediawiki/core@master] EditPage: cleanup mergeChangesIntoContent

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

Change 644825 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage: reduce coupling in internalAttemptSave

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

Change 647301 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] ImageRedirectConstraint: accept Content objects

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

Change 647303 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add EditPage::handleFailedConstraint

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

Change 647304 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] EditPage::internalAttemptSave - run onEditFilter hook earlier

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

Change 647301 merged by jenkins-bot:
[mediawiki/core@master] ImageRedirectConstraint: accept Content objects

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

Change 647303 merged by jenkins-bot:
[mediawiki/core@master] Add EditPage::handleFailedConstraint

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

Change 647304 merged by jenkins-bot:
[mediawiki/core@master] EditPage::internalAttemptSave - run onEditFilter hook earlier

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

Change 649916 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Factor out a backend from EditPage

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

Change 644825 abandoned by DannyS712:
[mediawiki/core@master] EditPage: reduce coupling in internalAttemptSave

Reason:
superseded by https://gerrit.wikimedia.org/r/c/mediawiki/core/ /649916

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

Change 644502 abandoned by DannyS712:
[mediawiki/core@master] EditManager: create initial service for editing backend

Reason:
superseded by https://gerrit.wikimedia.org/r/c/mediawiki/core/ /649916

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

Change 639658 abandoned by DannyS712:
[mediawiki/core@master] Make EditConstraintRunner reusable

Reason:
No longer needed, design at https://gerrit.wikimedia.org/r/c/mediawiki/core/ /649916 uses a new Runner for each set of checks

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

Change 651715 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Bring SpamRegexConstraint test coverage to 100%

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

Change 651716 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add tests for EditConstraintFactory

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

Change 651715 merged by jenkins-bot:
[mediawiki/core@master] Bring SpamRegexConstraint test coverage to 100%

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

Change 651716 merged by jenkins-bot:
[mediawiki/core@master] Add tests for EditConstraintFactory

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

Change 649916 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Factor out a backend from EditPage

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

This is still waiting for review. @Pchelolo @Clarakosi any chance you can take a look?

@Pchelolo and others, there was a question about naming on the patch. I currently have "EditManager" and "EditManagerFactory", but @Pchelolo pointed out that

Naming... Usually 'Manager' in MW is a singleton object ( or per-domain singleton ) providing some features. The key is that "Manager" is independent of the request context. What you are creating here is a Command pattern (like MovePage etc). EditPage name is taken, so perhaps EditCommand?

I suggested "ManagedEdit" or "EditPageEdit", since this seems to me to be bigger than the command pattern. What do others think about naming? Some options:

  • EditManager (not a singleton though)
  • EditCommand (bigger than the rest of the existing commands, in my view)
  • ManagedEdit
  • EditPageEdit
  • PageEdit

open to hearing other names though.

@Pchelolo and others, there was a question about naming on the patch. I currently have "EditManager" and "EditManagerFactory", but @Pchelolo pointed out that

Naming... Usually 'Manager' in MW is a singleton object ( or per-domain singleton ) providing some features. The key is that "Manager" is independent of the request context. What you are creating here is a Command pattern (like MovePage etc). EditPage name is taken, so perhaps EditCommand?

I suggested "ManagedEdit" or "EditPageEdit", since this seems to me to be bigger than the command pattern. What do others think about naming? Some options:

  • EditManager (not a singleton though)
  • EditCommand (bigger than the rest of the existing commands, in my view)
  • ManagedEdit
  • EditPageEdit
  • PageEdit

open to hearing other names though.

Discussed with @Pchelolo on IRC, we settled on MediaWiki\EditPage\PageEdit and MediaWiki\EditPage\PageEdit. Should be ready for re-review

Change 674116 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Update creation of edit constraints following switch to Authority

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

Change 674116 merged by jenkins-bot:
[mediawiki/core@master] Update creation of edit constraints following switch to Authority

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

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

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

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