Page MenuHomePhabricator

Hard deprecate the Revision class
Closed, ResolvedPublic

Description

It uses $wgUser in a lot of places
Rather than individually hard deprecating not passing a user, given that the whole class is soft deprecated, it should be hard deprecated and slated for removal

Larger subtasks:

Tracking pastes:

  • P10770 - Revision class
  • P10916 - Core methods that can accept Revisions
  • P10887 - Core methods that can return Revisions

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+100 -40
mediawiki/coremaster+209 -90
mediawiki/extensions/Thanksmaster+2 -5
mediawiki/coremaster+13 -18
mediawiki/extensions/FlaggedRevsmaster+163 -8
mediawiki/coremaster+44 -0
mediawiki/coremaster+50 -46
mediawiki/coremaster+22 -91
mediawiki/extensions/Jademaster+39 -19
mediawiki/coremaster+18 -2
mediawiki/extensions/Flowmaster+19 -4
mediawiki/extensions/CodeReviewmaster+10 -5
mediawiki/coremaster+28 -33
mediawiki/coremaster+3 -1
mediawiki/coremaster+5 -2
mediawiki/coremaster+64 -56
mediawiki/coremaster+24 -20
mediawiki/coremaster+13 -9
mediawiki/coremaster+6 -10
mediawiki/coremaster+5 -5
mediawiki/coremaster+22 -10
mediawiki/coremaster+5 -0
mediawiki/extensions/Wikibasemaster+35 -41
mediawiki/coremaster+32 -19
mediawiki/extensions/MobileFrontendmaster+4 -4
mediawiki/extensions/MassMessagemaster+41 -21
mediawiki/coremaster+2 -1
mediawiki/extensions/Thanksmaster+6 -1
mediawiki/extensions/MobileFrontendmaster+8 -7
mediawiki/coremaster+1 -0
mediawiki/extensions/Wikibasemaster+29 -24
mediawiki/extensions/Wikibasemaster+17 -5
mediawiki/coremaster+4 -0
mediawiki/coremaster+37 -19
mediawiki/extensions/FlaggedRevsmaster+85 -43
mediawiki/extensions/FlaggedRevsmaster+10 -2
mediawiki/extensions/Gadgetsmaster+18 -6
mediawiki/coremaster+6 -4
mediawiki/extensions/ConfirmEditmaster+6 -5
mediawiki/extensions/CiteThisPagemaster+3 -1
mediawiki/coremaster+12 -5
mediawiki/coremaster+31 -10
mediawiki/coremaster+14 -7
mediawiki/coremaster+15 -4
mediawiki/extensions/CirrusSearchmaster+5 -5
mediawiki/coremaster+22 -1
mediawiki/extensions/PageTriagemaster+8 -2
mediawiki/extensions/FlaggedRevsmaster+28 -14
mediawiki/extensions/Jademaster+3 -1
mediawiki/extensions/TemplateSandboxmaster+12 -3
mediawiki/extensions/Patrollermaster+16 -4
mediawiki/coremaster+4 -1
mediawiki/extensions/VisualEditormaster+4 -1
mediawiki/extensions/TemplateStylesmaster+0 -0
mediawiki/coremaster+5 -0
mediawiki/coremaster+90 -35
mediawiki/extensions/CheckUsermaster+8 -2
mediawiki/extensions/FlaggedRevsmaster+13 -6
mediawiki/extensions/LiquidThreadsmaster+1 -1
mediawiki/coremaster+9 -4
mediawiki/coremaster+4 -0
mediawiki/coremaster+3 -0
mediawiki/extensions/FlaggedRevsmaster+3 -1
mediawiki/extensions/HAWelcomemaster+0 -5
mediawiki/coremaster+45 -18
mediawiki/coremaster+26 -14
mediawiki/coremaster+63 -29
mediawiki/coremaster+7 -2
mediawiki/extensions/FlaggedRevsmaster+6 -3
mediawiki/extensions/FlaggedRevsmaster+26 -12
mediawiki/coremaster+132 -50
mediawiki/coremaster+12 -2
mediawiki/coremaster+34 -5
mediawiki/coremaster+61 -18
mediawiki/extensions/SpamBlacklistmaster+23 -10
mediawiki/extensions/Flowmaster+9 -6
mediawiki/extensions/CollaborationKitmaster+30 -14
mediawiki/extensions/Flowmaster+7 -3
mediawiki/extensions/GlobalCssJsmaster+9 -8
mediawiki/coremaster+7 -4
mediawiki/coremaster+35 -40
mediawiki/extensions/WikimediaMaintenancemaster+12 -4
mediawiki/extensions/MapSourcesmaster+7 -2
mediawiki/extensions/SecurePollmaster+4 -2
mediawiki/coremaster+15 -9
mediawiki/extensions/Collectionmaster+10 -3
mediawiki/extensions/FlaggedRevsmaster+4 -2
mediawiki/coremaster+32 -18
mediawiki/extensions/CentralNoticewmf_deploy+15 -6
mediawiki/extensions/CentralNoticemaster+15 -6
mediawiki/extensions/CentralNoticemaster+36 -15
mediawiki/coremaster+30 -14
mediawiki/coremaster+16 -7
mediawiki/extensions/TemplateSandboxmaster+24 -7
mediawiki/skins/MinervaNeuemaster+8 -6
mediawiki/extensions/PageImagesmaster+37 -21
mediawiki/skins/MinervaNeuemaster+3 -1
mediawiki/extensions/MobileFrontendmaster+4 -1
mediawiki/extensions/Translatemaster+38 -14
mediawiki/extensions/VisualEditormaster+6 -3
mediawiki/coremaster+4 -0
mediawiki/coremaster+6 -0
mediawiki/coremaster+35 -8
Show related patches Customize query in gerrit

Related Objects

StatusSubtypeAssignedTask
OpenNone
OpenNone
OpenDannyS712
OpenDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
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
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712
ResolvedDannyS712

Event Timeline

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

Change 592774 merged by jenkins-bot:
[mediawiki/core@master] ChangesList::insertRollback - remove internal use of Revision objects

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

Change 592777 abandoned by DannyS712:
DeletedContribsPager::formatRevisionRow - remove use of Revision objects

Reason:
doing in I223dd6dfba27e560aff89be6705d91c40c550dd8

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

Change 593785 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove use of Revision objects in RevisionList classes

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

Change 589911 abandoned by DannyS712:
Move SpecialMergeHistory::formatRevisionRow to pager, use RevisionRecord

Reason:
superseded by 49707c59da2c4776b7752a5f2d5710f772c8d541

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

Change 583840 abandoned by DannyS712:
Make RevDel(Archive|Revision)Item initRevision private

Reason:
Superseded

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

Change 583888 abandoned by DannyS712:
Make DifferenceEngine::revisionDeleteLink private

Reason:
Superseded

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

Change 585836 merged by jenkins-bot:
[mediawiki/core@master] Fix parameter documentation for dump methods for handling revisions

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

@BPirkle and @CCicalese_WMF, @Pchelolo said that you would be willing to help with reviews. Should I add you to each of the pending commits, or how should I coordinate with you? P11014 lists some selected patches that are pending and are needed for milestones (such as hard deprecating specific methods) or are blocking other work.

@DannyS712, yes, @BPirkle and I will be doing code review. Please feel free to add us both to the pending commits, and we will coordinate between the two of us on review.

@DannyS712, yes, @BPirkle and I will be doing code review. Please feel free to add us both to the pending commits, and we will coordinate between the two of us on review.

Can you both please take a look at T250638#6127389 regarding how to properly apply autopatrol to edits made via PageUpdater? Its blocking some other work

Change 585006 merged by jenkins-bot:
[mediawiki/extensions/CodeReview@master] Remove use of Revision::compressRevisionText

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

Change 587313 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Remove use of Revision::(de)?compressRevisionText

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

Change 602199 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace core uses and hard deprecate Revision::(de)compressRevisionText

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

Change 602199 merged by jenkins-bot:
[mediawiki/core@master] Replace core uses and hard deprecate Revision::(de)compressRevisionText

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

Change 604302 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Jade@master] CleanJudgmentLinksTest: Remove creation of Revision objects

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

Change 601946 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/FlaggedRevs@master] Update hook calling to use new HookContainer

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

DannyS712 added a comment.EditedJun 10 2020, 8:29 AM

Update - Revision objects manually created in deployed code outside of core:

Wikibase's SqlStore::rebuild - will be removed in T249563: Hard deprecate Revision use in WikiPage::doEditUpdates
Jade's CleanJudgmentLinksTest::createRevision - https://gerrit.wikimedia.org/r/604302
FlaggedRevs PageStabilityForm::updateLogsAndHistory - Revision needs to be created for a hook; solution: only create if the hook is registered, which won't occur in production code - https://gerrit.wikimedia.org/r/601946
FlaggedRevs UpdateFRTracking::update_flaggedpages - will be removed in T250318: Hard deprecate WikiPage::updateIfNewerOn

All of Revision objects are created in core - see P11461 for analysis of core creation

Change 604306 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove unneeded creation of revision objects

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

Change 604302 merged by jenkins-bot:
[mediawiki/extensions/Jade@master] CleanJudgmentLinksTest: Remove creation of Revision objects

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

Change 604306 merged by jenkins-bot:
[mediawiki/core@master] Remove unneeded creation of revision objects

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

Change 606805 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Revision: Hard deprecation constructing with an array or an object

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

Change 606811 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Release notes: Put the deprecations related to Revision work together

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

Change 606811 merged by jenkins-bot:
[mediawiki/core@master] Release notes: Put the deprecations related to Revision work together

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

(Note: This is a response to a misplaced discussion in T249434#6244623.)

I'm tired of arguing for other peoples sanity, for my team's sanity, for my sanity.

Sure, we should hard-deprecate the class. But not in 1.35. Probably not even in 1.36. There are tens of thousands of usages of the Revision class out there. Give people time to adapt. Remove more methods from the class. Shrink it down to what I showed in T249434#6227107. And most importantly: First deprecate and slowly remove all code that still depends on the Revision class.

1.35 might be be the most destructive, most painful release ever anyway, even without hard-deprecating one of it's absolute central classes:

  • There are ~380 @deprecated since 1.35 tags in core right now, ~790 in total. In other words: Pretty much exactly half of all soft deprecations are new.
  • There are ~200 wfDeprecated( …, '1.35' ) calls, ~420 in total. Again, almost 50% are new.
  • 2 wfDeprecated() in the Revision class are new in 1.35.
  • It's hard to tell how many constructors (a.k.a. "hard-deprecated classes") exist in core right now. I could only find 3: SimpleSearchResultSetWidget, SimpleSearchResultWidget, SpecialChangeContentModel.
  • ~180 top-level @deprecated tags exist right now. ~60 of them since 1.35. To be fair most of them are not about classes, but constants, global functions, hook interfaces, and aliases.
  • I can find 29 classes in core's includes folder that are soft-deprecated right now, 5 of then since 1.35: Autopromote, SquidPurgeClient, SquidPurgeClientPool, IP, IDGenerator.

That's all good and fine. But adding a hard-deprecation as massive as this on top is not ok.

Tagging Core Platform Team for a product decision regarding whether the class should be hard deprecated as part of the 1.35 release, or delayed per @thiemowmde above

Change 606805 merged by jenkins-bot:
[mediawiki/core@master] Revision: Hard deprecation constructing with an array or an object

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

1.35 might be be the most destructive, most painful release ever anyway

[Snip]

Please don't abuse numbers in this way. It's pretty insulting. Obviously most of the current hard-deprecations are from 1.35, because the older ones have been removed. Similarly with soft-deprecation documentation tags.

1.34.0 came with 183 wfDeprecated in the includes folder. The current master contains 396 wfDeprecated in the includes folder, 218 of them since 1.35. In other words, it looks like 178 of the hard deprecations from 1.34 are still there. Only 5 got removed, but 218 added.

"Obviously".

Hey all, I will follow up with the WMDE teams to check on the impacts for their side of these changes and determine next steps. I'll update here once we've done that.

daniel added a subscriber: daniel.EditedWed, Jun 24, 3:23 PM

Let me again try and summarize the key points. Please tell me if I got anything wrong.

  • Hard-deprecating the class means making the constructor emit a deprecation warning.
  • Deprecation breaks nothing in production, but is does effectively block development, since it makes phpunit tests fail for code that uses deprecated functionality. That is intentional, to force callers to migrate away from the deprecated functionality.
  • We can hard-deprecate things only if we have no callers in deployed code (except in code that is itself hard-deprecated).
  • We may delay hard-deprecating things still in use by third parties, but only in exceptional cases.
  • We are currently constructing Revision objects in non-deprecated code in core in about 20 places, in order to pass them to hooks.
  • All hooks that take Revision objects seem to be (soft) deprecated
  • IIRC, adding hooks to DeprecatedHooks means hard-deprecating them, causing a warning when extensions use them. This should currently only be the case for hooks soft-deprecated in 1.34 or earlier.

So, it seems to me like we can't hard-deprecate the Revision class as long as it is used by hooks that have not yet been hard-deprecated.

Furthermore, when constructing Revision objects for hard-deprecated hooks, the code should be guarded by a check to see whether a handler for that hook is registered, so we only instantiate the deprecated class when actually needed. This could be done in a HookRunner, perhaps by automatically triggering old hooks when their new replacement is triggered. Also see my comment on this on the ticket about deprecating hooks that use Revision: T249434#6253126.

Effectively, this means all code relying on Revision should be at least soft-deprecated in 1.35, so it can be hard-deprecated in 1.36. In 1.36, we can then hard-deprecate the Revision class.

Did I miss anything?

Already in CPT External Code Reviews, so untagging CPT so that it moves out of the inbox.

Change 601946 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Update hook calling to use new HookContainer

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

Already in CPT External Code Reviews, so untagging CPT so that it moves out of the inbox.

This was in CPT inbox for a product decision regarding when to hard deprecate the Revision class.

Let me again try and summarize the key points. Please tell me if I got anything wrong.

  • Hard-deprecating the class means making the constructor emit a deprecation warning.
  • Deprecation breaks nothing in production, but is does effectively block development, since it makes phpunit tests fail for code that uses deprecated functionality. That is intentional, to force callers to migrate away from the deprecated functionality.
  • We can hard-deprecate things only if we have no callers in deployed code (except in code that is itself hard-deprecated).
  • We may delay hard-deprecating things still in use by third parties, but only in exceptional cases.
  • We are currently constructing Revision objects in non-deprecated code in core in about 20 places, in order to pass them to hooks.

You can see breakdown of all constructions of Revision objects at P11656

  • All hooks that take Revision objects seem to be (soft) deprecated
  • IIRC, adding hooks to DeprecatedHooks means hard-deprecating them, causing a warning when extensions use them. This should currently only be the case for hooks soft-deprecated in 1.34 or earlier.

Hooks can be hard and soft deprecated in the same release, and the hooks added to DeprecatedHooks that use Revision objects were mostly soft and hard deprecated in 1.35.

So, it seems to me like we can't hard-deprecate the Revision class as long as it is used by hooks that have not yet been hard-deprecated.

The remaining hooks that have not yet been hard deprecated and use Revision objects are

  • TitleMoveCompleting
  • TitleMoveComplete
  • PageContentInsertComplete
  • PageContentSaveComplete

These are all soft deprecated, and uses are being replaced (see T250566 and T250023)

Furthermore, when constructing Revision objects for hard-deprecated hooks, the code should be guarded by a check to see whether a handler for that hook is registered, so we only instantiate the deprecated class when actually needed. This could be done in a HookRunner, perhaps by automatically triggering old hooks when their new replacement is triggered. Also see my comment on this on the ticket about deprecating hooks that use Revision: T249434#6253126.

Effectively, this means all code relying on Revision should be at least soft-deprecated in 1.35, so it can be hard-deprecated in 1.36. In 1.36, we can then hard-deprecate the Revision class.

Did I miss anything?

All code relying on Revision objects should already be soft-deprecated, and almost all of it is already hard deprecated. My suggestion was that the class be hard-deprecated in 1.35, rather than waiting till 1.36. The Revision class was officially soft deprecated in 1.31 (@deprecated since 1.31, use RevisionRecord, RevisionStore, and BlobStore instead.)

Hooks can be hard and soft deprecated in the same release, and the hooks added to DeprecatedHooks that use Revision objects were mostly soft and hard deprecated in 1.35.

Right. The policy says: "Often code is soft deprecated first, and hard deprecated at a later date, but they MAY occur at the same time."

The policy is not very clear on this, but my would be that we can hard-deprecate if at least one of the following conditions is met:

  • the deprecated code isn't used by anything in a known repo (per codesearch)
  • the deprecated code isn't used by anything that is bundled with MediaWiki or deployed on Wikimedia sites, AND it has been soft-deprecated for at least one release, AND there is a good reason not to wait for the next release.

This gives third parties one release to act on soft deprecation, and it ensures we are not using deprecated code ourselves.

The remaining hooks that have not yet been hard deprecated and use Revision [...] are all soft deprecated, and uses are being replaced (see T250566 and T250023)

Are these all remaining known uses, or just the remaining uses in deployed or bundled code?

My suggestion was that the class be hard-deprecated in 1.35, rather than waiting till 1.36.
The Revision class was officially soft deprecated in 1.31 (@deprecated since 1.31, use RevisionRecord, RevisionStore, and BlobStore instead.)

Yes, it has been sitting around for a long time unfortunately. I'd be very happy to finally see it go. But we can only do that if there are no usages that are not hard deprecated in bundled or deployed code, including signatures of hooks that are not hard-deprecated.

Hooks can be hard and soft deprecated in the same release, and the hooks added to DeprecatedHooks that use Revision objects were mostly soft and hard deprecated in 1.35.

Right. The policy says: "Often code is soft deprecated first, and hard deprecated at a later date, but they MAY occur at the same time."

The policy is not very clear on this, but my would be that we can hard-deprecate if at least one of the following conditions is met:

  • the deprecated code isn't used by anything in a known repo (per codesearch)
  • the deprecated code isn't used by anything that is bundled with MediaWiki or deployed on Wikimedia sites, AND it has been soft-deprecated for at least one release, AND there is a good reason not to wait for the next release.

Not sure I follow - if it has been soft deprecated for at least one release, and it isn't used by anything that is bundled or deployed, the policy already allows hard deprecation in the second release

This gives third parties one release to act on soft deprecation, and it ensures we are not using deprecated code ourselves.

The remaining hooks that have not yet been hard deprecated and use Revision [...] are all soft deprecated, and uses are being replaced (see T250566 and T250023)

Are these all remaining known uses, or just the remaining uses in deployed or bundled code?

Just uses of the hooks in deployed/bundled code

My suggestion was that the class be hard-deprecated in 1.35, rather than waiting till 1.36.
The Revision class was officially soft deprecated in 1.31 (@deprecated since 1.31, use RevisionRecord, RevisionStore, and BlobStore instead.)

Yes, it has been sitting around for a long time unfortunately. I'd be very happy to finally see it go. But we can only do that if there are no usages that are not hard deprecated in bundled or deployed code, including signatures of hooks that are not hard-deprecated.

Indeed. So, should we push to hard deprecate it in hooks, etc. and the overall class by the release of 1.35 or not? If yes, it is doable

Not sure I follow - if it has been soft deprecated for at least one release, and it isn't used by anything that is bundled or deployed, the policy already allows hard deprecation in the second release

Yes, my point was that doing it in the *same* release should only be done if we are sure it's totally unused.
With the new policy, after 1.35 has been branched, we could skip deprecation entirely in such cases, and just remove the code.

Are these all remaining known uses, or just the remaining uses in deployed or bundled code?

Just uses of the hooks in deployed/bundled code

Right, but these would need to be replaced before we can go for hard deprecation.
And we still need to agree on a mechanism for only constructing Revisions if something is actually using the deprecated hook, to avoid warnings from the constructor.

As far as I can see, there is still discussion about this.
If I see this correctly, Thiemo wants to keep constructing Revision instances unconditionally until the hook is fully removed, blocking hard deprecation of the class.
I prefer encapsulating logic for triggering deprecated hooks in HookRunner.
IIRC, other have in the past expressed a preference for having conditional on isRegistered() directly in the application logic that triggers the hook.

This needs to be sorted out before we can hard-deprecate the class. I'm not sure we can do this before the 1.35 branch.

Yes, it has been sitting around for a long time unfortunately. I'd be very happy to finally see it go. But we can only do that if there are no usages that are not hard deprecated in bundled or deployed code, including signatures of hooks that are not hard-deprecated.

Indeed. So, should we push to hard deprecate it in hooks, etc. and the overall class by the release of 1.35 or not? If yes, it is doable

What's your proposal for the conditional construction of Revision objects that are needed by deprecated hooks?

All in all, it would be nice to be able to hard-deprecate Revision in 1.35, but it's not super urgent. But it's not important enough to me to rush things if that means we cause contention or debt. If we need more time to sort out the conditional construction issue, waiting for 1.36 would be ok. I'd hope we could get to that soon though. We shouldn't wait until we are getting ready to cut the 1.36 branch.

One advantage we'd have after 1.35 is cut is the new deprecation policy.

Not sure I follow - if it has been soft deprecated for at least one release, and it isn't used by anything that is bundled or deployed, the policy already allows hard deprecation in the second release

Yes, my point was that doing it in the *same* release should only be done if we are sure it's totally unused.
With the new policy, after 1.35 has been branched, we could skip deprecation entirely in such cases, and just remove the code.

That works for me too (just removing in 1.36) but shouldn't we give the deprecation warnings if we are planning on removing it?

Are these all remaining known uses, or just the remaining uses in deployed or bundled code?

Just uses of the hooks in deployed/bundled code

Right, but these would need to be replaced before we can go for hard deprecation.
And we still need to agree on a mechanism for only constructing Revisions if something is actually using the deprecated hook, to avoid warnings from the constructor.

As far as I can see, there is still discussion about this.
If I see this correctly, Thiemo wants to keep constructing Revision instances unconditionally until the hook is fully removed, blocking hard deprecation of the class.
I prefer encapsulating logic for triggering deprecated hooks in HookRunner.
IIRC, other have in the past expressed a preference for having conditional on isRegistered() directly in the application logic that triggers the hook.

This needs to be sorted out before we can hard-deprecate the class. I'm not sure we can do this before the 1.35 branch.

Yes, it has been sitting around for a long time unfortunately. I'd be very happy to finally see it go. But we can only do that if there are no usages that are not hard deprecated in bundled or deployed code, including signatures of hooks that are not hard-deprecated.

Indeed. So, should we push to hard deprecate it in hooks, etc. and the overall class by the release of 1.35 or not? If yes, it is doable

What's your proposal for the conditional construction of Revision objects that are needed by deprecated hooks?

I propose the conditional construction shown in https://gerrit.wikimedia.org/r/#/c/604547

Yes, my point was that doing it in the *same* release should only be done if we are sure it's totally unused.
With the new policy, after 1.35 has been branched, we could skip deprecation entirely in such cases, and just remove the code.

That works for me too (just removing in 1.36) but shouldn't we give the deprecation warnings if we are planning on removing it?

Yes, but we can only do this if the code is not used in production.
And we only *need* to do this if it's used by extensions not maintained by WMF.

If all usages are in code maintained by WMF, one option would be to wait until 1.35 is merged, fix the usages, and remove the old code without deprecation once it is unused.
I suppose that may be possible for some hooks. For Revision itself, that seems unlikely. The Revision class is used far and wide. Removing it without deprecation wouldn't be wise, even if we had removed all "known" usages in non-deprecated code.

I propose the conditional construction shown in https://gerrit.wikimedia.org/r/#/c/604547

Ah right, I remember. I will talk to @Thiemo about unblocking this.

Change 608469 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Thanks@master] BeforeSpecialMobileDiffDisplay hook: Only accept RevisionRecord

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Thanks/ /608469

Change 608481 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Tests: Reduce use of Revision objects

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608481

Change 608481 merged by jenkins-bot:
[mediawiki/core@master] Tests: Reduce use of Revision objects

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608481

Change 604547 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Don't create Revisions for deprecated hooks if hook is not registered

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /604547

Change 608469 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] BeforeSpecialMobileDiffDisplay hook: Only accept RevisionRecord

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Thanks/ /608469

Change 604547 merged by jenkins-bot:
[mediawiki/core@master] Don't create Revisions for deprecated hooks if hook is not registered

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /604547

Okay, @Pchelolo @daniel I think we're ready! I've tracked all of the places where Revision objects are created in deployed code at P11724, and currently it is only created in three types of places (excluding the static creation methods in the Revision class, and creation in tests):

  • To use as a parameter for a hook; the hook is hard deprecated, and the Revision object is only created if the hook is registered
  • To use in a method that is itself hard deprecated
  • In a callback used by DeprecatablePropertyArray to provide a Revision, where the DeprecatablePropertyArray is set to hard deprecate interacting with the Revision

Since all of the access points are hard deprecated, there should be no remaining construction of Revision objects in deployed code, and as a result no remaining uses of any of the classes methods.
Should we wait until next week's train to actually hard deprecate the class, or can we do it this week?

@DannyS712 gosh... scary, exciting, I think I even feel a little ticklish... So many emotions. Great great great job.

Should we wait until next week's train to actually hard deprecate the class, or can we do it this week?

I think we should merge it this week and closely monitor the deprecations log. The more time we have to detect and fix the long tail of things that might have escaped the better. Also, it would be really easy to revert and worst case scenario we will have some logspam

Change 608845 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Hard deprecate the rest of the Revision class

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608845

DannyS712 claimed this task.Thu, Jul 2, 9:16 AM
DannyS712 moved this task from Projects to Awaiting review and deployment on the User-DannyS712 board.

Change 609286 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace uses of Revision constants

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

I think we should merge it this week and closely monitor the deprecations log. The more time we have to detect and fix the long tail of things that might have escaped the better. Also, it would be really easy to revert and worst case scenario we will have some logspam

I'm more worried about problems in CI that disrupt other team's work. While working on the Wikidata team, I frequently had issues with being unable to merge changes to our repos because of an incompatible change in core. This can happen for any extension that is not checked in core CI.

So, basically - I agree, but I wouldn't want to merge this on a Friday. Better merge it on Monday, so it goes into the next train, and people are around to deal with CI breaking.

Change 608845 merged by jenkins-bot:
[mediawiki/core@master] Hard deprecate the rest of the Revision class

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