r80519 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r80518‎ | r80519 | r80520 >
Date:21:32, 18 January 2011
Author:btongminh
Status:deferred
Tags:license-work 
Comment:
Rewrite the FileProperties backend a bit. Now both FileAuthor and FileLicense subclass a common FileProperty, and FileProperties is now property type agnostic, which should make extending easier. The class interfaces should be fairly stable from now on, but I reserve the right to make breaking changes any of those classes.
Modified paths:
  • /branches/license-work/phase3/includes/ImagePage.php (modified) (history)
  • /branches/license-work/phase3/includes/filerepo/FileProperties.php (modified) (history)

Diff [purge]

Index: branches/license-work/phase3/includes/ImagePage.php
——@@ -596,8 +596,8 @@
597597 */
598598 protected function showFileProperties() {
599599 $props = $this->img->properties();
600 - $authors = $props->getAuthors();
601 - $licenses = $props->getLicenses();
 600+ $authors = $props->get( 'author' );
 601+ $licenses = $props->get( 'license' );
602602 if ( $authors || $licenses ) {
603603 global $wgOut;
604604
Index: branches/license-work/phase3/includes/filerepo/FileProperties.php
——@@ -1,6 +1,11 @@
22 <?php
33
44 class FileProperties {
 5+ public static $propertyClasses = array(
 6+ 'license' => 'FileLicense',
 7+ 'author' => 'FileAuthor',
 8+ );
 9+
510 /**
611 * Constructor for the FileProperties class that represents proeprties
712 * associated with a file.
——@@ -12,8 +17,8 @@
1318 $this->file = $file;
1419 $this->revId = $revision;
1520
16 - $this->authors = array();
17 - $this->licenses = array();
 21+ $this->props = array();
 22+ $this->propsByType = array();
1823
1924 $this->load();
2025 }
——@@ -23,6 +28,7 @@
2429 */
2530 public function load() {
2631 if ( !$this->revId ) {
 32+ // Load from the latest revision by default
2733 $this->revId = $this->file->getTitle()->getLatestRevID();
2834 }
2935
——@@ -38,40 +44,72 @@
3945 * Construct FileAuthor and FileProperties objects from a database result
4046 */
4147 protected function loadFromResult( $res ) {
42 - $repo = $this->file->getRepo();
43 - $result = array();
44 -
4548 foreach ( $res as $row ) {
46 - switch ( $row->fp_key ) {
47 - case 'author':
48 - $this->authors[] = FileAuthor::newFromRow( $row );
49 - break;
50 - case 'license':
51 - $this->licenses[] = FileLicense::newFromRow( $repo, $row );
52 - break;
 49+ if ( isset( self::$propertyClasses[$row->fp_key] ) ) {
 50+ // Call the constructor
 51+ $prop = call_user_func( self::$propertyClasses[$row->fp_key] .
 52+ '::newFromRow', $this->file, $row );
 53+
 54+ $this->props[] = $prop;
 55+ if ( !isset( $this->propsByType[$row->fp_key] ) ) {
 56+ $this->propsByType[$row->fp_key] = array();
 57+ }
 58+ $this->propsByType[$row->fp_key][] = $prop;
5359 }
5460 }
5561
56 - if ( $this->licenses ) {
57 - FileLicense::loadArray( $this->licenses );
 62+ foreach ( self::$propertyClasses as $key => $class ) {
 63+ // Batch load properties
 64+ if ( !empty( $this->propsByType[$key] ) ) {
 65+ call_user_func( "$class::loadArray", $this->propsByType[$key] );
 66+ }
5867 }
5968 }
6069
6170 /**
62 - * Get all licenses associated with this file
 71+ * Get all properties of $type, or all if null or omitted
 72+ *
 73+ * @param $type mixed
 74+ * @return array
6375 */
64 - public function getLicenses() {
65 - return $this->licenses;
 76+ public function get( $type = null ) {
 77+ if ( is_null( $type ) ) {
 78+ return $this->props;
 79+ }
 80+ return isset( $this->propsByType[$type] ) ?
 81+ $this->propsByType[$type] : array();
6682 }
 83+
6784 /**
68 - * Get all authors associated with this file
 85+ * Deleted all properties and sets them to the passed array
 86+ *
 87+ * @param $props array
6988 */
70 - public function getAuthors() {
71 - return $this->authors;
 89+ public function set( $props ) {
 90+ $this->props = array();
 91+ $this->propsByType = array();
 92+
 93+ $this->append( $props );
7294 }
 95+ /**
 96+ * Appends an array of properties to the current properties
 97+ *
 98+ * @param $props array
 99+ */
 100+ public function append( $props ) {
 101+ foreach ( $props as $prop ) {
 102+ $this->props[] = $prop;
 103+
 104+ $key = $prop->getKey();
 105+ if ( !isset( $this->propsByType[$key] ) ) {
 106+ $this->propsByType = array();
 107+ }
 108+ $this->propsByType[$key] = $prop;
 109+ }
 110+ }
73111
74112 /**
75 - * Save the licenses and authors to the database and create a new revision
 113+ * Save the properties to the database and create a new revision
76114 *
77115 * @param $comment string Edit summary
78116 * @param $minor bool Minor edit
——@@ -97,42 +135,82 @@
98136 $dbw = $this->file->getRepo()->getMasterDB();
99137
100138 $insert = array();
101 - foreach ( $this->authors as $author ) {
102 - $a = array(
 139+ foreach ( $this->props as $prop ) {
 140+ $insert[] = array(
103141 'fp_rev_id' => $revId,
104 - 'fp_key' => 'author',
105 - 'fp_value_int' => $author->getId()
 142+ 'fp_key' => $prop->getKey(),
 143+ 'fp_value_int' => $prop->getIntValue(),
 144+ 'fp_value_string' => $prop->getStringValue(),
106145 );
107 -
108 - $text = $author->getText();
109 - if ( $text ) {
110 - $a['fp_value_text'] = $text;
111 - }
112 - $insert[] = $a;
113146 }
114 - foreach ( $this->licenses as $license ) {
115 - $insert[] = array(
116 - 'fp_rev_id' => $revId,
117 - 'fp_key' => 'license',
118 - 'fp_value_int' => $license->getId(),
119 - );
120 - }
121147
122 -
123148 $dbw->insert( 'file_props', $insert, __METHOD__ );
124149
125150 $this->revId = $revId;
126151 }
127152 }
128153
129 -class FileAuthor {
 154+/**
 155+ * Base class for file properties
 156+ */
 157+abstract class FileProperty {
130158 /**
 159+ * Create a new property object from a row
 160+ *
 161+ * @param $file File
 162+ * @param $row Stdclass
 163+ */
 164+ public static function newFromRow( $file, $row ) {
 165+ throw new MWException( __CLASS__ . ' does not implement newFromRow' );
 166+ }
 167+ /**
 168+ * Load any other information for an array of properties
 169+ *
 170+ * @param &$array array
 171+ */
 172+ public static function loadArray( &$array ) {}
 173+ /**
 174+ * Get the key name
 175+ *
 176+ * @return string
 177+ */
 178+ abstract public function getKey();
 179+ /**
 180+ * Get the string value; null by default
 181+ *
 182+ * @return string|null
 183+ */
 184+ public function getStringValue() {
 185+ return null;
 186+ }
 187+ /**
 188+ * Get the int value; null by default
 189+ *
 190+ * @return int|null
 191+ */
 192+ public function getIntValue() {
 193+ return null;
 194+ }
 195+ /**
 196+ * Sets the value
 197+ *
 198+ * @param $value int|string
 199+ */
 200+ abstract public function setValue( $value );
 201+}
 202+
 203+/**
 204+ * FileAuthor class that represents the author of a file; possibly a wiki user
 205+ */
 206+class FileAuthor extends FileProperty {
 207+ /**
131208 * Construct a FileAuthor object from a database row
132209 *
 210+ * @param $file File
133211 * @param $row Stdclass
134212 * @return FileAuthor
135213 */
136 - public static function newFromRow( $row ) {
 214+ public static function newFromRow( $file, $row ) {
137215 return new self( $row->fp_value_int, $row->fp_value_text );
138216 }
139217 /**
——@@ -150,7 +228,7 @@
151229 *
152230 * @return int
153231 */
154 - public function getUserId() {
 232+ public function getIntValue() {
155233 return $this->id;
156234 }
157235 /**
——@@ -158,21 +236,43 @@
159237 *
160238 * @return string
161239 */
162 - public function getText() {
 240+ public function getStringValue() {
163241 return $this->text;
164242 }
 243+
 244+ public function getKey() {
 245+ return 'author';
 246+ }
 247+
 248+ /**
 249+ * Sets the name or user id of the author. Changing either the id or the name
 250+ * does not change the other property
 251+ *
 252+ * @param $value string
 253+ */
 254+ public function setValue( $value ) {
 255+ if ( is_string( $value ) ) {
 256+ $this->name = $value;
 257+ } elseif ( is_int( $value ) ) {
 258+ $this->id = $value;
 259+ }
 260+ }
165261 }
166262
167 -class FileLicense {
 263+/**
 264+ * FileLicense class that represents the license of a file and provides access
 265+ * to the extended license attributes
 266+ */
 267+class FileLicense extends FileProperty {
168268 /**
169269 * Initialize a license object from a row
170270 *
171 - * @param $repo FileRepo
 271+ * @param $file File
172272 * @param $row Stdclass
173273 * @return FileLicense
174274 */
175 - public static function newFromRow( $repo, $row ) {
176 - $license = new self( $repo );
 275+ public static function newFromRow( $file, $row ) {
 276+ $license = new self( $file->getRepo() );
177277 $license->id = $row->fp_value_int;
178278 return $license;
179279 }
——@@ -200,15 +300,45 @@
201301 $this->name = null;
202302 $this->url = null;
203303 $this->count = 0;
 304+ $this->exists = null;
204305 }
205 - public function getId() {
 306+ public function getKey() {
 307+ return 'license';
 308+ }
 309+ /**
 310+ * Tries to load the license id and returns it
 311+ *
 312+ * @return int|null
 313+ */
 314+ public function getIntValue() {
 315+ if ( is_null( $this->id ) ) {
 316+ $this->load();
 317+ }
206318 return $this->id;
207319 }
208 - public function getName() {
 320+ /**
 321+ * Tries to load the license name and returns it
 322+ *
 323+ * @return string|null
 324+ */
 325+ public function getStringValue() {
 326+ if ( is_null( $this->name ) ) {
 327+ $this->load();
 328+ }
209329 return $this->name;
210330 }
211 - public function setName( $name ) {
212 - $this->name = $name;
 331+ /**
 332+ * Sets the license name or id and sets the other property to null
 333+ */
 334+ public function setValue( $value ) {
 335+ $this->exists = null;
 336+ if ( is_string( $value ) ) {
 337+ $this->id = null;
 338+ $this->name = $value;
 339+ } elseif ( is_int( $value ) ) {
 340+ $this->id = $value;
 341+ $this->name = null;
 342+ }
213343 }
214344 public function getUrl() {
215345 return $this->url;
——@@ -219,7 +349,36 @@
220350 public function getCount() {
221351 return $this->count;
222352 }
 353+ public function incrementCount( $inc ) {
 354+ $this->count += $inc;
 355+ }
 356+ public function exists() {
 357+ return $this->exists;
 358+ }
223359
 360+ public function load() {
 361+ if ( !is_null( $this->exists ) ) {
 362+ // Already loaded
 363+ return;
 364+ }
 365+
 366+ $a = array( $this );
 367+ if ( !is_null( $this->id ) ) {
 368+ $from = 'id';
 369+ } elseif ( !is_null( $this->name ) ) {
 370+ $from = 'name';
 371+ } else {
 372+ throw new MWException( 'Trying to load bogus license' );
 373+ }
 374+ self::loadArrayFrom( $a, $from );
 375+
 376+ $this->exists = (bool)count( $a );
 377+ }
 378+
 379+ public static function loadArray( &$licenses ) {
 380+ self::loadArrayFrom( $licenses, 'id' );
 381+ }
 382+
224383 /**
225384 * Initializes an uninitialized array of FileLicense objects from the db.
226385 * Removes non-existent licenses from the array. Allows loading from id or
——@@ -228,15 +387,15 @@
229388 * @param &$licenses array Array of FileLicense objects
230389 * @param $loadFrom string id|name depending on from which field to load
231390 */
232 - public static function loadArray( &$licenses, $loadFrom = 'id' ) {
 391+ public static function loadArrayFrom( &$licenses, $loadFrom ) {
233392 $licenseIds = array();
234393 foreach ( $licenses as $license ) {
235394 switch ( $loadFrom ) {
236395 case 'id':
237 - $licenseIds[] = $license->getId();
 396+ $licenseIds[] = $license->getIntValue();
238397 break;
239398 case 'name':
240 - $licenseIds[] = $license->getName();
 399+ $licenseIds[] = $license->getStringValue();
241400 break;
242401 }
243402
——@@ -276,6 +435,7 @@
277436 $this->name = $row->lic_name;
278437 $this->url = $row->lic_url;
279438 $this->count = $row->lic_count;
 439+ $this->exists = true;
280440 }
281441
282442 /**
——@@ -284,7 +444,7 @@
285445 public function insert() {
286446 $dbw = $this->repo->getMasterDb();
287447 $dbw->insert( 'licenses', array(
288 - 'lic_name' => $this->getName(),
 448+ 'lic_name' => $this->getStringValue(),
289449 'lic_url' => $this->getUrl(),
290450 'lic_count' => 0 ),
291451 __METHOD__ );
——@@ -296,15 +456,17 @@
297457 public function update() {
298458 $dbw = $this->repo->getMasterDb();
299459 $dbw->update( 'licenses', array(
300 - 'lic_name' => $this->getName(),
 460+ 'lic_name' => $this->getStringValue(),
301461 'lic_url' => $this->getUrl(),
302 - 'lic_count' => $this->count + 1 ),
 462+ 'lic_count' => $this->count ),
303463 array( 'lic_id' => $this->id ),
304464 __METHOD__ );
305465 }
306466 /**
307467 * Inserts a new license into the database if this license does not have
308 - * an id, updates it otherwise
 468+ * an id, updates it otherwise. This updates the properties of this
 469+ * license, as opposed to changing the license for this file. If you want
 470+ * to change the license of this file, you need to use FileProperties::save
309471 */
310472 public function save() {
311473 if ( $this->id ) {

Status & tagging log