Chad wrote:
On Fri, Sep 5, 2008 at 6:11 AM, Tim Starling tstarling@wikimedia.org wrote:
[snip]
The technical details were as follows: I fixed a bug in File.php, and without checking what other changes were made to it, deployed the most recent version of the file on the Wikimedia servers, without also updating the rest of MediaWiki. Because FileRepo::$thumbDir was unset, LocalFile::migrateThumbFile() had the effect of deleting the source image for any thumbnail request which reached the backend. I reverted the change after about 20 minutes, following a report on IRC.
My sincere apologies.
And mine as well. I introduced the $thumbDir code in r40385. I thought I had set a sane default of 'thumb/' in the constructor (which would work per current behavior of using hardcoded 'thumb/'. Is there a code-path in which $thumbDir isn't being set? If so, that needs fixing asap. Would a revert be in order, or is everything ok as-is?
If you had followed my example and used an accessor function, instead of having the File class access member variables of the repo directly, then there would have been no problem. Adding an accessor is good style in any case, and you should make that change. But it wasn't your fault.
I patched two files in quick succession: GlobalFunctions.php and then File.php. With GlobalFunctions.php, I checked the diff carefully for any dependencies before I updated it on Wikimedia. There were no changes other than my own. With File.php, I assumed it would be OK and didn't check. I didn't think about it at the time, I was working quickly. Call it cognitive bias, loss of concentration, laziness, whatever. Not your fault.
There was a second programming error here, and that was the fact that I put an unlink() call in the code in the first place. It didn't seem dangerous at the time, but obviously migrateThumbFile() is a recipe for disaster if there's a potential for adverse input coming from getThumbPath().
However, the thumb directory is inherently temporary, and lots of things delete from it. I think I'd be most comfortable not having the thumbDir feature at all. Is there some reason for it?
-- Tim Starling