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

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/coremaster+13 -5
mediawiki/coremaster+185 -56
mediawiki/coremaster+3 K -657
mediawiki/coremaster+1 K -707
mediawiki/coremaster+2 -2
mediawiki/coremaster+47 -68
mediawiki/coremaster+135 -149
mediawiki/coremaster+495 -183
mediawiki/coremaster+319 -233
mediawiki/coremaster+221 -183
mediawiki/coremaster+6 -6
mediawiki/coremaster+30 -16
mediawiki/coremaster+1 -1
mediawiki/coremaster+41 -19
mediawiki/coremaster+38 -12
mediawiki/coremaster+14 -14
mediawiki/coremaster+43 -19
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 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!

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

[mediawiki/core@master] EditPage::internalAttemptSave() - move more logic to constraints

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

Okay, so coming back to this after a *while*, I'm going to try to make some more progress

  • some more logic can be moved to constraints (first patch for that already sent ^)
  • internalAttemptSave() isn't called anywhere in core or deployed extensions outside of EditPage.php and tests, so we can make it private (with a changed name) and replace the public version with a hard deprecated wrapper for the private one - after the hard deprecation, we can remove the public-ness of the method, and then refactoring the interface will be a lot simpler
  • 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? I'm happy to provide code review

Change #1053782 merged by jenkins-bot:

[mediawiki/core@master] EditPage::internalAttemptSave() - move more logic to constraints

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

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

[mediawiki/core@master] Rename AutoSummaryMissingSummaryConstraint

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

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

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

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

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

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