r99441 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r99440‎ | r99441 | r99442 >
Date:22:43, 10 October 2011
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Remove some dupe comments

Member variables
Modified paths:
  • /trunk/extensions/SwiftMedia/SwiftMedia.body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/SwiftMedia/SwiftMedia.body.php
@@ -54,10 +54,17 @@
5555 /**#@-*/
5656
5757 /**
 58+ * @var SwiftRepo
 59+ */
 60+ protected $repo;
 61+
 62+ /**
5863 * Create a LocalFile from a title
5964 * Do not call this except from inside a repo class.
6065 *
6166 * Note: $unused param is only here to avoid an E_STRICT
 67+ *
 68+ * @return SwiftFile
6269 */
6370 static function newFromTitle( $title, $repo, $unused = null ) {
6471 if ( empty($title) ) {
@@ -213,7 +220,6 @@
214221 throw new MWException( __METHOD__.': not implemented' );
215222 }
216223
217 -
218224 /** getHandler inherited */
219225 /** iconThumb inherited */
220226 /** getLastError inherited */
@@ -332,6 +338,8 @@
333339 * Given a connection and container name, return the container.
334340 * We KNOW the container should exist, so puke if it doesn't.
335341 *
 342+ * @param $conn CF_Connection
 343+ *
336344 * @return CF_Container
337345 */
338346 function get_container($conn, $cont) {
@@ -370,7 +378,6 @@
371379 * Given a container and object name, delete the object.
372380 * None of these error conditions are recoverable by the user, so we just dump
373381 * an Internal Error on them.
374 - *
375382 */
376383 function swift_delete( $container, $rel ) {
377384 try {
@@ -401,7 +408,7 @@
402409
403410 // Validate each triplet
404411 $status = $this->newGood();
405 - foreach ( $triplets as $i => $triplet ) {
 412+ foreach ( $triplets as $triplet ) {
406413 list( $srcPath, $dstZone, $dstRel ) = $triplet;
407414
408415 if ( !$this->validateFilename( $dstRel ) ) {
@@ -1192,16 +1199,16 @@
11931200 /**
11941201 * Foreign file with an accessible MediaWiki database
11951202 *
1196 - * @file
11971203 * @ingroup FileRepo
11981204 */
 1205+class SwiftForeignDBFile extends SwiftFile {
11991206
1200 -/**
1201 - * Foreign file with an accessible MediaWiki database
1202 - *
1203 - * @ingroup FileRepo
1204 - */
1205 -class SwiftForeignDBFile extends SwiftFile {
 1207+ /**
 1208+ * @param $title
 1209+ * @param $repo
 1210+ * @param $unused
 1211+ * @return SwiftForeignDBFile
 1212+ */
12061213 static function newFromTitle( $title, $repo, $unused = null ) {
12071214 return new self( $title, $repo );
12081215 }
@@ -1225,12 +1232,15 @@
12261233 $watch = false, $timestamp = false ) {
12271234 $this->readOnlyError();
12281235 }
 1236+
12291237 function restore( $versions = array(), $unsuppress = false ) {
12301238 $this->readOnlyError();
12311239 }
 1240+
12321241 function delete( $reason, $suppress = false ) {
12331242 $this->readOnlyError();
12341243 }
 1244+
12351245 function move( $target ) {
12361246 $this->readOnlyError();
12371247 }
@@ -1249,15 +1259,8 @@
12501260 /**
12511261 * A foreign repository with an accessible MediaWiki database
12521262 *
1253 - * @file
12541263 * @ingroup FileRepo
12551264 */
1256 -
1257 -/**
1258 - * A foreign repository with an accessible MediaWiki database
1259 - *
1260 - * @ingroup FileRepo
1261 - */
12621265 class SwiftForeignDBRepo extends SwiftRepo {
12631266 # Settings
12641267 var $dbType, $dbServer, $dbUser, $dbPassword, $dbName, $dbFlags,
@@ -1280,6 +1283,9 @@
12811284 $this->hasSharedCache = $info['hasSharedCache'];
12821285 }
12831286
 1287+ /**
 1288+ * @return DatabaseBase
 1289+ */
12841290 function getMasterDB() {
12851291 wfDebug( __METHOD__.": {$this->dbServer}\n" );
12861292 if ( !isset( $this->dbConn ) ) {
@@ -1297,6 +1303,9 @@
12981304 return $this->dbConn;
12991305 }
13001306
 1307+ /**
 1308+ * @return DatabaseBase
 1309+ */
13011310 function getSlaveDB() {
13021311 return $this->getMasterDB();
13031312 }
@@ -1323,9 +1332,11 @@
13241333 function store( $srcPath, $dstZone, $dstRel, $flags = 0 ) {
13251334 throw new MWException( get_class($this) . ': write operations are not supported' );
13261335 }
 1336+
13271337 function publish( $srcPath, $dstRel, $archiveRel, $flags = 0 ) {
13281338 throw new MWException( get_class($this) . ': write operations are not supported' );
13291339 }
 1340+
13301341 function deleteBatch( $sourceDestPairs ) {
13311342 throw new MWException( get_class($this) . ': write operations are not supported' );
13321343 }
@@ -1355,13 +1366,23 @@
13561367 $this->hasSharedCache = $info['hasSharedCache'];
13571368 }
13581369
 1370+ /**
 1371+ * @return DatabaseBase
 1372+ */
13591373 function getMasterDB() {
13601374 return wfGetDB( DB_MASTER, array(), $this->wiki );
13611375 }
13621376
 1377+ /**
 1378+ * @return DatabaseBase
 1379+ */
13631380 function getSlaveDB() {
13641381 return wfGetDB( DB_SLAVE, array(), $this->wiki );
13651382 }
 1383+
 1384+ /**
 1385+ * @return bool
 1386+ */
13661387 function hasSharedCache() {
13671388 return $this->hasSharedCache;
13681389 }
@@ -1384,9 +1405,11 @@
13851406 function store( $srcPath, $dstZone, $dstRel, $flags = 0 ) {
13861407 throw new MWException( get_class($this) . ': write operations are not supported' );
13871408 }
 1409+
13881410 function publish( $srcPath, $dstRel, $archiveRel, $flags = 0 ) {
13891411 throw new MWException( get_class($this) . ': write operations are not supported' );
13901412 }
 1413+
13911414 function deleteBatch( $fileMap ) {
13921415 throw new MWException( get_class($this) . ': write operations are not supported' );
13931416 }

Sign-offs

UserFlagDate
RussNelsoninspected22:44, 11 October 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r99590Fixed FileRepo::storeBatch() to check for overwrites, and edited TODO accordi...nelson22:30, 11 October 2011

Comments

#Comment by RussNelson (talk | contribs)   22:43, 11 October 2011

I'm not sure what problem was to be solved by protecting $repo, but it breaks things. Could you perhaps teach me something about when variables should be protected and when not?

#Comment by Reedy (talk | contribs)   22:46, 11 October 2011

Protected is when they only want to allow subclasses to use it

Else public... Use getFoo() style methods

#Comment by RussNelson (talk | contribs)   23:14, 11 October 2011

Hrm. If you protect it all the way down, through LocalFile.php and File.php, everything that phpunit tests still works. I don't know of any reason why it needs to be public. It wasn't *marked* as public in File.php (an abstract class), but neither was it protected.

If you want to go ahead and protect it, then it should be sufficient to mark it as protected in File.php, no?

#Comment by Reedy (talk | contribs)   23:36, 11 October 2011

You also benefit from type hinting in IDE's etc where that's reported....

#Comment by Tim Starling (talk | contribs)   01:04, 12 December 2011

The "protected $repo" was removed in r99590. It's not necessary to declare it here since it is in the grandparent, File.

Status & tagging log