MediaWiki r58079 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r58078‎ | r58079 (on ViewVC)‎ | r58080 >
Date:04:36, 24 October 2009
Author:mrzman
Status:resolved (Comments)
Tags:
Comment:
(bug 18019) Warn users when moving a file to a name in use on a shared repo.
Allow only users with the 'reupload-shared' right to complete the move.
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r58080Follow-up r58079: Add new messages to maintenance scriptsraymond06:24, 24 October 2009
r60418Per CR on r58079, add a permission check for 'reupload-shared' to...mrzman21:56, 26 December 2009

Comments

#Comment by Bryan (talk | contribs)   22:05, 28 October 2009

Permissions checking should be down on a lower level, i.e. in Title::isValidMoveOperation.

#Comment by Mr.Z-man (talk | contribs)   17:14, 30 October 2009

I can add a permission check to Title::isValidMoveOperation, but it will mainly just increase code duplication. For it to be able to provide a different warning with the option to override to users who can move it, versus a "you can't do this" warning to users who can't, it still needs to do the permission check in SpecialMovepage/ApiMove.

#Comment by Bryan (talk | contribs)   22:02, 21 December 2009

Title::isValidMoveOperation can return detailed error information, so supplying whether or not the user is actually allowed to complete the move can be supplied by it.

#Comment by Mr.Z-man (talk | contribs)   00:23, 22 December 2009

The issue is that it needs to be overridden for users who do have permission, but it still needs to give a warning on the initial attempt, which means it needs to be able to distinguish between the initial attempt, and the attempt after the warning. Title::isValidMoveOperation is only called directly in SpecialMovepage.php in rare situations, normally its only called in Title::moveTo, where its too late to modify the submit button on the form.

#Comment by Tim Starling (talk | contribs)   04:27, 23 December 2009

I think it's OK to duplicate these few lines of code in this case, to be on the safe side. The file object cache in RepoGroup should prevent a second DB query from being generated, so it's just an extra few microseconds. You can avoid a second check for the file on the local repo, the equivalent of your

&& !RepoGroup::singleton()->getLocalRepo()->findFile( $nt ) 

by reusing the results from the following code in isValidMoveOperation():

$file = wfLocalFile( $this );
if( $file->exists() ) {

They have the same effect.

#Comment by Bryan (talk | contribs)   21:14, 24 December 2009

I'm not quire concerned about performance, but about the fact that having a permissions check outside a place where you actually suspect all permission checking to be done is error-prone. That said, seeing that it is very hard to fix this a proper way, I do not think this is a blocking bug.

#Comment by Mr.Z-man (talk | contribs)   22:05, 26 December 2009

Added permission check in Title::isValidMoveOperation in r60418/r60420.

Status & tagging log

  • 06:47, 30 December 2009 Tim Starling (talk | contribs) changed the status of r58079 [removed: fixme added: resolved]
  • 22:05, 28 October 2009 Bryan (talk | contribs) changed the status of r58079 [removed: new added: fixme]