r77908 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r77907‎ | r77908 | r77909 >
Date:20:57, 6 December 2010
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
(bug 26130) Revert changes to WebStart.php in r72349, which turn out to have been misguided. This should fix double-gzip issues
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/WebStart.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadStash.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadStash.php
@@ -9,7 +9,6 @@
1010 * - enable the uploading user (and *ONLY* the uploading user) to access said files, and thumbnails of said files, via a URL.
1111 * We accomplish this by making the session serve as a URL->file mapping, on the assumption that nobody else can access
1212 * the session, even the uploading user. See SpecialUploadStash, which implements a web interface to some files stored this way.
13 - *
1413 */
1514 class UploadStash {
1615
@@ -22,6 +21,12 @@
2322
2423 // array of initialized objects obtained from session (lazily initialized upon getFile())
2524 private $files = array();
 25+
 26+ // Session ID
 27+ private $sessionID;
 28+
 29+ // Cache to store stash metadata in
 30+ private $cache;
2631
2732 // TODO: Once UploadBase starts using this, switch to use these constants rather than UploadBase::SESSION*
2833 // const SESSION_VERSION = 2;
@@ -41,14 +46,13 @@
4247
4348 $this->repo = $repo;
4449
45 - if ( ! isset( $_SESSION ) ) {
46 - throw new UploadStashNotAvailableException( 'no session variable' );
 50+ if ( session_id() === '' ) {
 51+ // FIXME: Should we just start a session in this case?
 52+ // Anonymous uploading could be allowed
 53+ throw new UploadStashNotAvailableException( 'no session ID' );
4754 }
48 -
49 - if ( !isset( $_SESSION[UploadBase::SESSION_KEYNAME] ) ) {
50 - $_SESSION[UploadBase::SESSION_KEYNAME] = array();
51 - }
52 -
 55+ $this->sessionID = '';
 56+ $this->cache = wfGetCache( CACHE_ANYTHING );
5357 }
5458
5559 /**
@@ -64,32 +68,37 @@
6569 if ( ! preg_match( self::KEY_FORMAT_REGEX, $key ) ) {
6670 throw new UploadStashBadPathException( "key '$key' is not in a proper format" );
6771 }
68 -
 72+
6973 if ( !isset( $this->files[$key] ) ) {
70 - if ( !isset( $_SESSION[UploadBase::SESSION_KEYNAME][$key] ) ) {
 74+ $cacheKey = wfMemcKey( 'uploadstash', $this->sessionID, $key );
 75+ $data = $this->cache->get( $cacheKey );
 76+ if ( !$data ) {
7177 throw new UploadStashFileNotFoundException( "key '$key' not found in stash" );
7278 }
7379
74 - $data = $_SESSION[UploadBase::SESSION_KEYNAME][$key];
75 - // guards against PHP class changing while session data doesn't
76 - if ($data['version'] !== UploadBase::SESSION_VERSION ) {
77 - throw new UploadStashBadVersionException( $data['version'] . " does not match current version " . UploadBase::SESSION_VERSION );
78 - }
79 -
80 - // separate the stashData into the path, and then the rest of the data
81 - $path = $data['mTempPath'];
82 - unset( $data['mTempPath'] );
 80+ $this->files[$key] = $this->getFileFromData( $data );
8381
84 - $file = new UploadStashFile( $this, $this->repo, $path, $key, $data );
85 - if ( $file->getSize === 0 ) {
86 - throw new UploadStashZeroLengthFileException( "File is zero length" );
87 - }
88 - $this->files[$key] = $file;
89 -
9082 }
9183 return $this->files[$key];
9284 }
 85+
 86+ protected function getFileFromData( $data ) {
 87+ // guards against PHP class changing while session data doesn't
 88+ if ( $data['version'] !== UploadBase::SESSION_VERSION ) {
 89+ throw new UploadStashBadVersionException( $data['version'] . " does not match current version " . UploadBase::SESSION_VERSION );
 90+ }
 91+
 92+ // separate the stashData into the path, and then the rest of the data
 93+ $path = $data['mTempPath'];
 94+ unset( $data['mTempPath'] );
9395
 96+ $file = new UploadStashFile( $this, $this->repo, $path, $key, $data );
 97+ if ( $file->getSize() === 0 ) {
 98+ throw new UploadStashZeroLengthFileException( "File is zero length" );
 99+ }
 100+ return $file;
 101+ }
 102+
94103 /**
95104 * Stash a file in a temp directory and record that we did this in the session, along with other metadata.
96105 * We store data in a flat key-val namespace because that's how UploadBase did it. This also means we have to
@@ -163,10 +172,15 @@
164173
165174 // now, merge required info and extra data into the session. (The extra data changes from application to application.
166175 // UploadWizard wants different things than say FirefoggChunkedUpload.)
 176+ $finalData = array_merge( $data, $requiredData );
 177+
 178+ global $wgUploadStashExpiry;
167179 wfDebug( __METHOD__ . " storing under $key\n" );
168 - $_SESSION[UploadBase::SESSION_KEYNAME][$key] = array_merge( $data, $requiredData );
 180+ $cacheKey = wfMemcKey( 'uploadstash', $this->sessionID, $key );
 181+ $this->cache->set( $cacheKey, array_merge( $data, $requiredData ), $wgUploadStashExpiry );
169182
170 - return $this->getFile( $key );
 183+ $this->files[$key] = $this->getFileFromData( $data );
 184+ return $this->files[$key];
171185 }
172186
173187 /**
Index: trunk/phase3/includes/WebStart.php
@@ -126,15 +126,10 @@
127127
128128 wfProfileIn( 'WebStart.php-ob_start' );
129129 # Initialise output buffering
130 -
131130 # Check that there is no previous output or previously set up buffers, because
132131 # that would cause us to potentially mix gzip and non-gzip output, creating a
133132 # big mess.
134 -# In older versions of PHP ob_get_level() returns 0 if there is no buffering or
135 -# previous output, in newer versions the default output buffer is always set up
136 -# and ob_get_level() returns 1. In this case we check that the buffer is empty.
137 -# FIXME: Check that this is the right way to handle this
138 -if ( !defined( 'MW_NO_OUTPUT_BUFFER' ) && ( ob_get_level() == 0 || ( ob_get_level() == 1 && ob_get_contents() === '' ) ) ) {
 133+if ( !defined( 'MW_NO_OUTPUT_BUFFER' ) && ob_get_level() == 0 ) {
139134 require_once( "$IP/includes/OutputHandler.php" );
140135 ob_start( 'wfOutputHandler' );
141136 }
Index: trunk/phase3/includes/DefaultSettings.php
@@ -959,6 +959,11 @@
960960 */
961961 $wgDjvuOutputExtension = 'jpg';
962962
 963+/**
 964+ * How long (in seconds) stashed uploads are kept in cache.
 965+ */
 966+$wgUploadStashExpiry = 3600; // 1 hour
 967+
963968 /** @} */ # end of file uploads }
964969
965970 /************************************************************************//**

Follow-up revisions

RevisionCommit summaryAuthorDate
r77909Revert unintended changes in r77908catrope21:01, 6 December 2010
r78199(bug 26130) ob_start( 'ob_gzhandler' ) in LocalSettings.php broke gzip output...catrope17:06, 10 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r72349Merging resourceloader branch into trunk. Full documentation is at http://www...catrope04:00, 4 September 2010

Comments

#Comment by MaxSem (talk | contribs)   20:59, 6 December 2010

Are the other changes intended?

#Comment by Catrope (talk | contribs)   21:00, 6 December 2010

CRAP

#Comment by Catrope (talk | contribs)   21:02, 6 December 2010

Reverted in r77909, thanks for spotting this in two minutes :D

#Comment by Platonides (talk | contribs)   22:40, 6 December 2010

I wonder if it was related to gzip not working (Bug 26243)

#Comment by Catrope (talk | contribs)   12:04, 7 December 2010

It couldn't be. The reporter is using 1.16.0 and this reverts a change introduced in September, well after the 1.16 release.

Status & tagging log