r77083 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r77082‎ | r77083 | r77084 >
Date:19:56, 21 November 2010
Author:maxsem
Status:resolved (Comments)
Tags:
Comment:
Refactoring of *Field classes:
* Made them all implement one common interface (might add more functions to it later)
* Moved MySQLField to DatabaseMysql.php
* Renamed nullable() to isNullable()
* Removed maxLength() from:
** SQLiteField: makes no sense
** MySQLField: doesn't do what people may think, useless for this class' purpose of assisting querying the DB schema
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseIbm_db2.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseMssql.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseMysql.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)
  • /trunk/phase3/includes/db/DatabasePostgres.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseSqlite.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history)
  • /trunk/phase3/includes/installer/PostgresUpdater.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseMysql.php
——@@ -503,6 +503,56 @@
504504 */
505505 class Database extends DatabaseMysql {}
506506
 507+/**
 508+ * Utility class.
 509+ * @ingroup Database
 510+ */
 511+class MySQLField implements Field {
 512+ private $name, $tablename, $default, $max_length, $nullable,
 513+ $is_pk, $is_unique, $is_multiple, $is_key, $type;
 514+
 515+ function __construct ( $info ) {
 516+ $this->name = $info->name;
 517+ $this->tablename = $info->table;
 518+ $this->default = $info->def;
 519+ $this->max_length = $info->max_length;
 520+ $this->nullable = !$info->not_null;
 521+ $this->is_pk = $info->primary_key;
 522+ $this->is_unique = $info->unique_key;
 523+ $this->is_multiple = $info->multiple_key;
 524+ $this->is_key = ( $this->is_pk || $this->is_unique || $this->is_multiple );
 525+ $this->type = $info->type;
 526+ }
 527+
 528+ function name() {
 529+ return $this->name;
 530+ }
 531+
 532+ function tableName() {
 533+ return $this->tableName;
 534+ }
 535+
 536+ function type() {
 537+ return $this->type;
 538+ }
 539+
 540+ function isNullable() {
 541+ return $this->nullable;
 542+ }
 543+
 544+ function defaultValue() {
 545+ return $this->default;
 546+ }
 547+
 548+ function isKey() {
 549+ return $this->is_key;
 550+ }
 551+
 552+ function isMultipleKey() {
 553+ return $this->is_multiple;
 554+ }
 555+}
 556+
507557 class MySQLMasterPos {
508558 var $file, $pos;
509559
Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -124,7 +124,7 @@
125125 * Utility class.
126126 * @ingroup Database
127127 */
128 -class ORAField {
 128+class ORAField implements Field {
129129 private $name, $tablename, $default, $max_length, $nullable,
130130 $is_pk, $is_unique, $is_multiple, $is_key, $type;
131131
——@@ -157,7 +157,7 @@
158158 return $this->max_length;
159159 }
160160
161 - function nullable() {
 161+ function isNullable() {
162162 return $this->nullable;
163163 }
164164
Index: trunk/phase3/includes/db/DatabasePostgres.php
——@@ -6,7 +6,7 @@
77 * @ingroup Database
88 */
99
10 -class PostgresField {
 10+class PostgresField implements Field {
1111 private $name, $tablename, $type, $nullable, $max_length, $deferred, $deferrable, $conname;
1212
1313 static function fromText($db, $table, $field) {
——@@ -69,7 +69,7 @@
7070 return $this->type;
7171 }
7272
73 - function nullable() {
 73+ function isNullable() {
7474 return $this->nullable;
7575 }
7676
Index: trunk/phase3/includes/db/DatabaseIbm_db2.php
——@@ -13,7 +13,7 @@
1414 * This represents a column in a DB2 database
1515 * @ingroup Database
1616 */
17 -class IBM_DB2Field {
 17+class IBM_DB2Field implements Field {
1818 private $name = '';
1919 private $tablename = '';
2020 private $type = '';
——@@ -75,7 +75,7 @@
7676 * Can column be null?
7777 * @return bool true or false
7878 */
79 - function nullable() { return $this->nullable; }
 79+ function isNullable() { return $this->nullable; }
8080 /**
8181 * How much can you fit in the column per row?
8282 * @return int length
Index: trunk/phase3/includes/db/Database.php
——@@ -2646,57 +2646,33 @@
26472647 }
26482648
26492649 /**
2650 - * Utility class.
 2650+ * Base for all database-specific classes representing information about database fields
26512651 * @ingroup Database
26522652 */
2653 -class MySQLField {
2654 - private $name, $tablename, $default, $max_length, $nullable,
2655 - $is_pk, $is_unique, $is_multiple, $is_key, $type;
 2653+interface Field {
 2654+ /**
 2655+ * Field name
 2656+ * @return string
 2657+ */
 2658+ function name();
26562659
2657 - function __construct ( $info ) {
2658 - $this->name = $info->name;
2659 - $this->tablename = $info->table;
2660 - $this->default = $info->def;
2661 - $this->max_length = $info->max_length;
2662 - $this->nullable = !$info->not_null;
2663 - $this->is_pk = $info->primary_key;
2664 - $this->is_unique = $info->unique_key;
2665 - $this->is_multiple = $info->multiple_key;
2666 - $this->is_key = ( $this->is_pk || $this->is_unique || $this->is_multiple );
2667 - $this->type = $info->type;
2668 - }
 2660+ /**
 2661+ * Name of table this field belongs to
 2662+ * @return string
 2663+ */
 2664+ function tableName();
26692665
2670 - function name() {
2671 - return $this->name;
2672 - }
 2666+ /**
 2667+ * Database type
 2668+ * @return string
 2669+ */
 2670+ function type();
26732671
2674 - function tableName() {
2675 - return $this->tableName;
2676 - }
2677 -
2678 - function defaultValue() {
2679 - return $this->default;
2680 - }
2681 -
2682 - function maxLength() {
2683 - return $this->max_length;
2684 - }
2685 -
2686 - function nullable() {
2687 - return $this->nullable;
2688 - }
2689 -
2690 - function isKey() {
2691 - return $this->is_key;
2692 - }
2693 -
2694 - function isMultipleKey() {
2695 - return $this->is_multiple;
2696 - }
2697 -
2698 - function type() {
2699 - return $this->type;
2700 - }
 2672+ /**
 2673+ * Whether this field can store NULL values
 2674+ * @return bool
 2675+ */
 2676+ function isNullable();
27012677 }
27022678
27032679 /******************************************************************************
Index: trunk/phase3/includes/db/DatabaseMssql.php
——@@ -1051,7 +1051,7 @@
10521052 *
10531053 * @ingroup Database
10541054 */
1055 -class MssqlField {
 1055+class MssqlField implements Field {
10561056 private $name, $tablename, $default, $max_length, $nullable, $type;
10571057 function __construct ( $info ) {
10581058 $this->name = $info['COLUMN_NAME'];
——@@ -1077,7 +1077,7 @@
10781078 return $this->max_length;
10791079 }
10801080
1081 - function nullable() {
 1081+ function isNullable() {
10821082 return $this->nullable;
10831083 }
10841084
Index: trunk/phase3/includes/db/DatabaseSqlite.php
——@@ -626,7 +626,7 @@
627627 /**
628628 * @ingroup Database
629629 */
630 -class SQLiteField {
 630+class SQLiteField implements Field {
631631 private $info, $tableName;
632632 function __construct( $info, $tableName ) {
633633 $this->info = $info;
——@@ -651,17 +651,10 @@
652652 return $this->info->dflt_value;
653653 }
654654
655 - function maxLength() {
656 - return -1;
657 - }
658 -
659 - function nullable() {
 655+ function isNullable() {
660656 return !$this->info->notnull;
661657 }
662658
663 - # isKey(), isMultipleKey() not implemented, MySQL-specific concept.
664 - # Suggest removal from base class [TS]
665 -
666659 function type() {
667660 return $this->info->type;
668661 }
Index: trunk/phase3/includes/installer/MysqlUpdater.php
——@@ -601,7 +601,7 @@
602602 */
603603 protected function doWatchlistNull() {
604604 $info = $this->db->fieldInfo( 'watchlist', 'wl_notificationtimestamp' );
605 - if ( $info->nullable() ) {
 605+ if ( $info->isNullable() ) {
606606 $this->output( "...wl_notificationtimestamp is already nullable.\n" );
607607 return;
608608 }
Index: trunk/phase3/includes/installer/PostgresUpdater.php
——@@ -442,7 +442,7 @@
443443 $this->output( "... error: expected column $table.$field to exist\n" );
444444 exit( 1 );
445445 }
446 - if ( $fi->nullable() ) {
 446+ if ( $fi->isNullable() ) {
447447 # # It's NULL - does it need to be NOT NULL?
448448 if ( 'NOT NULL' === $null ) {
449449 $this->output( "Changing \"$table.$field\" to not allow NULLs\n" );
——@@ -616,7 +616,7 @@
617617
618618 protected function checkRcCurIdNullable(){
619619 $fi = $this->db->fieldInfo( 'recentchanges', 'rc_cur_id' );
620 - if ( !$fi->nullable() ) {
 620+ if ( !$fi->isNullable() ) {
621621 $this->output( "Removing NOT NULL constraint from \"recentchanges.rc_cur_id\"\n" );
622622 $this->applyPatch( 'patch-rc_cur_id-not-null.sql' );
623623 } else {
Index: trunk/phase3/includes/AutoLoader.php
——@@ -381,6 +381,7 @@
382382 'DBQueryError' => 'includes/db/Database.php',
383383 'DBUnexpectedError' => 'includes/db/Database.php',
384384 'FakeResultWrapper' => 'includes/db/Database.php',
 385+ 'Field' => 'includes/db/Database.php',
385386 'IBM_DB2Blob' => 'includes/db/DatabaseIbm_db2.php',
386387 'LBFactory' => 'includes/db/LBFactory.php',
387388 'LBFactory_Multi' => 'includes/db/LBFactory_Multi.php',
——@@ -389,7 +390,7 @@
390391 'LoadBalancer' => 'includes/db/LoadBalancer.php',
391392 'LoadMonitor' => 'includes/db/LoadMonitor.php',
392393 'LoadMonitor_MySQL' => 'includes/db/LoadMonitor.php',
393 - 'MySQLField' => 'includes/db/Database.php',
 394+ 'MySQLField' => 'includes/db/DatabaseMysql.php',
394395 'MySQLMasterPos' => 'includes/db/DatabaseMysql.php',
395396 'ORABlob' => 'includes/db/DatabaseOracle.php',
396397 'ORAField' => 'includes/db/DatabaseOracle.php',

Comments

#Comment by Nikerabbit (talk | contribs)   09:09, 22 November 2010

Field is a bit generic, isn't it? How about DatabaseField?

#Comment by MarkAHershberger (talk | contribs)   07:30, 25 November 2010

Perhaps it is just my Postgres version (8.4), but fieldInfo() isn't returning any info about fields, so addPgField() is failing.

#Comment by MaxSem (talk | contribs)   14:44, 25 November 2010 
C:\Projects\MediaWiki\maintenance>php eval.php
> $db = wfGetDB(DB_MASTER);

> echo $db->getServerInfo();
8.4.3
> var_dump($db->fieldInfo('user', 'user_name'));
object(PostgresField)#19 (8) {
  ["name":"PostgresField":private]=>
  string(9) "user_name"
  ["tablename":"PostgresField":private]=>
  string(6) "mwuser"
  ["type":"PostgresField":private]=>
  string(4) "text"
  ["nullable":"PostgresField":private]=>
  bool(false)
  ["max_length":"PostgresField":private]=>
  string(2) "-1"
  ["deferred":"PostgresField":private]=>
  bool(false)
  ["deferrable":"PostgresField":private]=>
  bool(false)
  ["conname":"PostgresField":private]=>
  string(0) ""
}
#Comment by MarkAHershberger (talk | contribs)   01:33, 26 November 2010

Yep, looks like it was a mix of new-installer and schema problem

#Comment by MarkAHershberger (talk | contribs)   22:02, 10 January 2011

Please update RELEASE-NOTES with the changes made re is*Key() -- at least one extension (OpenID) relies on isMultipleKey().

#Comment by MaxSem (talk | contribs)   15:16, 11 January 2011

isMultipleKey()'s behaviour hasn't changed, because other field classes were not inherited from MysqlField and thus those of them that did not have this function before that revision continue not having it and vice versa. OpenID relies on it, but only in MySQL-specific code. As of committing, I'm currently unable to commit and would appreciate if someone did that. The only thing worth mentioning is actually nullable() --> isNullable() change, which nevertheless affects no extension in our repo.

Status & tagging log