r90747 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r90746‎ | r90747 | r90748 >
Date:01:43, 25 June 2011
Author:nelson
Status:ok (Comments)
Tags:
Comment:
getPath() does the wrong thing under SwiftMedia, but getRel() works for everything.
Modified paths:
  • /trunk/phase3/includes/filerepo/LocalFile.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/LocalFile.php
@@ -889,7 +889,7 @@
890890
891891 # Fail now if the file isn't there
892892 if ( !$this->fileExists ) {
893 - wfDebug( __METHOD__ . ": File " . $this->getPath() . " went missing!\n" );
 893+ wfDebug( __METHOD__ . ": File " . $this->getRel() . " went missing!\n" );
894894 return false;
895895 }
896896

Comments

#Comment by Brion VIBBER (talk | contribs)   23:38, 28 June 2011

If I understand right, this is because SwiftFile::getPath() implicitly copies the file to the local filesystem?

I honestly find that pretty worrying; getPath() is not expected to have open-ended side effects like that, and this may indicate that the File & LocalFile APIs aren't actually very suitable for accessing remote files... should it be refactored to include explicit concepts of checking out a temporary local file and removing it when done? It looks like the file path will be implicitly deleted when the SwiftLocalFile object is destroyed, which should keep from leaking on batch operations, but that might still mean a lot of unexpected copies being made and deleted.

I also see a fully duplicated getHistory method which has had a couple bits swapped out. (It kinda looks like all it needs to do is set $this->oldFileFromRowFactory on the SwiftRepo -- which appears to already be done -- and then just inherit the entire LocalFile::getHistory method?)

#Comment by RussNelson (talk | contribs)   00:07, 29 June 2011

Yes, it implicitly gets you a read-only copy of the file. I've audited all the (checked-in) code which calls File::getPath() and I see no problem with adding this semantic to it. There are *many* places where one piece of code or another expect the file to have local filesystem semantics. Fortunately, they all gateway through File::getPath(), so I think we can "refactor" by changing the API. We just say that people shouldn't expect File::getPath() to be lightweight on the first call. The copy is cached, so subsequent calls are lightweight.

There's a LOT of duplicated code in SwiftFile which will be deleted on the checkin I'm about to do. Just reviewing the diff for sanity before I commit.

Status & tagging log