r78426 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78425‎ | r78426 | r78427 >
Date:01:21, 15 December 2010
Author:neilk
Status:ok (Comments)
Tags:
Comment:
added a page to list all stashed files, and clear them if necessary -- helpful while debugging some concurrency issues, also gets us closer to keeping the UploadWizard state on the server
Modified paths:
  • /trunk/phase3/includes/specials/SpecialUploadStash.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadStash.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadStash.php
@@ -30,17 +30,12 @@
3131 /**
3232 * Represents the session which contains temporarily stored files.
3333 * Designed to be compatible with the session stashing code in UploadBase (should replace it eventually)
34 - *
35 - * @param $repo FileRepo: optional -- repo in which to store files. Will choose LocalRepo if not supplied.
3634 */
37 - public function __construct( $repo = null ) {
 35+ public function __construct() {
3836
39 - if ( is_null( $repo ) ) {
40 - $repo = RepoGroup::singleton()->getLocalRepo();
41 - }
 37+ // this might change based on wiki's configuration.
 38+ $this->repo = RepoGroup::singleton()->getLocalRepo();
4239
43 - $this->repo = $repo;
44 -
4540 if ( ! isset( $_SESSION ) ) {
4641 throw new UploadStashNotAvailableException( 'no session variable' );
4742 }
@@ -51,6 +46,7 @@
5247
5348 }
5449
 50+
5551 /**
5652 * Get a file and its metadata from the stash.
5753 * May throw exception if session data cannot be parsed due to schema change, or key not found.
@@ -81,7 +77,7 @@
8278 unset( $data['mTempPath'] );
8379
8480 $file = new UploadStashFile( $this, $this->repo, $path, $key, $data );
85 - if ( $file->getSize === 0 ) {
 81+ if ( $file->getSize() === 0 ) {
8682 throw new UploadStashZeroLengthFileException( "File is zero length" );
8783 }
8884 $this->files[$key] = $file;
@@ -170,6 +166,36 @@
171167 }
172168
173169 /**
 170+ * Remove all files from the stash.
 171+ * Does not clean up files in the repo, just the record of them.
 172+ * @return boolean: success
 173+ */
 174+ public function clear() {
 175+ $_SESSION[UploadBase::SESSION_KEYNAME] = array();
 176+ return true;
 177+ }
 178+
 179+
 180+ /**
 181+ * Remove a particular file from the stash.
 182+ * Does not clean up file in the repo, just the record of it.
 183+ * @return boolean: success
 184+ */
 185+ public function removeFile( $key ) {
 186+ unset ( $_SESSION[UploadBase::SESSION_KEYNAME][$key] );
 187+ return true;
 188+ }
 189+
 190+
 191+ /**
 192+ * List all files in the stash.
 193+ */
 194+ public function listFiles() {
 195+ return array_keys( $_SESSION[UploadBase::SESSION_KEYNAME] );
 196+ }
 197+
 198+
 199+ /**
174200 * Find or guess extension -- ensuring that our extension matches our mime type.
175201 * Since these files are constructed from php tempnames they may not start off
176202 * with an extension.
Index: trunk/phase3/includes/specials/SpecialUploadStash.php
@@ -20,6 +20,12 @@
2121 // UploadStash
2222 private $stash;
2323
 24+ // is the edit request authorized? boolean
 25+ private $isEditAuthorized;
 26+
 27+ // did the user request us to clear the stash? boolean
 28+ private $requestedClear;
 29+
2430 // Since we are directly writing the file to STDOUT,
2531 // we should not be reading in really big files and serving them out.
2632 //
@@ -30,18 +36,21 @@
3137 // uploading.
3238 const MAX_SERVE_BYTES = 262144; // 256K
3339
34 - public function __construct( ) {
 40+ public function __construct( $request = null ) {
 41+ global $wgRequest;
 42+
3543 parent::__construct( 'UploadStash', 'upload' );
3644 try {
3745 $this->stash = new UploadStash( );
38 - } catch (UploadStashNotAvailableException $e) {
 46+ } catch ( UploadStashNotAvailableException $e ) {
3947 return null;
4048 }
 49+
 50+ $this->loadRequest( is_null( $request ) ? $wgRequest : $request );
4151 }
4252
4353 /**
44 - * If file available in stash, cats it out to the client as a simple HTTP response.
45 - * n.b. Most sanity checking done in UploadStashLocalFile, so this is straightforward.
 54+ * Execute page -- can output a file directly or show a listing of them.
4655 *
4756 * @param $subPage String: subpage, e.g. in http://example.com/wiki/Special:UploadStash/foo.jpg, the "foo.jpg" part
4857 * @return Boolean: success
@@ -54,56 +63,62 @@
5564 return;
5665 }
5766
 67+ if ( !isset( $subPage ) || $subPage === '' ) {
 68+ return $this->showUploads();
 69+ }
 70+
 71+ return $this->showUpload( $subPage );
 72+ }
 73+
 74+
 75+ /**
 76+ * If file available in stash, cats it out to the client as a simple HTTP response.
 77+ * n.b. Most sanity checking done in UploadStashLocalFile, so this is straightforward.
 78+ *
 79+ * @param $key String: the key of a particular requested file
 80+ */
 81+ public function showUpload( $key ) {
 82+ global $wgOut;
 83+
5884 // prevent callers from doing standard HTML output -- we'll take it from here
5985 $wgOut->disable();
6086
61 - if ( !isset( $subPage ) || $subPage === '' ) {
62 - // the user probably visited the page just to see what would happen, so explain it a bit.
63 - $code = '400';
64 - $message = "Missing key\n\n"
65 - . 'This page provides access to temporarily stashed files for the user that '
66 - . 'uploaded those files. See the upload API documentation. To access a stashed file, '
67 - . 'use the URL of this page, with a slash and the key of the stashed file appended.';
68 - } else {
69 - try {
70 - if ( preg_match( '/^(\d+)px-(.*)$/', $subPage, $matches ) ) {
71 - list( /* full match */, $width, $key ) = $matches;
72 - return $this->outputThumbFromStash( $key, $width );
73 - } else {
74 - return $this->outputFileFromStash( $subPage );
75 - }
76 - } catch( UploadStashFileNotFoundException $e ) {
77 - $code = 404;
78 - $message = $e->getMessage();
79 - } catch( UploadStashZeroLengthFileException $e ) {
80 - $code = 500;
81 - $message = $e->getMessage();
82 - } catch( UploadStashBadPathException $e ) {
83 - $code = 500;
84 - $message = $e->getMessage();
85 - } catch( SpecialUploadStashTooLargeException $e ) {
86 - $code = 500;
87 - $message = 'Cannot serve a file larger than ' . self::MAX_SERVE_BYTES . ' bytes. ' . $e->getMessage();
88 - } catch( Exception $e ) {
89 - $code = 500;
90 - $message = $e->getMessage();
 87+ try {
 88+ if ( preg_match( '/^(\d+)px-(.*)$/', $key, $matches ) ) {
 89+ list( /* full match */, $width, $key ) = $matches;
 90+ return $this->outputThumbFromStash( $key, $width );
 91+ } else {
 92+ return $this->outputFileFromStash( $key );
9193 }
 94+ } catch( UploadStashFileNotFoundException $e ) {
 95+ $code = 404;
 96+ $message = $e->getMessage();
 97+ } catch( UploadStashZeroLengthFileException $e ) {
 98+ $code = 500;
 99+ $message = $e->getMessage();
 100+ } catch( UploadStashBadPathException $e ) {
 101+ $code = 500;
 102+ $message = $e->getMessage();
 103+ } catch( SpecialUploadStashTooLargeException $e ) {
 104+ $code = 500;
 105+ $message = 'Cannot serve a file larger than ' . self::MAX_SERVE_BYTES . ' bytes. ' . $e->getMessage();
 106+ } catch( Exception $e ) {
 107+ $code = 500;
 108+ $message = $e->getMessage();
92109 }
93110
94111 wfHttpError( $code, OutputPage::getStatusMessage( $code ), $message );
95112 return false;
96113 }
97 -
 114+
98115 /**
99116 * Get a file from stash and stream it out. Rely on parent to catch exceptions and transform them into HTTP
100117 * @param String: $key - key of this file in the stash, which probably looks like a filename with extension.
101 - * @throws ....?
102118 * @return boolean
103119 */
104120 private function outputFileFromStash( $key ) {
105121 $file = $this->stash->getFile( $key );
106 - $this->outputLocalFile( $file );
107 - return true;
 122+ return $this->outputLocalFile( $file );
108123 }
109124
110125
@@ -111,11 +126,13 @@
112127 * Get a thumbnail for file, either generated locally or remotely, and stream it out
113128 * @param String $key: key for the file in the stash
114129 * @param int $width: width of desired thumbnail
115 - * @return ??
 130+ * @return boolean success
116131 */
117132 private function outputThumbFromStash( $key, $width ) {
118133
119134 // this global, if it exists, points to a "scaler", as you might find in the Wikimedia Foundation cluster. See outputRemoteScaledThumb()
 135+ // this is part of our horrible NFS-based system, we create a file on a mount point here, but fetch the scaled file from somewhere else that
 136+ // happens to share it over NFS
120137 global $wgUploadStashScalerBaseUrl;
121138
122139 // let exceptions propagate to caller.
@@ -222,7 +239,7 @@
223240 if ( $file->getSize() > self::MAX_SERVE_BYTES ) {
224241 throw new SpecialUploadStashTooLargeException();
225242 }
226 - self::outputHeaders( $file->getMimeType(), $file->getSize() );
 243+ self::outputFileHeaders( $file->getMimeType(), $file->getSize() );
227244 readfile( $file->getPath() );
228245 return true;
229246 }
@@ -238,7 +255,7 @@
239256 if ( $size > self::MAX_SERVE_BYTES ) {
240257 throw new SpecialUploadStashTooLargeException();
241258 }
242 - self::outputHeaders( $contentType, $size );
 259+ self::outputFileHeaders( $contentType, $size );
243260 print $content;
244261 return true;
245262 }
@@ -250,13 +267,102 @@
251268 * @param String $contentType : string suitable for content-type header
252269 * @param String $size: length in bytes
253270 */
254 - private static function outputHeaders( $contentType, $size ) {
 271+ private static function outputFileHeaders( $contentType, $size ) {
255272 header( "Content-Type: $contentType", true );
256273 header( 'Content-Transfer-Encoding: binary', true );
257274 header( 'Expires: Sun, 17-Jan-2038 19:14:07 GMT', true );
258275 header( "Content-Length: $size", true );
259276 }
260277
 278+
 279+ /**
 280+ * Initialize authorization & actions to take, from the request
 281+ * @param $request: WebRequest
 282+ */
 283+ private function loadRequest( $request ) {
 284+ global $wgUser;
 285+ if ( $request->wasPosted() ) {
 286+
 287+ $token = $request->getVal( 'wpEditToken' );
 288+ $this->isEditAuthorized = $wgUser->matchEditToken( $token );
 289+
 290+ $this->requestedClear = $request->getBool( 'clear' );
 291+
 292+ }
 293+ }
 294+
 295+ /**
 296+ * Static callback for the HTMLForm in showUploads, to process
 297+ * Note the stash has to be recreated since this is being called in a static context.
 298+ * This works, because there really is only one stash per logged-in user, despite appearances.
 299+ *
 300+ * @return Status
 301+ */
 302+ public static function tryClearStashedUploads( $formData ) {
 303+ wfDebug( __METHOD__ . " form data : " . print_r( $formData, 1 ) );
 304+ if ( isset( $formData['clear'] ) and $formData['clear'] ) {
 305+ $stash = new UploadStash();
 306+ wfDebug( "stash has: " . print_r( $stash->listFiles(), 1 ) );
 307+ if ( ! $stash->clear() ) {
 308+ return Status::newFatal( 'uploadstash-errclear' );
 309+ }
 310+ }
 311+ return Status::newGood();
 312+ }
 313+
 314+ /**
 315+ * Default action when we don't have a subpage -- just show links to the uploads we have,
 316+ * Also show a button to clear stashed files
 317+ * @param Status : $status - the result of processRequest
 318+ */
 319+ private function showUploads( $status = null ) {
 320+ global $wgOut;
 321+ if ( $status === null ) {
 322+ $status = Status::newGood();
 323+ }
 324+
 325+ // sets the title, etc.
 326+ $this->setHeaders();
 327+ $this->outputHeader();
 328+
 329+
 330+ // create the form, which will also be used to execute a callback to process incoming form data
 331+ // this design is extremely dubious, but supposedly HTMLForm is our standard now?
 332+
 333+ $form = new HTMLForm( array( 'clear' => array( 'class' => 'HTMLHiddenField', 'default' => true ) ), 'clearStashedUploads' );
 334+ $form->setSubmitCallback( array( __CLASS__, 'tryClearStashedUploads' ) );
 335+ $form->setTitle( $this->getTitle() );
 336+ $form->addHiddenField( 'clear', true, array( 'type' => 'boolean' ) );
 337+ $form->setSubmitText( wfMsg( 'uploadstash-clear' ) );
 338+
 339+ $form->prepareForm();
 340+ $formResult = $form->tryAuthorizedSubmit();
 341+
 342+
 343+ // show the files + form, if there are any, or just say there are none
 344+ $refreshHtml = Html::element( 'a', array( 'href' => $this->getTitle()->getLocalURL() ), wfMsg( 'uploadstash-refresh' ) );
 345+ $files = $this->stash->listFiles();
 346+ if ( count( $files ) ) {
 347+ sort( $files );
 348+ $fileListItemsHtml = '';
 349+ foreach ( $files as $file ) {
 350+ $fileListItemsHtml .= Html::rawElement( 'li', array(),
 351+ Html::element( 'a', array( 'href' => $this->getTitle( $file )->getLocalURL() ), $file )
 352+ );
 353+ }
 354+ $wgOut->addHtml( Html::rawElement( 'ul', array(), $fileListItemsHtml ) );
 355+ $form->displayForm( $formResult );
 356+ $wgOut->addHtml( Html::rawElement( 'p', array(), $refreshHtml ) );
 357+ } else {
 358+ $wgOut->addHtml( Html::rawElement( 'p', array(),
 359+ Html::element( 'span', array(), wfMsg( 'uploadstash-nofiles' ) )
 360+ . ' '
 361+ . $refreshHtml
 362+ ) );
 363+ }
 364+
 365+ return true;
 366+ }
261367 }
262368
263369 class SpecialUploadStashTooLargeException extends MWException {};
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -2214,6 +2214,15 @@
22152215 'upload-unknown-size' => 'Unknown size',
22162216 'upload-http-error' => 'An HTTP error occured: $1',
22172217
 2218+# Special:UploadStash
 2219+'uploadstash' => 'Upload stash',
 2220+'uploadstash-summary' => 'This page provides access to files which are uploaded (or in the process of uploading) but are not yet published to the wiki. These files are not visible to anyone but the user who uploaded them. See the documentation for the upload API.',
 2221+'uploadstash-clear' => 'Clear stashed files',
 2222+'uploadstash-nofiles' => 'You have no stashed files.',
 2223+'uploadstash-badtoken' => 'We could not perform that action, perhaps because your editing credentials expired. Try again.',
 2224+'uploadstash-errclear' => 'We could not clear the files.',
 2225+'uploadstash-refresh' => 'Refresh the list of files',
 2226+
22182227 # img_auth script messages
22192228 'img-auth-accessdenied' => 'Access denied',
22202229 'img-auth-nopathinfo' => 'Missing PATH_INFO.

Follow-up revisions

RevisionCommit summaryAuthorDate
r78434Follow-up r78426: Tweak messages for consistency. Remove sentence about "API ...raymond13:07, 15 December 2010
r81401Cleanup SpecialUploadStash...btongminh16:07, 2 February 2011
r850001.17wmf1: MFT changes for UploadWizard deployment: r78426, r81393, r81401, r8...catrope08:20, 30 March 2011

Comments

#Comment by Bryan (talk | contribs)   16:09, 2 February 2011

Does this need to be backported to 1.17?

#Comment by Catrope (talk | contribs)   16:10, 2 February 2011

I think Neil and I agreed to keep the upcoming UploadStash changes out of 1.17 for now and patch them into 1.17wmfWhatever when they're ready.

#Comment by Bryan (talk | contribs)   16:13, 2 February 2011

This can be marked OK, but I don't know if Roan wants to look at all the UploadStash stuff.

#Comment by Catrope (talk | contribs)   16:16, 2 February 2011

If you think it's OK, mark it as OK. I have no burning desire to review UploadStash stuff given that you're probably better at it than I am, and I have plenty of other things on my plate right now.

#Comment by Bryan (talk | contribs)   17:02, 2 February 2011

Ok :)

#Comment by Mormegil (talk | contribs)   17:41, 26 March 2011

I am not sure whether you decided to patch the change into the wmf branch or not, but it seems half of it got there in r79129, resulting in PHP fatals on wmf production servers. See bugzilla:28236#c3.

Status & tagging log