Page MenuHomePhabricator

Video undeletion hook is non-functional
Open, Needs TriagePublic

Description

VideoHooks::specialUndeleteSwitchArchive attempts to replace $archive but neglects to declare it as a reference parameter. So it has no effect and the feature will not work. Apparently broken since the initial commit of the file.

Event Timeline

Restricted Application added a project: Social-Tools. · View Herald TranscriptFeb 28 2020, 5:20 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ashley added a subscriber: ashley.Feb 28 2020, 10:28 PM

Hi Tim, thanks for taking the time to report this!

Video undeletion code is particularly nasty currently; in Video's extension.json we literally overwrite core Special:Undelete special page with our custom class, SpecialUndeleteWithVideoSupport, which is literally a fork of core SpecialUndelete.php with the bits that Video needs hacked in; this is nasty in every way, but sadly currently necessary because even with this bug fixed, there just aren't enough hooks in core SpecialUndelete.php to implement something like a new media type (which is what Video effectively does; it implements something *like* MW's File architecture, but not quite the same, since Video doesn't actually store any video files on the server) so Special:Undelete would still fail.

So currently there are 12 (!) hacks in SpecialUndeleteWithVideoSupport as opposed to the base SpecialUndelete class. Two of these are temporary tweaks for CI (not functionally relevant to the video undeletion feature). Fixing this bug would cause the hacks in SpecialUndeleteWithVideoSupport#showRevision, SpecialUndeleteWithVideoSupport#showHistory & SpecialUndeleteWithVideoSupport#undelete to become obsolete (yay!); still, that leaves us with seven chunks of very much necessary code that can't currently be implemented via hooks. And as people are prone to making mistakes, patching core code in the Video extension is arguably the slightly saner solution than asking people to patch core code themselves (as there's a very real risk that they'd lose the patches when upgrading, naturally).

How would you recommend trying to clean up this mess and make the SpecialUndeleteWithVideoSupport class completely obsolete?

Thanks in advance for any and all tips, suggestions & feedback!

Your question was a bit of a rabbit hole. I think I will file a separate bug about general refactoring of file deletion and undeletion.

The quick fix here is to factor out all ArchivedFile construction and newFromRow() in SpecialUndelete into one or two hookable private methods. Essentially introduce a private ArchivedFile factory. That shouldn't prevent the wider issues from being fixed. Then your idea of subclassing ArchivedFile will be less fragile and require fewer hooks.

Looking at the "core hacks" by line number:

  • 86: I don't understand why this is here.
  • 210: mostly just needs ArchivedFile factory?
  • 246: ArchivedFile factory
  • 397: this bug, to be superseded by ArchivedFile factory
  • 730: this bug, to be superseded by ArchivedFile factory
  • 768: Call the ArchivedFile::newFromRow() replacement on each row and use getRawUserText(). ArchivedVideo should provide getRawUserText().
  • 811, 828: seems obsolete already
  • 1047: Maybe it would be enough to use the ArchivedFile factory and to use accessors in it instead of direct access to $row
  • 1162: Needs a new hook?
  • 1202: Provide ArchivedVideo::getRawUser() and getRawUserText()
  • 1253: this bug, ArchivedFile factory

Also VideoPageArchive::listFiles() should join on actor, to avoid an additional DB query when mapping video_actor to a user name.

ashley moved this task from Backlog to Video on the Social-Tools board.Wed, Jul 22, 11:12 AM