| DannyS712 | |
| Feb 26 2020, 8:02 PM |
| F31903483: Screenshot from 2020-06-24 09-51-13.png | |
| Jun 24 2020, 7:53 AM |
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:
The last patch probably caused T251386: Special:RecentChanges broken: MutableRevisionRecord: Bad value for parameter $id: must be a integer
Change 592777 abandoned by DannyS712:
DeletedContribsPager::formatRevisionRow - remove use of Revision objects
Reason:
doing in I223dd6dfba27e560aff89be6705d91c40c550dd8
Change 593785 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove use of Revision objects in RevisionList classes
Change 589911 abandoned by DannyS712:
Move SpecialMergeHistory::formatRevisionRow to pager, use RevisionRecord
Reason:
superseded by 49707c59da2c4776b7752a5f2d5710f772c8d541
Change 583840 abandoned by DannyS712:
Make RevDel(Archive|Revision)Item initRevision private
Reason:
Superseded
Change 583888 abandoned by DannyS712:
Make DifferenceEngine::revisionDeleteLink private
Reason:
Superseded
Change 585836 merged by jenkins-bot:
[mediawiki/core@master] Fix parameter documentation for dump methods for handling revisions
@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.
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
Change 587313 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Remove use of Revision::(de)?compressRevisionText
Change 602199 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace core uses and hard deprecate Revision::(de)compressRevisionText
Change 602199 merged by jenkins-bot:
[mediawiki/core@master] Replace core uses and hard deprecate Revision::(de)compressRevisionText
Change 604302 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Jade@master] CleanJudgmentLinksTest: Remove creation of Revision objects
Change 601946 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/FlaggedRevs@master] Update hook calling to use new HookContainer
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
Change 604302 merged by jenkins-bot:
[mediawiki/extensions/Jade@master] CleanJudgmentLinksTest: Remove creation of Revision objects
Change 604306 merged by jenkins-bot:
[mediawiki/core@master] Remove unneeded creation of revision objects
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
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
Change 606811 merged by jenkins-bot:
[mediawiki/core@master] Release notes: Put the deprecations related to Revision work together
(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:
That's all good and fine. But adding a hard-deprecation as massive as this on top is not ok.
Tagging Platform Engineering 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
[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.
Let me again try and summarize the key points. Please tell me if I got anything wrong.
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
This was in CPT inbox for a product decision regarding when to hard deprecate the Revision class.
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
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.)
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:
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.
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
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.
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, 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
Change 608481 merged by jenkins-bot:
[mediawiki/core@master] Tests: Reduce use of Revision objects
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
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
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):
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
Change 609286 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Replace uses of Revision constants
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