r83979 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r83978‎ | r83979 | r83980 >
Date:23:33, 14 March 2011
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
* (bug 28034) uploading file to local wiki when file exists on shared repository (commons) gives spurious info in the warning message

Applying patch with one minor addition
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/specials/SpecialUpload.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadBase.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/upload/UploadBase.php
——@@ -473,7 +473,7 @@
474474
475475 $overwriteError = $this->checkOverwrite( $user );
476476 if ( $overwriteError !== true ) {
477 - return array( array( $overwriteError ) );
 477+ return array( $overwriteError );
478478 }
479479
480480 return true;
——@@ -1101,14 +1101,14 @@
11021102 *
11031103 * @param $user User
11041104 *
1105 - * @return mixed true on success, error string on failure
 1105+ * @return mixed true on success, array on failure
11061106 */
11071107 private function checkOverwrite( $user ) {
11081108 // First check whether the local file can be overwritten
11091109 $file = $this->getLocalFile();
11101110 if( $file->exists() ) {
11111111 if( !self::userCanReUpload( $user, $file ) ) {
1112 - return 'fileexists-forbidden';
 1112+ return array( 'fileexists-forbidden', $file->getName() );
11131113 } else {
11141114 return true;
11151115 }
——@@ -1119,7 +1119,7 @@
11201120 */
11211121 $file = wfFindFile( $this->getTitle() );
11221122 if ( $file && !$user->isAllowed( 'reupload-shared' ) ) {
1123 - return 'fileexists-shared-forbidden';
 1123+ return array( 'fileexists-shared-forbidden', $file->getName() );
11241124 }
11251125
11261126 return true;
Index: trunk/phase3/includes/specials/SpecialUpload.php
——@@ -455,8 +455,8 @@
456456 $permErrors = $this->mUpload->verifyPermissions( $wgUser );
457457 if( $permErrors !== true ) {
458458 $code = array_shift( $permErrors[0] );
459 - $this->showRecoverableUploadError( wfMsgExt( $code,
460 - 'parseinline', $permErrors[0] ) );
 459+ $this->showRecoverableUploadError( wfMsgExt( $code[0],
 460+ 'parseinline', $code[1] ) );
461461 return;
462462 }
463463
Index: trunk/phase3/RELEASE-NOTES
——@@ -191,7 +191,9 @@
192192 * Do not show enotifminoredits preference, if disabled by $wgEnotifMinorEdits.
193193 * AbortLogin returning "ABORTED" now handled. Also allows message identifier
194194 for "ABORTED" reason to be returned and displayed to user.
195 -
 195+* (bug 28034) uploading file to local wiki when file exists on shared repository
 196+ (commons) gives spurious info in the warning message
 197+
196198 === API changes in 1.18 ===
197199 * (bug 26339) Throw warning when truncating an overlarge API result
198200 * (bug 14869) Add API module for accessing QueryPage-based special pages

Follow-up revisions

RevisionCommit summaryAuthorDate
r844201.17wmf1: MFT r76372, r76377, r83586, r83587, r83817, r83876, r83979, r84118,...catrope21:04, 20 March 2011
r84577* (bug 28166) UploadBase assumes that 'edit' and 'upload' rights are not per ...reedy22:36, 22 March 2011
r84579Partial revert to r83979...reedy22:49, 22 March 2011
r85434MFT: r83885, r83891, r83897, r83902, r83903, r83934, r83965, r83979, r83988, ...demon13:38, 5 April 2011

Comments

#Comment by Catrope (talk | contribs)   19:47, 20 March 2011 
 			$code = array_shift( $permErrors[0] );
-			$this->showRecoverableUploadError( wfMsgExt( $code,
-					'parseinline', $permErrors[0] ) );
+			$this->showRecoverableUploadError( wfMsgExt( $code[0],
+					'parseinline', $code[1] ) );

This part looks wrong to me: how will this work for errors with more than one parameter?

Looks good otherwise.

#Comment by Reedy (talk | contribs)   19:51, 20 March 2011

It seemed to me that things bailed after one error, so there wasn't the possibility of getting > 1 errors. 

	private function checkOverwrite( $user ) {
		// First check whether the local file can be overwritten
		$file = $this->getLocalFile();
		if( $file->exists() ) {
			if( !self::userCanReUpload( $user, $file ) ) {
				return array( 'fileexists-forbidden', $file->getName() );
			} else {
				return true;
			}
		}

		/* Check shared conflicts: if the local file does not exist, but
		 * wfFindFile finds a file, it exists in a shared repository.
		 */
		$file = wfFindFile( $this->getTitle() );
		if ( $file && !$user->isAllowed( 'reupload-shared' ) ) {
			return array( 'fileexists-shared-forbidden', $file->getName() );
		}

		return true;
	}

etc. I don't know if we need to ask Bryan to have a look as he's more familiar with the area of code...

The array( array( array( array( ) ) ) esk syntax is quite difficult to follow sometimes ;)

#Comment by Catrope (talk | contribs)   19:56, 20 March 2011

I meant, what will happen if the function ever changes and returns errors that take more than one parameter?

#Comment by Reedy (talk | contribs)   20:01, 20 March 2011

Oh. You're meaning just take off the message, and push whatever is left to wfMsgExt() to deal with it?

#Comment by Nikerabbit (talk | contribs)   20:02, 20 March 2011

wfMessage() exists. Wouldn't it fit here perfectly?

#Comment by Reedy (talk | contribs)   20:09, 20 March 2011

Dunno. What's the difference? :)

#Comment by Nikerabbit (talk | contribs)   20:26, 20 March 2011

You return an object which contains all the necessary parameters. The caller can then use it to extract the text in a way it wants.

#Comment by Reedy (talk | contribs)   22:19, 20 March 2011

So, what would it change into...?

#Comment by Nikerabbit (talk | contribs)   07:48, 21 March 2011

To return wfMessage( 'key', $a, $b, $c ); and caller should then call $foo->parse() or whatever;

#Comment by Reedy (talk | contribs)   22:39, 22 March 2011

Reverting 

-			$this->showRecoverableUploadError( wfMsgExt( $code,
-					'parseinline', $permErrors[0] ) );
+			$this->showRecoverableUploadError( wfMsgExt( $code[0],
+					'parseinline', $code[1] ) );

"fixes" the broken error messages for TitleBlacklist. Marking fixme till I fix the other error messages...

#Comment by Bryan (talk | contribs)   11:17, 27 March 2011

Subscribing myself to follow-up mails as a reminder that I should review this.

Status & tagging log