r102683 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r102682‎ | r102683 | r102684 >
Date:20:39, 10 November 2011
Author:reedy
Status:ok (Comments)
Tags:
Comment:
Add $fname to tableExists

Pass $fname/__METHOD__ in in upstream callers
Modified paths:
  • /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/installer/DatabaseInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/DatabaseUpdater.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseMysql.php
——@@ -787,7 +787,7 @@
788788 * @return bool|ResultWrapper
789789 */
790790 public function dropTable( $tableName, $fName = 'DatabaseMysql::dropTable' ) {
791 - if( !$this->tableExists( $tableName ) ) {
 791+ if( !$this->tableExists( $tableName, $fName ) ) {
792792 return false;
793793 }
794794 return $this->query( "DROP TABLE IF EXISTS " . $this->tableName( $tableName ), $fName );
Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -856,7 +856,7 @@
857857 /**
858858 * Query whether a given table exists (in the given schema, or the default mw one if not given)
859859 */
860 - function tableExists( $table ) {
 860+ function tableExists( $table, $fname = __METHOD__ ) {
861861 $table = $this->tableName( $table );
862862 $table = $this->addQuotes( strtoupper( $this->removeIdentifierQuotes( $table ) ) );
863863 $owner = $this->addQuotes( strtoupper( $this->mDBname ) );
——@@ -1316,9 +1316,9 @@
13171317 public function getSearchEngine() {
13181318 return 'SearchOracle';
13191319 }
1320 -
 1320+
13211321 public function getInfinity() {
13221322 return '31-12-2030 12:00:00.000000';
13231323 }
1324 -
 1324+
13251325 } // end DatabaseOracle class
Index: trunk/phase3/includes/db/DatabasePostgres.php
——@@ -771,7 +771,7 @@
772772 * For backward compatibility, this function checks both tables and
773773 * views.
774774 */
775 - function tableExists( $table, $schema = false ) {
 775+ function tableExists( $table, $fname = __METHOD__, $schema = false ) {
776776 return $this->relationExists( $table, array( 'r', 'v' ), $schema );
777777 }
778778
Index: trunk/phase3/includes/db/DatabaseIbm_db2.php
——@@ -492,7 +492,7 @@
493493 * Queries whether a given table exists
494494 * @return boolean
495495 */
496 - public function tableExists( $table ) {
 496+ public function tableExists( $table, $fname = __METHOD__ ) {
497497 $schema = $this->mSchema;
498498
499499 $sql = "SELECT COUNT( * ) FROM SYSIBM.SYSTABLES ST WHERE ST.NAME = '" .
Index: trunk/phase3/includes/db/Database.php
——@@ -1528,13 +1528,14 @@
15291529 * Query whether a given table exists
15301530 *
15311531 * @param $table string
 1532+ * @param $fname string
15321533 *
15331534 * @return bool
15341535 */
1535 - function tableExists( $table ) {
 1536+ function tableExists( $table, $fname = __METHOD__ ) {
15361537 $table = $this->tableName( $table );
15371538 $old = $this->ignoreErrors( true );
1538 - $res = $this->query( "SELECT 1 FROM $table LIMIT 1", __METHOD__ );
 1539+ $res = $this->query( "SELECT 1 FROM $table LIMIT 1", $fname );
15391540 $this->ignoreErrors( $old );
15401541
15411542 return (bool)$res;
——@@ -3311,7 +3312,7 @@
33123313 * @since 1.18
33133314 */
33143315 public function dropTable( $tableName, $fName = 'DatabaseBase::dropTable' ) {
3315 - if( !$this->tableExists( $tableName ) ) {
 3316+ if( !$this->tableExists( $tableName, $fName ) ) {
33163317 return false;
33173318 }
33183319 $sql = "DROP TABLE " . $this->tableName( $tableName );
Index: trunk/phase3/includes/db/DatabaseMssql.php
——@@ -631,7 +631,7 @@
632632 return $version;
633633 }
634634
635 - function tableExists ( $table, $schema = false ) {
 635+ function tableExists ( $table, , $fname = __METHOD__, $schema = false ) {
636636 $res = sqlsrv_query( $this->mConn, "SELECT * FROM information_schema.tables
637637 WHERE table_type='BASE TABLE' AND table_name = '$table'" );
638638 if ( $res === false ) {
Index: trunk/phase3/includes/installer/DatabaseUpdater.php
——@@ -315,7 +315,7 @@
316316 * @return boolean
317317 */
318318 protected function canUseNewUpdatelog() {
319 - return $this->db->tableExists( 'updatelog' ) &&
 319+ return $this->db->tableExists( 'updatelog', __METHOD__ ) &&
320320 $this->db->fieldExists( 'updatelog', 'ul_value' );
321321 }
322322
——@@ -399,7 +399,7 @@
400400 * @param $fullpath Boolean Whether to treat $patch path as a relative or not
401401 */
402402 protected function addTable( $name, $patch, $fullpath = false ) {
403 - if ( $this->db->tableExists( $name ) ) {
 403+ if ( $this->db->tableExists( $name, __METHOD__ ) ) {
404404 $this->output( "...$name table already exists.\n" );
405405 } else {
406406 $this->output( "Creating $name table..." );
——@@ -416,7 +416,7 @@
417417 * @param $fullpath Boolean Whether to treat $patch path as a relative or not
418418 */
419419 protected function addField( $table, $field, $patch, $fullpath = false ) {
420 - if ( !$this->db->tableExists( $table ) ) {
 420+ if ( !$this->db->tableExists( $table, __METHOD__ ) ) {
421421 $this->output( "...$table table does not exist, skipping new field patch\n" );
422422 } elseif ( $this->db->fieldExists( $table, $field ) ) {
423423 $this->output( "...have $field field in $table table.\n" );
——@@ -486,7 +486,7 @@
487487 * @param $fullpath bool
488488 */
489489 protected function dropTable( $table, $patch, $fullpath = false ) {
490 - if ( $this->db->tableExists( $table ) ) {
 490+ if ( $this->db->tableExists( $table, __METHOD__ ) ) {
491491 $this->output( "Dropping table $table... " );
492492 $this->applyPatch( $patch, $fullpath );
493493 $this->output( "ok\n" );
——@@ -505,7 +505,7 @@
506506 */
507507 public function modifyField( $table, $field, $patch, $fullpath = false ) {
508508 $updateKey = "$table-$field-$patch";
509 - if ( !$this->db->tableExists( $table ) ) {
 509+ if ( !$this->db->tableExists( $table, __METHOD__ ) ) {
510510 $this->output( "...$table table does not exist, skipping modify field patch\n" );
511511 } elseif ( !$this->db->fieldExists( $table, $field ) ) {
512512 $this->output( "...$field field does not exist in $table table, skipping modify field patch\n" );
Index: trunk/phase3/includes/installer/MysqlUpdater.php
——@@ -243,7 +243,7 @@
244244 protected function doInterwikiUpdate() {
245245 global $IP;
246246
247 - if ( $this->db->tableExists( "interwiki" ) ) {
 247+ if ( $this->db->tableExists( "interwiki", __METHOD__ ) ) {
248248 $this->output( "...already have interwiki table\n" );
249249 return;
250250 }
——@@ -315,7 +315,7 @@
316316 }
317317
318318 function doSchemaRestructuring() {
319 - if ( $this->db->tableExists( 'page' ) ) {
 319+ if ( $this->db->tableExists( 'page', __METHOD__ ) ) {
320320 $this->output( "...page table already exists.\n" );
321321 return;
322322 }
——@@ -506,7 +506,7 @@
507507 }
508508
509509 protected function doPagelinksUpdate() {
510 - if ( $this->db->tableExists( 'pagelinks' ) ) {
 510+ if ( $this->db->tableExists( 'pagelinks', __METHOD__ ) ) {
511511 $this->output( "...already have pagelinks table.\n" );
512512 return;
513513 }
——@@ -555,7 +555,7 @@
556556 }
557557
558558 protected function doUserGroupsUpdate() {
559 - if ( $this->db->tableExists( 'user_groups' ) ) {
 559+ if ( $this->db->tableExists( 'user_groups', __METHOD__ ) ) {
560560 $info = $this->db->fieldInfo( 'user_groups', 'ug_group' );
561561 if ( $info->type() == 'int' ) {
562562 $oldug = $this->db->tableName( 'user_groups' );
——@@ -582,7 +582,7 @@
583583 $this->applyPatch( 'patch-user_groups.sql' );
584584 $this->output( "ok\n" );
585585
586 - if ( !$this->db->tableExists( 'user_rights' ) ) {
 586+ if ( !$this->db->tableExists( 'user_rights', __METHOD__ ) ) {
587587 if ( $this->db->fieldExists( 'user', 'user_rights' ) ) {
588588 $this->output( "Upgrading from a 1.3 or older database? Breaking out user_rights for conversion..." );
589589 $this->db->applyPatch( 'patch-user_rights.sql' );
——@@ -651,7 +651,7 @@
652652 }
653653
654654 protected function doTemplatelinksUpdate() {
655 - if ( $this->db->tableExists( 'templatelinks' ) ) {
 655+ if ( $this->db->tableExists( 'templatelinks', __METHOD__ ) ) {
656656 $this->output( "...templatelinks table already exists\n" );
657657 return;
658658 }
——@@ -709,7 +709,7 @@
710710 * -- Andrew Garrett, January 2007.
711711 */
712712 protected function doRestrictionsUpdate() {
713 - if ( $this->db->tableExists( 'page_restrictions' ) ) {
 713+ if ( $this->db->tableExists( 'page_restrictions', __METHOD__ ) ) {
714714 $this->output( "...page_restrictions table already exists.\n" );
715715 return;
716716 }
——@@ -760,7 +760,7 @@
761761 }
762762
763763 protected function doMaybeProfilingMemoryUpdate() {
764 - if ( !$this->db->tableExists( 'profiling' ) ) {
 764+ if ( !$this->db->tableExists( 'profiling', __METHOD__ ) ) {
765765 // Simply ignore
766766 } elseif ( $this->db->fieldExists( 'profiling', 'pf_memory' ) ) {
767767 $this->output( "...profiling table has pf_memory field.\n" );
Index: trunk/phase3/includes/installer/DatabaseInstaller.php
——@@ -148,7 +148,7 @@
149149 }
150150 $this->db->selectDB( $this->getVar( 'wgDBname' ) );
151151
152 - if( $this->db->tableExists( 'user' ) ) {
 152+ if( $this->db->tableExists( 'user', __METHOD__ ) ) {
153153 $status->warning( 'config-install-tables-exist' );
154154 $this->enableLB();
155155 return $status;
——@@ -466,7 +466,7 @@
467467 if ( !$this->db->selectDB( $this->getVar( 'wgDBname' ) ) ) {
468468 return false;
469469 }
470 - return $this->db->tableExists( 'cur' ) || $this->db->tableExists( 'revision' );
 470+ return $this->db->tableExists( 'cur', __METHOD__ ) || $this->db->tableExists( 'revision', __METHOD__ );
471471 }
472472
473473 /**
Index: trunk/phase3/includes/installer/MysqlInstaller.php
——@@ -164,7 +164,7 @@
165165 $conn->selectDB( $this->getVar( 'wgDBname' ) );
166166
167167 # Determine existing default character set
168 - if ( $conn->tableExists( "revision" ) ) {
 168+ if ( $conn->tableExists( "revision", __METHOD__ ) ) {
169169 $revision = $conn->buildLike( $this->getVar( 'wgDBprefix' ) . 'revision' );
170170 $res = $conn->query( "SHOW TABLE STATUS $revision", __METHOD__ );
171171 $row = $conn->fetchObject( $res );

Sign-offs

UserFlagDate
Hasharinspected21:37, 6 December 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r102686Fix syntax error I introduced from r102683reedy20:49, 10 November 2011

Comments

#Comment by Hashar (talk | contribs)   15:49, 28 November 2011

Looks like it will break MSSQL / Postgre tableExists() calls since you are changing the parameter order: 

-	function tableExists( $table, $schema = false ) {
+	function tableExists( $table, $fname = __METHOD__, $schema = false ) {


Also, instead of passing the calling method as an argument, can't we look up the calling function from the stack table? This way we could get ride of those $fname = __METHOD__ arguments.

#Comment by Reedy (talk | contribs)   15:56, 28 November 2011

Nothing seems to obviously use it

Even in PostgresUpdater 

	protected function convertArchive2() {
		if ( $this->db->tableExists( "archive2" ) ) {

Similar in DatabaseMssql

Looks fine to me

#Comment by 😂 (talk | contribs)   17:22, 6 December 2011

Getting the info from the call stack would be slower than just passing a predefined constant.

#Comment by Hashar (talk | contribs)   21:37, 6 December 2011

Thanks for the answer ^demon :-D

#Comment by Hashar (talk | contribs)   21:38, 6 December 2011

This revision is probably fine. I am not confident enough to validate the PostgreSQL part.

#Comment by Tim Starling (talk | contribs)   02:17, 12 December 2011

That's a nice trick using __METHOD__ as a default value, I didn't know that was possible. I checked it in PHP 5.0.x, it even worked back then.

Status & tagging log