Page MenuHomePhabricator

Remove unused method Title::validateFileMoveOperation
Closed, ResolvedPublic

Event Timeline

daniel created this task.Jan 21 2019, 4:47 PM
Restricted Application added projects: Commons, Multimedia. · View Herald TranscriptJan 21 2019, 4:47 PM

This is a protected function that has existed since at least 1.32 (although it appeared to be unused in core there too, I didn't check extensions yet).

Can we simply remove it (given that it is unused and has been for some time), or must we go through the Deprecation Policy at https://www.mediawiki.org/wiki/Deprecation_policy by starting with soft deprecation? If we can remove in this case, should we email wikitech-l and document per the policy page?

Reedy added subscribers: Legoktm, Reedy.EditedFeb 28 2019, 8:21 PM

Looks to have been there since 1.16 :)

https://github.com/wikimedia/mediawiki/commit/3e4467422987e4214e563133c7f4d7168e1dab60

It is mentioned a couple of times in TitleTest, which suggest they're covering it. That's obviously a lie... They were added by @Legoktm in https://github.com/wikimedia/mediawiki/commit/b3dd0fb560aa1a3a0d3c1cd75eacb055e56222b8

The obvious one would've been some call that has been removed/replaced at some point (if it was ever called)... I wonder if it was orphaned in the 1.16 refactoring? :)

I think the question here is therefore whether anyone is likely to subclass Title in any random extension (because that's basically the only way it'd be used)... Considering we don't have any examples of anyone doing that in any of our hosted extensions, I imagine it's unlikely

If it was public, it forms part of an "interface"... We would follow protocol. For a protected unused method? I'd get rid. You can add it to RELEASE-NOTES, but I wouldn't bother with wikitech-l etc

If it happens to be used by some weird dynamically generated call.. Well, the tests should break. If they don't, and it is used weirdly like that... Well, shame on them for not leaving any grep-ability behind

BPirkle reassigned this task from BPirkle to Hknust.Feb 28 2019, 8:23 PM
BPirkle added a subscriber: BPirkle.
Tgr added a comment.Feb 28 2019, 8:48 PM

More specifically, the stable part of the PHP API is comprised of all code that is explicitly marked public, and has been included in at least one stable release. In addition, some classes expect to be subclassed in extensions; in those cases protected functions also are included in the API. These classes should have a note in their documentation comment that they expect subclassing. If no note is present, it SHOULD be assumed that the class is not expected to be subclassed.

...so the deprecation policy clearly doesn't apply here.

I removed Title::validateFileMoveOperation and also removed the test comment saying the method was covered in the test but not the test itself since it is calling the replacement method.

Reedy added a comment.Feb 28 2019, 9:48 PM

I removed Title::validateFileMoveOperation and also removed the test comment saying the method was covered in the test but not the test itself since it is calling the replacement method.

There was also another comment in one of the data providers

commit it and push it to gerrit. See if the tests still all pass :)

Looks like it. :) I removed the comment but left the actual test case since that should be covered by the replacement method
https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/493508/

Change 493508 had a related patch set uploaded (by Legoktm; owner: Holger Knust):
[mediawiki/core@master] Removed unused method Title::validateFileMoveOperation

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

Change 493508 merged by jenkins-bot:
[mediawiki/core@master] Remove unused method Title::validateFileMoveOperation()

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

daniel closed this task as Resolved.Mar 4 2019, 6:45 PM