New subject: [Mediawiki-l] Bug 4421 - Image file extension should not be part of the name

15 Dec 2009

Hi folks,

Exec summary:  I'm looking for help on a possible change needed to
FileRepo::publish() API, since it currently makes some assumptions
that seem to be incompatible with fixing bug 4421 ("Image file
extension should not be part of the name"):
https://bugzilla.wikimedia.org/show_bug.cgi?id=4421

I'm also more generally looking for feedback on my approach to 4421.

Details:
A while back, I decided to try my hand at fixing bug 4421.  It's a
longstanding usability wart, and I figured it'd be a good way to
re-familiarize myself with the codebase.  It's mostly working, though
working through all of the edge cases is a bit of a game of
whack-a-mole.  Most of the complexity comes from needing to store the
files on the filesystem with appropriate file extensions, since these
get served directly from the filesystem from Apache.  Thus, there's a
lot of convoluted logic for tacking on the file extension in the
appropriate spots.  You can read the comments on 4421 to understand
why things are the way they are now.

An example of something that isn't working that I need advice on is
this:  with my modified version, it's possible to upload a jpeg to a
location without an extension, then upload a png to that same
location.  The problem comes in LocalFile::publish().  Here's the call
it makes from that function:
$status = $this->repo->publish( $srcPath, $dstRel, $archiveRel, $flags );

This causes two things to happen:
1.  copy $dstRel to $archiveRel
2.  copy $srcPath to $dstRel.

The problem here is with uploading a png over the top of a jpg.  For
example, if the title name is "File:Foo", then the filename for the
first version of the file will be "Foo.jpg", and the replacement will
be "Foo.png".  So, if we pass "Foo.jpg" to $dstRel, then step 1
works,
but step 2 fails.  If we pass "Foo.png", then the opposite problem
occurs.

The only solution it would seem is to rework FileRepo::publish() to
accommodate this case (adding another parameter).  That doesn't seem
*so* bad, though being an interface change.  Here's what I'm hoping
folks will weigh in on:
a.  If reworking FileRepo::publish() is the way to do it.
b.  If my general approach on 4421 seems sound, and to get a sense if
there's going to be objections to landing this in trunk soon
(presumably after 1.16).  I've just updated my latest version here:
https://bugzilla.wikimedia.org/attachment.cgi?id=6885&action=diff

Feel free to answer here or to ping me on IRC ("robla" on Freenode #mediawiki)

Thanks!
Rob