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

SubjectRepoBranchLines +/-
mediawiki/coremaster+3 K -657
mediawiki/coremaster+13 -5
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 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

What's the status of this task? I would very much like to see EditPage go away, and I may be able to help with CR. The main patch is a few months old and out-of-date, at the point that it may be easier to start over. I also wonder if it would make more sense to split the changes into multiple patches, e.g. move everything as static methods in another file, then make everything non-static, inject dependencies, clean up etc., so that it's easier to review and it's less likely for things to break (which remains quite likely, given that we're talking about EditPage).

What's the status of this task? I would very much like to see EditPage go away, and I may be able to help with CR. The main patch is a few months old and out-of-date, at the point that it may be easier to start over. I also wonder if it would make more sense to split the changes into multiple patches, e.g. move everything as static methods in another file, then make everything non-static, inject dependencies, clean up etc., so that it's easier to review and it's less likely for things to break (which remains quite likely, given that we're talking about EditPage).

Makes sense. I should have some time to revisit this soon, and will definitely split it up as suggested. This won't be able to finish completely until T251588 drops the hook that passes an entire EditPage before each edit, but we can definitely make progress

@DannyS712: Removing task assignee as this open task has been assigned for more than two years - See the email sent to task assignee on Feburary 22nd, 2023.
Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome! :)
If this task has been resolved in the meantime, or should not be worked on by anybody ("declined"), please update its task status via "Add Action… 🡒 Change Status".
Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator. Thanks!