Page MenuHomePhabricator

Replace uses and hard deprecate WikiPage::doEditContent and PageUpdater::getStatus
Closed, DeclinedPublic

Description

Soft deprecated in favor of PageUpdater::saveRevision since 1.32
Returned status can include Revision objects
If a user isn't passed, falls back to $wgUser

Widely used: https://codesearch.wmflabs.org/deployed/?q=-%3EdoEditContent%5C(&i=nope&files=&repos=

The inclusion of Revision objects comes from the underlying call to PageUpdater::getStatus, which includes Revision objects. That method is tagged as @note This is here for compatibility with WikiPage::doEditContent. It may be deprecated soon.

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+55 -20
mediawiki/extensions/FileImportermaster+25 -17
mediawiki/coremaster+4 -4
mediawiki/extensions/CentralNoticemaster+17 -17
mediawiki/extensions/NewUserMessagemaster+10 -7
mediawiki/extensions/SecurePollmaster+55 -26
mediawiki/extensions/GWToolsetmaster+11 -6
mediawiki/extensions/CheckUsermaster+9 -2
mediawiki/extensions/SpamBlacklistmaster+4 -4
mediawiki/extensions/PageTriagemaster+17 -6
mediawiki/coremaster+210 -2
mediawiki/extensions/Thanksmaster+10 -26
mediawiki/coremaster+15 -7
mediawiki/extensions/Jademaster+0 -0
mediawiki/extensions/Jademaster+132 -92
mediawiki/extensions/WikimediaMaintenancemaster+23 -10
mediawiki/extensions/TemplateStylesmaster+13 -4
mediawiki/extensions/UploadWizardmaster+13 -7
mediawiki/extensions/Babelmaster+11 -8
mediawiki/extensions/Translatemaster+217 -105
Show related patches Customize query in gerrit

Related Objects

StatusSubtypeAssignedTask
OpenNone
OpenNone
OpenDannyS712
OpenDannyS712
OpenDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
DeclinedPchelolo
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712

Event Timeline

DannyS712 renamed this task from Hard deprecate WikiPage::doEditContent to Replace uses and hard deprecate WikiPage::doEditContent.Apr 19 2020, 11:38 PM
DannyS712 triaged this task as Medium priority.
DannyS712 created this task.
DannyS712 added a project: patch-welcome.
DannyS712 renamed this task from Replace uses and hard deprecate WikiPage::doEditContent to Replace uses and hard deprecate WikiPage::doEditContent and PageUpdater::getStatus.Apr 19 2020, 11:46 PM
DannyS712 updated the task description. (Show Details)

Change 590749 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/WikimediaMaintenance@master] Remove use of WikiPage::doEditContent

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

Change 590757 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/UploadWizard@master] Remove use of WikiPage::doEditContent

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

Change 590762 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/TemplateStyles@master] Remove use of WikiPage::doEditContent

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

Change 590762 merged by jenkins-bot:
[mediawiki/extensions/TemplateStyles@master] Remove use of WikiPage::doEditContent

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

Change 590757 merged by jenkins-bot:
[mediawiki/extensions/UploadWizard@master] Remove use of WikiPage::doEditContent

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

Change 592392 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Thanks@master] ApiCoreThankIntegrationTest: Remove use of WikiPage::doEditContent

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

Change 592762 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/SpamBlacklist@master] Remove use of WikiPage::doEditContent

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

Change 590749 merged by jenkins-bot:
[mediawiki/extensions/WikimediaMaintenance@master] Remove use of WikiPage::doEditContent

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

Change 592807 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Translate@master] Remove uses of WikiPage::doEditContent

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

Change 593090 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Make MessageCacheTest::makePage private, return RevisionRecord

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

Change 592807 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Remove uses of WikiPage::doEditContent

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

Change 594258 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Babel@master] Remove use of WikiPage::doEditContent

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

Change 594260 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/CheckUser@master] Remove use of WikiPage::doEditContent

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

Change 594262 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/FileImporter@master] Remove use of WikiPage::doEditContent

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

Change 594263 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/NewUserMessage@master] Remove use of WikiPage::doEditContent

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

Change 594312 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/SecurePoll@master] Remove use of WikiPage::doEditContent

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

Change 594314 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/PageTriage@master] Remove use of WikiPage::doEditContent

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

Change 594315 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/GWToolset@master] Remove use of WikiPage::doEditContent

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

Change 594342 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/CentralNotice@master] Remove use of WikiPage::doEditContent and the Revision object returned

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

Change 594343 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Jade@master] TestStorageHelper: Replace createEntity with createNewEntity

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

Change 594258 merged by jenkins-bot:
[mediawiki/extensions/Babel@master] Remove use of WikiPage::doEditContent

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

When changing WikiPage::doEditContent to use page updaters, I missed the fact that the former automatically added autopatrol status, while the latter doesn't.

WikiPage.php
	 * @deprecated since 1.32, use PageUpdater::saveRevision instead. Note that the new method
	 * expects callers to take care of checking EDIT_MINOR against the minoredit right, and to
	 * apply the autopatrol right as appropriate.
	 *

This does not make sense to me. Callers like Babel, UploadWizard, Translate etc must never concern themselves with these internal details of core.

If this is the only non-deprecated method to save edits in this way, then it should be un-deprecated. Let's revert these for now (tracked via T252179), and think about this some more. Copying this logic everywhere is imho not an acceptable solution, not even temporarily.

When changing WikiPage::doEditContent to use page updaters, I missed the fact that the former automatically added autopatrol status, while the latter doesn't.

WikiPage.php
	 * @deprecated since 1.32, use PageUpdater::saveRevision instead. Note that the new method
	 * expects callers to take care of checking EDIT_MINOR against the minoredit right, and to
	 * apply the autopatrol right as appropriate.
	 *

This does not make sense to me. Callers like Babel, UploadWizard, Translate etc must never concern themselves with these internal details of core.

If this is the only non-deprecated method to save edits in this way, then it should be un-deprecated. Let's revert these for now (tracked via T252179), and think about this some more. Copying this logic everywhere is imho not an acceptable solution, not even temporarily.

As far as I can tell, there is only 1 place that a PageUpdater is constructed, and it has no subclasses. Perhaps inject a PermissionManager as well?

Minor edit is determined by passing EDIT_MINOR as one of the flags to saveRevision(), but autopatrol is only done via setRcPatrolStatus. I suggest:

  • Inject a permission manager
  • Automatically handle setting autopatrol
  • Deprecate and then remove setRcPatrolStatus

This does not affect handling minor edit rights (yet)

Either at the same time or later:

  • If the flags contain EDIT_MINOR, check permissions; if the user lacks permission
    • Ignore the minor flag?
    • Error?

Change 595609 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] PageUpdater: Inject a permission manager, automatically apply autopatrol

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

Change 594343 merged by jenkins-bot:
[mediawiki/extensions/Jade@master] TestStorageHelper: Replace createEntity with createNewEntity

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

Change 595667 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Jade@master] TestStorageHelper: Replace saveJudgment with updateJudgement

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

Change 595609 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] PageUpdater: Inject a permission manager, automatically apply autopatrol

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

Has it been decided then to make PageUpdater responsible for everything? Is that something we want to support long-term?

I believe @daniel omitted it with the intent to keep PageUpdater more separate from the higher-level logic. The problem we surfaced here is that there needs to be a unified and central way to decide how edit meta-data is derived from the user and other context stuff.

We already have numerous of such abstractions in WikiPage and EditPage. Has it been considered to migrate toward one of those? And/or to undeprecate or rename doEditPage to be that better entrypoint? Or do we really want to move it all to PageUpdater?

I don't have a preference here, I just want to make sure we don't blindly follow my (unintended) suggestion for Danny to move it there.

Change 595667 abandoned by DannyS712:
TestStorageHelper: Replace saveJudgment with updateJudgement

Reason:
Messed up, will redo when I have a chance

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

DannyS712 added a comment.EditedMay 11 2020, 11:01 PM

Change 595609 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] PageUpdater: Inject a permission manager, automatically apply autopatrol

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

Has it been decided then to make PageUpdater responsible for everything? Is that something we want to support long-term?

I believe @daniel omitted it with the intent to keep PageUpdater more separate from the higher-level logic. The problem we surfaced here is that there needs to be a unified and central way to decide how edit meta-data is derived from the user and other context stuff.

We already have numerous of such abstractions in WikiPage and EditPage. Has it been considered to migrate toward one of those? And/or to undeprecate or rename doEditPage to be that better entrypoint? Or do we really want to move it all to PageUpdater?

I don't have a preference here, I just want to make sure we don't blindly follow my (unintended) suggestion for Danny to move it there.

doEditContent returns information that includes a Revision object, and for that to be deprecated doEditContent likewise needs to be deprecated.

Also, the current interface for saving an edit via EditPage is horrible (hence T157658: Factor out a backend from EditPage) so trying to use that would be more problematic

Change 593090 merged by jenkins-bot:
[mediawiki/core@master] Make MessageCacheTest::makePage private, return RevisionRecord

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

doEditContent returns information that includes a Revision object, and for that to be deprecated doEditContent likewise needs to be deprecated.

We could make the value in the Status object be an ArrayAccess instance that triggers a warning when something tries to access the "revision" key, rather than the "revision-record" key.

doEditContent returns information that includes a Revision object, and for that to be deprecated doEditContent likewise needs to be deprecated.

We could make the value in the Status object be an ArrayAccess instance that triggers a warning when something tries to access the "revision" key, rather than the "revision-record" key.

No idea how to do that

Pchelolo claimed this task.Jun 3 2020, 3:23 AM

Change 602115 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] WIP: Introduce DeprecatablePropertyArray and use it for PageUpdater

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

Change 592392 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Remove use of WikiPage::doEditContent

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

Change 602115 merged by jenkins-bot:
[mediawiki/core@master] Introduce DeprecatablePropertyArray and use it for PageUpdater

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

@Pchelolo should this be declined in favor of removing uses of the returned Revision object?

Yeah. I believe removing the 'revision' key in the status value overall would be easier then dropping the doEditContent completely.

DannyS712 closed this task as Declined.Tue, Jun 9, 9:49 PM

Change 594314 abandoned by DannyS712:
Replace use of WikiPage::doEditContent with PageUpdater

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

Change 592762 abandoned by DannyS712:
Replace use of WikiPage::doEditContent with PageUpdater

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

Change 594260 abandoned by DannyS712:
Replace use of WikiPage::doEditContent with PageUpdater

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

Change 594315 abandoned by DannyS712:
Replace use of WikiPage::doEditContent with PageUpdater

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

Change 594312 abandoned by DannyS712:
Replace use of WikiPage::doEditContent with PageUpdater

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

Change 594263 abandoned by DannyS712:
Replace use of WikiPage::doEditContent with PageUpdater

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

Change 594342 abandoned by DannyS712:
Remove use of WikiPage::doEditContent and the Revision object returned

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

Method is currently soft deprecated - should it be un-deprecated then?

Change 605692 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Un-deprecate WikiPage::doEditContent

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

Change 594262 abandoned by DannyS712:
Replace deprecated WikiPage::doEditContent() with a PageUpdater

Reason:
no longer needed

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

Change 595609 abandoned by DannyS712:
PageUpdater: Inject a permission manager, automatically apply autopatrol

Reason:
no longer needed

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