Page MenuHomePhabricator

Clarify confusion between FileRepo and LocalRepo in various Upload classes
Closed, ResolvedPublic

Description

A few classes under Upload/ seem to use FileRepo and LocalRepo somewhat interchangeably, which is wrong and makes phan v6 complain a lot while trying to upgrade (T406326). In particular, UploadFromChunks::__construct and UploadStash::__construct take FileRepo, but assign the parameter to properties documented as LocalRepo. It's not clear which of the two should be used:

  • UploadStash calls $this->repo->getPrimaryDB() multiple times, and that method only exists in LocalRepo.
  • But FileRepo has a getUploadStash method, and it passes $this as argument, so there's no guarantee that we're passing a LocalRepo

We don't have any errors in production, and this code has probably been like this for a long time, so I suppose there's some kind of implicit assumption that makes everything work correctly despite the type mismatch. However, as someone unfamiliar with this area of the code, this is really confusing and it would be nice to sort it out ahead of the phan v6 upgrade. To me it seems like we should change all those parameters to LocalRepo, and move getUploadStash to LocalRepo, plus any other changes needed to support that, but I'm not in a position to make this decision.

Event Timeline

Umherirrender subscribed.

ForeignDBRepo also declares getPrimaryDB, but upload works only on the local wiki

FileRepo::getUploadStash was added in 9fff147a379cad7afef268a68f465170ece95d67 / r80993 but only used with the local repo.

I do not see a problem when making everything local. Maybe FileRepo was used to use a narrow class as FileRepo::isLocal exists as well to enure hat a local repo is given, not only the LocalRepo class.

Change #1206442 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] Upload: Type against LocalRepo instead of FileRepo for UploadStash

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

Change #1206442 merged by jenkins-bot:

[mediawiki/core@master] Upload: Type against LocalRepo instead of FileRepo for UploadStash

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

matmarex subscribed.