r92815 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r92814‎ | r92815 | r92816 >
Date:22:28, 21 July 2011
Author:raindrift
Status:ok (Comments)
Tags:
Comment:
Actually alias sessionkey to filekey, fixes bug in r92459
Modified paths:
  • /trunk/phase3/includes/api/ApiQueryStashImageInfo.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiQueryStashImageInfo.php
——@@ -45,6 +45,11 @@
4646 $this->dieUsage( "One of filekey or sessionkey must be supplied", 'nofilekey');
4747 }
4848
 49+ // Alias sessionkey to filekey, but give an existing filekey precedence.
 50+ if ( !$params['filekey'] && $params['sessionkey'] ) {
 51+ $params['filekey'] = $params['sessionkey'];
 52+ }
 53+
4954 try {
5055 $stash = RepoGroup::singleton()->getLocalRepo()->getUploadStash();
5156

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r92459changed sessionkey to filekey to keep consistent with the rest of UploadStash...raindrift19:31, 18 July 2011

Comments

#Comment by Catrope (talk | contribs)   21:19, 22 July 2011 
+		if ( !$params['filekey'] && $params['sessionkey'] ) {

I missed a similar thing in the previous rev, but: you should be checking for !== null instead of casting to boolean. The latter has lots of nasty and usually unintended effects, because things like the empty string or the string '0' can also evaluate to false.

#Comment by Raindrift (talk | contribs)   22:04, 22 July 2011

Totally, but I did that on purpose: in this case, it fails in the correct direction: I intend to disallow 0 (string or number), empty string, and null. None of those are valid session keys, and I can imagine cases where one might be supplied, especially if the caller is written in a weakly typed language. There's a failure much later that rejects keys that don't match a regexp, but passing such cases through seems like a less graceful way to handle it.

#Comment by Catrope (talk | contribs)   22:08, 22 July 2011

Alright, as long as you know what the side effects are...

Status & tagging log