r71944 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71943‎ | r71944 | r71945 >
Date:12:51, 30 August 2010
Author:daniel
Status:reverted (Comments)
Tags:
Comment:
as per r71789: don't guess mime type again, that's expensive. Re-use the value from FileProps.
Modified paths:
  • /trunk/phase3/includes/upload/UploadBase.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadBase.php
——@@ -344,9 +344,8 @@
345345 $this->mFileProps = File::getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
346346 $this->checkMacBinary();
347347
348 - # magically determine mime type
 348+ $mime = $this->mFileProps[ 'mime' ];
349349 $magic = MimeMagic::singleton();
350 - $mime = $magic->guessMimeType( $this->mTempPath, false );
351350
352351 # check mime type, if desired
353352 $status = $this->verifyMimeType( $magic, $mime );

Follow-up revisions

RevisionCommit summaryAuthorDate
r72019reverting r71944 as per TheDJ's comment. Will investigate a better way to avo...daniel09:38, 31 August 2010
r72023Follow-up to r71944: Interoducing MimeMagic::improveTypeFromExtension() for t...daniel13:47, 31 August 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r71789abort PagedTiffHandler::check if the file in question isn't a tiff filedaniel12:43, 27 August 2010

Comments

#Comment by Duesentrieb (talk | contribs)   12:52, 30 August 2010

avoids redundant file access

#Comment by TheDJ (talk | contribs)   14:41, 30 August 2010

This changes behavior at this point from $magic->guessMimeType( $this->mTempPath, false ); to $magic->guessMimeType( $this->mTempPath, true/ext ) (as used by getPropsFromPath;

This might influence the way verification works and I'm not sure if that is desired. The change is that with true/ext it is allowed to use a part of the filename as the way of mime detection, whereas with ext==false, the filename may not be used to base mime type upon. This is the key element that makes the guess step different from the verification step and might be critical in strict filetype mode.

This part of the code is a tad of a mess, but if we change it, we need to make sure we aren't breaking anything.

#Comment by Duesentrieb (talk | contribs)   14:57, 30 August 2010

right. from reading the code, it seems that the extension is relevant in two cases:

1) the mime type can't be detected otherwise. In this case, guessMimeType(..., false) returns "unknown/unknown", while guessMimeType(..., $ext ) will retunr a mime type guessed based on the file extension. 2) the file is a zip file. In that case, if $ext is given, the mime type is determined based on the extension. This is useful for ODT files, etc.

It seems that it's indeed a bad idea to use the mime type from the file props here. The question is: what do do instead?

I propose this: 1) always use guessMimeType(..., false), deprecate the $ext parameter 2) in getPropsFromPath, set $props['raw-mime'] first. If it's application/zip or unknown/unknown, call guessTypesForExtension() to find $props['mime'].

this avoids running the "heavy" detection twice.


#Comment by Duesentrieb (talk | contribs)   09:38, 31 August 2010

reverted in r72019, will investigate a better way to avoid redundant mime guesses

Status & tagging log