r70037 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70036‎ | r70037 | r70038 >
Date:20:38, 27 July 2010
Author:btongminh
Status:resolved (Comments)
Tags:
Comment:
(bug 23380) Uploaded files that are larger than allowed by PHP now show a useful error message.

Introduced a WebRequestUpload class which is a wrapper around $_FILES and contains all getUpload* and getFile* methods. This has as advantage that the upload can be passed along without $wgRequest. Also because I like objects.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/WebRequest.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadBase.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadFromFile.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadBase.php
——@@ -236,7 +236,10 @@
237237 */
238238 global $wgMaxUploadSize;
239239 if( $this->mFileSize > $wgMaxUploadSize ) {
240 - return array( 'status' => self::FILE_TOO_LARGE );
 240+ return array(
 241+ 'status' => self::FILE_TOO_LARGE,
 242+ 'max' => $wgMaxUploadSize,
 243+ );
241244 }
242245
243246 /**
Index: trunk/phase3/includes/upload/UploadFromFile.php
——@@ -8,16 +8,18 @@
99 * Implements regular file uploads
1010 */
1111 class UploadFromFile extends UploadBase {
 12+ protected $mWebUpload = null;
1213
13 -
1414 function initializeFromRequest( &$request ) {
 15+ $this->mWebUpload = $request->getUpload( 'wpUploadFile' );
 16+
1517 $desiredDestName = $request->getText( 'wpDestFile' );
1618 if( !$desiredDestName )
17 - $desiredDestName = $request->getFileName( 'wpUploadFile' );
 19+ $desiredDestName = $this->mWebUpload->getName();
1820 return $this->initializePathInfo(
1921 $desiredDestName,
20 - $request->getFileTempName( 'wpUploadFile' ),
21 - $request->getFileSize( 'wpUploadFile' )
 22+ $this->mWebUpload->getTempName(),
 23+ $this->mWebUpload->getSize()
2224 );
2325 }
2426 /**
——@@ -27,6 +29,35 @@
2830 return $this->initializePathInfo( $name, $tempPath, $fileSize );
2931 }
3032 static function isValidRequest( $request ) {
31 - return (bool)$request->getFileTempName( 'wpUploadFile' );
 33+ # Allow all requests, even if no file is present, so that an error
 34+ # because a post_max_size or upload_max_filesize overflow
 35+ return true;
3236 }
 37+
 38+ public function verifyUpload() {
 39+ # Check for a post_max_size or upload_max_size overflow, so that a
 40+ # proper error can be shown to the user
 41+ if ( is_null( $this->mTempPath ) || $this->isEmptyFile() ) {
 42+ # Using the Content-length header is not an absolutely fail-safe
 43+ # method, but the only available option. Otherwise broken uploads
 44+ # will be handled by the parent method and return empty-file
 45+ $contentLength = intval( $_SERVER['CONTENT_LENGTH'] );
 46+ $maxPostSize = wfShorthandToInteger( ini_get( 'post_max_size' ) );
 47+ if ( $this->mWebUpload->getError() == UPLOAD_ERR_INI_SIZE
 48+ || $contentLength > $maxPostSize ) {
 49+
 50+ global $wgMaxUploadSize;
 51+ return array(
 52+ 'status' => self::FILE_TOO_LARGE,
 53+ 'max' => min(
 54+ $wgMaxUploadSize,
 55+ wfShorthandToInteger( ini_get( 'upload_max_filesize' ) ),
 56+ $maxPostSize
 57+ ),
 58+ );
 59+ }
 60+ }
 61+
 62+ return parent::verifyUpload();
 63+ }
3364 }
Index: trunk/phase3/includes/WebRequest.php
——@@ -590,23 +590,20 @@
591591 * @return string or NULL if no such file.
592592 */
593593 public function getFileTempname( $key ) {
594 - if( !isset( $_FILES[$key] ) ) {
595 - return null;
596 - }
597 - return $_FILES[$key]['tmp_name'];
 594+ $file = new WebRequestUpload( $key );
 595+ return $file->getTempName();
598596 }
599597
600598 /**
601599 * Return the size of the upload, or 0.
602600 *
 601+ * @deprecated
603602 * @param $key String:
604603 * @return integer
605604 */
606605 public function getFileSize( $key ) {
607 - if( !isset( $_FILES[$key] ) ) {
608 - return 0;
609 - }
610 - return $_FILES[$key]['size'];
 606+ $file = new WebRequestUpload( $key );
 607+ return $file->getSize();
611608 }
612609
613610 /**
——@@ -616,10 +613,8 @@
617614 * @return integer
618615 */
619616 public function getUploadError( $key ) {
620 - if( !isset( $_FILES[$key] ) || !isset( $_FILES[$key]['error'] ) ) {
621 - return 0/*UPLOAD_ERR_OK*/;
622 - }
623 - return $_FILES[$key]['error'];
 617+ $file = new WebRequestUpload( $key );
 618+ return $file->getError();
624619 }
625620
626621 /**
——@@ -634,19 +629,19 @@
635630 * @return string or NULL if no such file.
636631 */
637632 public function getFileName( $key ) {
638 - global $wgContLang;
639 - if( !isset( $_FILES[$key] ) ) {
640 - return null;
641 - }
642 - $name = $_FILES[$key]['name'];
643 -
644 - # Safari sends filenames in HTML-encoded Unicode form D...
645 - # Horrid and evil! Let's try to make some kind of sense of it.
646 - $name = Sanitizer::decodeCharReferences( $name );
647 - $name = $wgContLang->normalize( $name );
648 - wfDebug( "WebRequest::getFileName() '" . $_FILES[$key]['name'] . "' normalized to '$name'\n" );
649 - return $name;
 633+ $file = new WebRequestUpload( $key );
 634+ return $file->getName();
650635 }
 636+
 637+ /**
 638+ * Return a WebRequestUpload object corresponding to the key
 639+ *
 640+ * @param @key string
 641+ * @return WebRequestUpload
 642+ */
 643+ public function getUpload( $key ) {
 644+ return new WebRequestUpload( $key );
 645+ }
651646
652647 /**
653648 * Return a handle to WebResponse style object, for setting cookies,
——@@ -770,6 +765,96 @@
771766 }
772767
773768 /**
 769+ * Object to access the $_FILES array
 770+ */
 771+class WebRequestUpload {
 772+ protected $doesExist;
 773+ protected $fileInfo;
 774+
 775+ /**
 776+ * Constructor. Should only be called by WebRequest
 777+ *
 778+ * @param $key string Key in $_FILES array (name of form field)
 779+ */
 780+ public function __construct( $key ) {
 781+ $this->doesExist = isset( $_FILES[$key] );
 782+ if ( $this->doesExist ) {
 783+ $this->fileInfo = $_FILES[$key];
 784+ }
 785+ }
 786+
 787+ /**
 788+ * Return whether a file with this name was uploaded.
 789+ *
 790+ * @return bool
 791+ */
 792+ public function exists() {
 793+ return $this->doesExist;
 794+ }
 795+
 796+ /**
 797+ * Return the original filename of the uploaded file
 798+ *
 799+ * @return mixed Filename or null if non-existent
 800+ */
 801+ public function getName() {
 802+ if ( !$this->exists() ) {
 803+ return null;
 804+ }
 805+
 806+ global $wgContLang;
 807+ $name = $this->fileInfo['name'];
 808+
 809+ # Safari sends filenames in HTML-encoded Unicode form D...
 810+ # Horrid and evil! Let's try to make some kind of sense of it.
 811+ $name = Sanitizer::decodeCharReferences( $name );
 812+ $name = $wgContLang->normalize( $name );
 813+ wfDebug( __METHOD__ . ": {$this->fileInfo['name']} normalized to '$name'\n" );
 814+ return $name;
 815+ }
 816+
 817+ /**
 818+ * Return the file size of the uploaded file
 819+ *
 820+ * @return int File size or zero if non-existent
 821+ */
 822+ public function getSize() {
 823+ if ( !$this->exists() ) {
 824+ return 0;
 825+ }
 826+
 827+ return $this->fileInfo['size'];
 828+ }
 829+
 830+ /**
 831+ * Return the path to the temporary file
 832+ *
 833+ * @return mixed Path or null if non-existent
 834+ */
 835+ public function getTempName() {
 836+ if ( !$this->exists() ) {
 837+ return null;
 838+ }
 839+
 840+ return $this->fileInfo['tmp_name'];
 841+ }
 842+
 843+ /**
 844+ * Return the upload error. See link for explanation
 845+ * http://www.php.net/manual/en/features.file-upload.errors.php
 846+ *
 847+ * @return int One of the UPLOAD_ constants, 0 if non-existent
 848+ */
 849+ public function getError() {
 850+ if ( !$this->exists() ) {
 851+ return 0; # UPLOAD_ERR_OK
 852+ }
 853+
 854+ return $this->fileInfo['error'];
 855+ }
 856+}
 857+
 858+/**
774859 * WebRequest clone which takes values from a provided array.
775860 *
776861 * @ingroup HTTP
Index: trunk/phase3/includes/AutoLoader.php
——@@ -228,6 +228,7 @@
229229 'WatchedItem' => 'includes/WatchedItem.php',
230230 'WatchlistEditor' => 'includes/WatchlistEditor.php',
231231 'WebRequest' => 'includes/WebRequest.php',
 232+ 'WebRequestUpload' => 'includes/WebRequest.php',
232233 'WebResponse' => 'includes/WebResponse.php',
233234 'WikiError' => 'includes/WikiError.php',
234235 'WikiErrorMsg' => 'includes/WikiError.php',
Index: trunk/phase3/RELEASE-NOTES
——@@ -253,6 +253,8 @@
254254 page.
255255 * (bug 24517) LocalFile::newFromKey() and OldLocalFile::newFromKey() no longer
256256 throw fatal errors
 257+* (bug 23380) Uploaded files that are larger than allowed by PHP now show a
 258+ useful error message.
257259
258260 === API changes in 1.17 ===
259261 * (bug 22738) Allow filtering by action type on query=logevent.

Follow-up revisions

RevisionCommit summaryAuthorDate
r70043Follow-up r70037: Move isIniSizeOverflow magic to WebRequestUploadbtongminh20:54, 27 July 2010
r70049Follow-up r70037: Fix ApiUpload by passing a WebRequestUpload to the the init...btongminh21:53, 27 July 2010
r76093Merge the WebRequest part of r70037 to unbreak UploadStashcatrope14:02, 5 November 2010
r76096Merge UploadFromFile.php changes in r70037 and r70049, but without the max fi...catrope14:42, 5 November 2010

Comments

#Comment by 😂 (talk | contribs)   13:13, 7 February 2011

I like objects too. Especially objects that hide nasty superglobals :)

Status & tagging log