The UndeleteShowRevision hook is entirely unused; it can just be hard deprecated. If a use case emerges, it can be added back as UndeleteShowRevisionRecord with a RevisionRecord instead

Even if this task by itself might not have patches attached to it, all the subtasks will have them, so for CPT this is a code review task.

Change 588197 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Deprecate the UndeleteShowRevision hook

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

Change 588198 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Video@master] Update hook call with deprecation of UndeleteShowRevision

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

Change 588197 merged by jenkins-bot:
[mediawiki/core@master] Deprecate the UndeleteShowRevision hook

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

Change 588198 merged by jenkins-bot:
[mediawiki/extensions/Video@master] Update hook call with deprecation of UndeleteShowRevision

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

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

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

In T249434#6213539, @gerritbot wrote:

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

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

In the patch linked, I proposed that Revision objects only be created for hooks if those hooks were registered.
Since the goal is to hard deprecate all of the hooks, and the Revision class itself, with 1.35, this ensures that deprecation warnings aren't triggered by creating Revision objects that then aren't used because the hooks themselves are unused (in deployed code)

@thiemowmde gave the patch a -2, suggesting that we should wait a version cycle between hard deprecating the hooks and hard deprecating the Revision class, saying I don't see why we need to push this so hard that it hurts..

I disagree, and believe that this doesn't "hurt" and that its possible to finish hard deprecating the Revision class with 1.35. I'd like to see what others think. @Pchelolo @daniel @BPirkle @cicalese

I think that most of the time we are dragging he deprecations so long that it hurts. Revision was soft-deprecated in 1.31, so it's been 2 years. I do not think waiting for 2 more years would be overly beneficial.

I don't think this is the correct ticket to discuss this. The patch https://gerrit.wikimedia.org/r/604547 doesn't "replace hooks that use Revision objects". It doesn't even make this easier. The patch is about deprecating the Revision class. That's T246284.

I will try to be as concise as possible:

• Personally, I agree the Revision class should be deprecated. But what needs discussion and very careful consideration (possibly even an RfC) is the migration path. Hard-deprecating it while there are probably tens of thousands of usages is – as far as I'm concerned – impossible.
  • new Revision() calls: https://codesearch.wmflabs.org/search/?q=%5Cbnew%5CW*Revision%5C%28, ~300 of them in core
  • use Revision statements: https://codesearch.wmflabs.org/search/?q=%5Cbuse%5CW*Revision%3B, ~400 of them in core
  • Revision $… type hints: https://codesearch.wmflabs.org/search/?q=%5CbRevision%20%5C%24, ~400 of them in core
• No problem with soft-deprecating the Revision class. It already is.
• What goals do we even have in mind?
  • Do we want to discourage people from using certain hooks? Ok. Let's deprecate these hooks then.
  • Do we want to discourage people from using Revision objects? Ok. Let's deprecate every way to interact with Revision objects then, i.e. let's deprecate all methods. At the moment, there are 53 public methods. Almost all of them are already marked as wfDeprecated(), with a few exceptions.
  • Do we want to discourage people from creating Revision objects? Maybe. But not for code that only exists because it has to, to please interfaces that still require Revision objects. Give people a chance to adapt and deprecate these interfaces first.
• The proposal https://gerrit.wikimedia.org/r/604547 means users will run into deprecation warnings for lines of code that are in core. What are these people supposed to do about this? Stop using the hook? Ok. Let's deprecate the hook then.
• A possible migration path is to continue reducing the Revision class to be nothing but a thin wrapper around a RevisionRecord. This allows people to slowly migrate to RevisionRecord, without the need to break thousands of interfaces the same time. When a good chunk of this migration is done, we can start removing the then pointless Revision wrapper from more and more code.

In T249434#6222765, @thiemowmde wrote:

I don't think this is the correct ticket to discuss this. The patch https://gerrit.wikimedia.org/r/604547 doesn't "replace hooks that use Revision objects". It doesn't even make this easier. The patch is about deprecating the Revision class. That's T246284.

It does indeed make it easier, because it means that we don't need to wait a release between deprecating the hooks and deprecating the Revision class. I guess I could have tagged the other task, but since it deals with the use of Revision objects in hooks I chose this one.

I will try to be as concise as possible:

• Personally, I agree the Revision class should be deprecated. But what needs discussion and very careful consideration (possibly even an RfC) is the migration path. Hard-deprecating it while there are probably tens of thousands of usages is – as far as I'm concerned – impossible.
  • new Revision() calls: https://codesearch.wmflabs.org/search/?q=%5Cbnew%5CW*Revision%5C%28, ~300 of them in core

https://codesearch.wmflabs.org/core/?q=%5Cbnew%5CW*Revision%5C%28 shows about a hundred in core - the search linked is for all repositories. See P11461 for an analysis of each time a Revision object is created in core - almost all methods that create one are already hard deprecated

• use Revision statements: https://codesearch.wmflabs.org/search/?q=%5Cbuse%5CW*Revision%3B, ~400 of them in core

https://codesearch.wmflabs.org/core/?q=%5Cbuse%5CW*Revision%3B shows about 19 in core - the search linked is for all repositories. Of those

• 3 are for use in tests
• 10 are for hooks that are deprecated
• the remainder are mostly for use in creating Revision objects in methods that are already hard deprecated

• Revision $… type hints: https://codesearch.wmflabs.org/search/?q=%5CbRevision%20%5C%24, ~400 of them in core

https://codesearch.wmflabs.org/core/?q=%5CbRevision%20%5C%24 shows about 72 in core - the search linked is for all repositories. Of those

• 41 are used in deprecated core
• 17 are used in tests
• 5 are false positives

• No problem with soft-deprecating the Revision class. It already is.
• What goals do we even have in mind?
  • Do we want to discourage people from using certain hooks? Ok. Let's deprecate these hooks then.

We are deprecating the hooks

• Do we want to discourage people from using Revision objects? Ok. Let's deprecate every way to interact with Revision objects then, i.e. let's deprecate all methods. At the moment, there are 53 public methods. Almost all of them are already marked as wfDeprecated(), with a few exceptions.

We are separately deprecating all Revision class methods, all methods returning revision objects, and support for passing Revision objects as parameters. See

• T249561: Deprecate methods accepting Revision objects as parameters
• T249393: Deprecate methods returning Revision objects
• T251067: Hard deprecate individual Revision class methods [tracking]

• Do we want to discourage people from creating Revision objects? Maybe. But not for code that only exists because it has to, to please interfaces that still require Revision objects. Give people a chance to adapt and deprecate these interfaces first.

The Revision class won't be hard deprecated until all of the code paths that need it are themselves hard deprecated - the interfaces with be hard deprecated first

• The proposal https://gerrit.wikimedia.org/r/604547 means users will run into deprecation warnings for lines of code that are in core. What are these people supposed to do about this? Stop using the hook? Ok. Let's deprecate the hook then.

The hooks are being hard deprecated

• A possible migration path is to continue reducing the Revision class to be nothing but a thin wrapper around a RevisionRecord. This allows people to slowly migrate to RevisionRecord, without the need to break thousands of interfaces the same time. When a good chunk of this migration is done, we can start removing the then pointless Revision wrapper from more and more code.

Not sure where you got the "thousands of interfaces", but its been soft deprecated since 1.32, allowing people to slowly migrate, and I've sent hundreds of patches to migrate the deployed extensions

It does indeed make it easier, because it means that we don't need to wait a release between deprecating the hooks and deprecating the Revision class.

I don't think we have the same idea of what "easy" means in this context. Your idea seems to be to make it easier for the devs that want to kill the Revision class as fast as possible. I'm talking about all the other devs that need to follow.

shows about […] in core

CodeSearch shows the number of affected files on top of the search results. This is what I'm quoting. These numbers might be wrong – I don't know.

The Revision class won't be hard deprecated until all of the code paths that need it are themselves hard deprecated

Great. https://gerrit.wikimedia.org/r/604547 is not needed then.

"thousands of interfaces" […]

I'm using "interface" in the broader sense: "a shared boundary across which two or more separate components of a computer system exchange information." (Wikipedia) There are hundreds of extensions – many of them not even monitored via CodeSearch – running in tens of thousands of MediaWiki installations. The number of lines of code that expect, create, or return Revision objects is certainly in the tens of thousands. I don't want us to act like we expect all these developers to essentially throw away and rewrite major parts of their code at once.

We can easily give these people a thin compatibility layer that could – in the very last stage – look like this:

class Revision {
	private $revisionRecord;

	public function __construct( RevisionRecord $revisionRecord ) {
		$this->revisionRecord = $revisionRecord;
	}

	public function getRevisionRecord() {
		return $this->revisionRecord;
	}
}

Don't tell me this is hard to maintain.

soft deprecated since 1.32

Good. Do you realize 1.32 is not even 1 and ½ years old?

In T249434#6227107, @thiemowmde wrote:

It does indeed make it easier, because it means that we don't need to wait a release between deprecating the hooks and deprecating the Revision class.

I don't think we have the same idea of what "easy" means in this context. Your idea seems to be to make it easier for the devs that want to kill the Revision class as fast as possible. I'm talking about all the other devs that need to follow.

That is exactly the "easier" I'm talking about - the devs that want to hard deprecate a class 3 versions after it was soft deprecated

shows about […] in core

CodeSearch shows the number of affected files on top of the search results. This is what I'm quoting. These numbers might be wrong – I don't know.

The Revision class won't be hard deprecated until all of the code paths that need it are themselves hard deprecated

Great. https://gerrit.wikimedia.org/r/604547 is not needed then.

It is needed, so that the Revision class can be hard deprecated once the code paths are hard deprecated

"thousands of interfaces" […]

I'm using "interface" in the broader sense: "a shared boundary across which two or more separate components of a computer system exchange information." (Wikipedia) There are hundreds of extensions – many of them not even monitored via CodeSearch – running in tens of thousands of MediaWiki installations. The number of lines of code that expect, create, or return Revision objects is certainly in the tens of thousands. I don't want us to act like we expect all these developers to essentially throw away and rewrite major parts of their code at once.

For deployed extensions, I personally have been rewriting major parts of the code to allow this. For extensions that aren't deployed, the code is only being hard deprecated, not removed

We can easily give these people a thin compatibility layer that could – in the very last stage – look like this:

class Revision {
	private $revisionRecord;

	public function __construct( RevisionRecord $revisionRecord ) {
		$this->revisionRecord = $revisionRecord;
	}

	public function getRevisionRecord() {
		return $this->revisionRecord;
	}
}

Don't tell me this is hard to maintain.

soft deprecated since 1.32

Good. Do you realize 1.32 is not even 1 and ½ years old?

Its also 3 versions ago - are you saying you would -2 a patch to hard deprecate the Revision class in 1.35?

Interesting how you skipped the code example.

I fundamentally disagree with the implied idea of "we need to kill this to make our lives easier". Our (the MediaWiki core developer's) job is not to make our lives easier. Our job is to make other's lives easier.

I'm confused - this ticket is about deprecating hooks, right? Skimmed the discussion, and it seems to be about deprecating construction of Revision objects.

Thiemo wrote:

Do we want to discourage people from using certain hooks? Ok. Let's deprecate these hooks then.

That's what this ticket is about... no disagreement there, right?

Do we want to discourage people from using Revision objects? Ok. Let's deprecate every way to interact with Revision objects then, i.e. let's deprecate all methods. At the moment, there are 53 public methods. Almost all of them are already marked as wfDeprecated(), with a few exceptions.

Yes, we should continue doing that. No disagreement, right?

Do we want to discourage people from creating Revision objects? Maybe. But not for code that only exists because it has to, to please interfaces that still require Revision objects. Give people a chance to adapt and deprecate these interfaces first.

I have to agree with that. Making construction trigger a deprecation warning should be the last thing we do.

The proposal https://gerrit.wikimedia.org/r/604547 means users will run into deprecation warnings for lines of code that are in core. What are these people supposed to do about this? Stop using the hook? Ok. Let's deprecate the hook then.

I see nothing in that code that would cause deprecation warnings. It just wraps calls to deprecated hooks in conditionals, to avoid construction of Revision instances when they are not needed. I think this is a good pattern. For one thing, it guards against accidental use of the Revision object later in the code. It also makes the fact that the hook being called is deprecated very obvious.

However, such guards only make sense for deprecated hooks. I did not check if all of the affected hooks are deprecated, and have "modern" replacements. That should be the case.

This could be done more nicely by creating a specialized HookRunner, a LegacyRevisionHookRunner or some such. It would behave much like the normal HookRunner, but it would take RevisionRecord objects, and internally only construct Revision objects when needed.

Or even nicer: in HookRunner, the new hook method would just call the old hook method automatically if needed.

A possible migration path is to continue reducing the Revision class to be nothing but a thin wrapper around a RevisionRecord. This allows people to slowly migrate to RevisionRecord, without the need to break thousands of interfaces the same time. When a good chunk of this migration is done, we can start removing the then pointless Revision wrapper from more and more code.

This was basically done in 1.32 already... the problem is that there is little incentive to actually migrate away from the deprecated class until things break. Hard deprecation of more and more things is needed to move this along.

In T249434#6227181, @thiemowmde wrote:

Interesting how you skipped the code example.

Yes, we could keep that wrapper, but even if we do, it should be hard deprecated. This isn't about a patch to remove the Revision class, but just only create Revision objects when they are needed, so that the class can (eventually) be hard deprecated

[…] only create Revision objects when they are needed, so that the class can (eventually) be hard deprecated

I know. But what is the point of that if the class is not hard-deprecated? The constructor is as cheap as it can be. The extra conditionals are possibly expensive, and add unnecessary additional sources of errors, mistakes, and confusing.

In T249434#6244213, @thiemowmde wrote:

[…] only create Revision objects when they are needed, so that the class can (eventually) be hard deprecated

I know. But what is the point of that if the class is not hard-deprecated? The constructor is as cheap as it can be. The extra conditionals are possibly expensive, and add unnecessary additional sources of errors, mistakes, and confusing.

I don't follow - are you saying we shouldn't hard deprecate the class? I've been working for months towards that hard deprecation, with the support of Platform Engineering, to try and get it into 1.35

Who "tries to get it in 1.35"? The Platform Engineering? Where is this decision documented?

Continued in T246284#6247391.

In T249434#6244213, @thiemowmde wrote:

[…] only create Revision objects when they are needed, so that the class can (eventually) be hard deprecated

I know. But what is the point of that if the class is not hard-deprecated? The constructor is as cheap as it can be. The extra conditionals are possibly expensive, and add unnecessary additional sources of errors, mistakes, and confusing.

This follows the idea that in core, only hard-deprecated code must call hard-deprecated code. Or to put it the other way around, we can only hard-deprecated Revision's constructor once all code that calls it is hard-deprecated. For deprecated extensions, this implies that we have to make sure we only call the deprecated constructor if the Revision is needed by a (deprecated) hook.

I would prefer this logic to live in HookRunner (or a wrapper of HookHandler), so it's hidden from application logic, and application logic no longer needs to know about Revision at all.

Please also see my comment on deprecating Revision: T246284#6253122.

In T249434#6247392, @thiemowmde wrote:

Who "tries to get it in 1.35"? The Platform Engineering? Where is this decision documented?

The intent is to remove deprecated code. One step is to hard-deprecated things that were once soft-deprecated. When deprecating classes, this also entails deprecating methods (including hooks) that make use of the deprecated class. For hooks, this is especially complicated, since we have to be able to still provide the deprecated class as a parameter, which would trigger a warning, unless we add a guard to makes sure the old class is only constructed when needed by an extension that uses a deprecated hook.

That's how we got here. There is no set goal to get anything specific into 1.35, but there is a goal to make things that were soft-deprecated in 1.34 and before be hard-deprecated now, as soon as reasonably possible.

I'll note that my response, as with this thread, is offtopic to this task (the corrector one is above, of course). However, the discussion started here...

In T249434#6244213, @thiemowmde wrote:

[…] only create Revision objects when they are needed, so that the class can (eventually) be hard deprecated

I know. But what is the point of that if the class is not hard-deprecated? The constructor is as cheap as it can be. The extra conditionals are possibly expensive, and add unnecessary additional sources of errors, mistakes, and confusing.

I think the answer to this question is that the interface policy requires (among other items):

1. MediaWiki core code that triggers hard deprecation warnings MUST NOT be reachable from non-deprecated core code using non-deprecated configuration settings
2. Extensions and skins bundled with the MediaWiki tarball MUST NOT trigger hard deprecation warnings and MUST be updated to use the new code.

The class cannot be hard-deprecated until its uses are removed or refactored to meet these conditions. See also Daniel's comment.

(Probably the policy should instead or also say that software deployed to Wikimedia servers MUST NOT have hard deprecation warnings, since there are extensions/skins deployed to Wikimedia that are not in the tarball.)

In T249434#6254147, @Izno wrote:

(Probably the policy should instead or also say that software deployed to Wikimedia servers MUST NOT have hard deprecation warnings, since there are extensions/skins deployed to Wikimedia that are not in the tarball.)

Good point. I added it to the draft for the upcoming amendment to the policy.