r92520 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r92519‎ | r92520 | r92521 >
Date:03:44, 19 July 2011
Author:nelson
Status:ok (Comments)
Tags:
Comment:
mark a non-API function private; rename it to reflect what it does; call getPath() rather than accessing object variable directly
Modified paths:
  • /trunk/phase3/includes/filerepo/README (modified) (history)
  • /trunk/phase3/includes/filerepo/UnregisteredLocalFile.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/README
@@ -48,7 +48,7 @@
4949 ForeignDBFile.php extends LocalFile
5050 Image.php extends LocalFile
5151 UnregisteredLocalFile.php extends File.
52 -FileRepo.php defined an abstract class FileRepo.
 52+FileRepo.php defines an abstract class FileRepo.
5353 ForeignAPIRepo.php extends FileRepo
5454 FSRepo extends FileRepo
5555 LocalRepo.php extends FSRepo
@@ -57,3 +57,4 @@
5858 NullRepo extends FileRepo
5959
6060 Russ Nelson, March 2011
 61+
Index: trunk/phase3/includes/filerepo/UnregisteredLocalFile.php
@@ -74,7 +74,7 @@
7575 $this->dims = array();
7676 }
7777
78 - function getPageDimensions( $page = 1 ) {
 78+ private function cachePageDimensions( $page = 1 ) {
7979 if ( !isset( $this->dims[$page] ) ) {
8080 if ( !$this->getHandler() ) {
8181 return false;
@@ -85,19 +85,19 @@
8686 }
8787
8888 function getWidth( $page = 1 ) {
89 - $dim = $this->getPageDimensions( $page );
 89+ $dim = $this->cachePageDimensions( $page );
9090 return $dim['width'];
9191 }
9292
9393 function getHeight( $page = 1 ) {
94 - $dim = $this->getPageDimensions( $page );
 94+ $dim = $this->cachePageDimensions( $page );
9595 return $dim['height'];
9696 }
9797
9898 function getMimeType() {
9999 if ( !isset( $this->mime ) ) {
100100 $magic = MimeMagic::singleton();
101 - $this->mime = $magic->guessMimeType( $this->path );
 101+ $this->mime = $magic->guessMimeType( $this->getPath() );
102102 }
103103 return $this->mime;
104104 }

Comments

#Comment by RussNelson (talk | contribs)   03:46, 19 July 2011

It's possible that the getPath() call is wrong, but I can't see how it could be. $this->path is a cached return value of $this->getPath(), which it returns once set.

#Comment by Brion VIBBER (talk | contribs)   17:02, 19 July 2011

Looks fine, and the media file-related phpunit tests which use UnregisteredLocalFile haven't exploded, so so far so good. :)

Status & tagging log