User "Catrope" posted a comment on MediaWiki.r92009.
Full URL:
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/92009#c20805
Commit summary:
Refactored UploadStash and related classes to use the database for file
metadata storage instead of the session, see bug 26179
Tweaked the UploadWizard to work properly with the new backend code, updated
tests
Comment:
<pre>
+ return self::isValidKey( $request->getText( 'wpFileKey' ) ||
$request->getText( 'wpSessionKey' ) );
...
+ $fileKey = $request->getText( 'wpFileKey' ) ||
$request->getText( 'wpSessionKey' );
...
+ $desiredDestName = $request->getText( 'wpUploadFile' )
|| $request->getText( 'filename' );
</pre>
This doesn't work. The <code>||</code> operator in PHP doesn't behave like the
one in JavaScript or Python or whatever, but more like the one in C:
<pre>
> $request = new FauxRequest(array('wpFileKey' => 'foo', 'wpSessionKey' =>
> 'bar'));
> var_dump($request->getText( 'wpSessionKey' ) || $request->getText(
> 'wpSessionKey' ))
bool(true)
</pre>
Instead, what you want to use is the second parameter to <code>getText()</code>
(and most of the other getters in WebRequest), which is a default value that is
returned if the key you asked for isn't set. So something like
<code>$request->getText( 'wpFileKey', $request->getText( 'wpSessionKey',
'defaultValueIfNeitherIsSet,DefaultsToEmptyString' ) )</code> would do what you
want.
I'm confused, though: I don't have a very good understanding of the code, but
it seems to me like this accidental creation of booleans that then get
converted back to strings must break '''something''', somewhere. Is there some
devious reason why this doesn't break things, or at least doesn't appear to
during basic testing?
<pre>
+ if ( !empty( $this->mLocalFile ) ) {
</pre>
This looks a lot like an [[Manual:Coding conventions#PHP
pitfalls|inappropriate]] use of <code>empty()</code>.
The waiting-for-lag behavior is unnecessarily complex, scary and fragile. If
your data doesn't appear due to lag, fetch it from the master instead. This
probably involves adding a parameter to <code>fetchFileMetadata()</code> to use
the master instead of the slave.
<pre>
public function listFiles() {
...
array( 'us_key' => $key ),
</pre>
<code>$key</code> is undefined, and this condition probably breaks
<code>listFiles()</code> entirely.
<pre>
+ $this->fileMetadata[$key] = array(
+ 'us_user' => $row->us_user,
+ 'us_key' => $row->us_key,
+ 'us_orig_path' => $row->us_orig_path,
+ 'us_path' => $row->us_path,
+ 'us_size' => $row->us_size,
+ 'us_sha1' => $row->us_sha1,
+ 'us_mime' => $row->us_mime,
+ 'us_media_type' => $row->us_media_type,
+ 'us_image_width' => $row->us_image_width,
+ 'us_image_height' => $row->us_image_height,
+ 'us_image_bits' => $row->us_image_bits,
+ 'us_source_type' => $row->us_source_type,
+ 'us_timestamp' => $row->us_timestamp,
+ 'us_status' => $row->us_status
+ );
</pre>
You can also use <code>$this->fileMetadata[$key] = (array)$row;</code> .
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
