r61179 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r61178‎ | r61179 | r61180 >
Date:20:42, 17 January 2010
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
Fixed error suppressing suggested in r58559 by Tim. Also fixed some warnings and bugs in Oracle abstraction.
Modified paths:
  • /trunk/phase3/config/Installer.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -297,28 +297,26 @@
298298 $union_unique = ( preg_match( '/\/\* UNION_UNIQUE \*\/ /', $sql ) != 0 );
299299 // EXPLAIN syntax in Oracle is EXPLAIN PLAN FOR and it return nothing
300300 // you have to select data from plan table after explain
301 - $olderr = error_reporting( E_ERROR );
302301 $explain_id = date( 'dmYHis' );
303 - error_reporting( $olderr );
304302
305303 $sql = preg_replace( '/^EXPLAIN /', 'EXPLAIN PLAN SET STATEMENT_ID = \'' . $explain_id . '\' FOR', $sql, 1, $explain_count );
306304
307305
308 - $olderr = error_reporting( E_ERROR );
 306+ wfSuppressWarnings();
309307
310308 if ( ( $this->mLastResult = $stmt = oci_parse( $this->mConn, $sql ) ) === false ) {
311309 $e = oci_error( $this->mConn );
312310 $this->reportQueryError( $e['message'], $e['code'], $sql, __FUNCTION__ );
313311 }
314312
315 - $olderr = error_reporting( E_ERROR );
316313 if ( oci_execute( $stmt, $this->execFlags() ) == false ) {
317314 $e = oci_error( $stmt );
318315 if ( !$this->ignore_DUP_VAL_ON_INDEX || $e['code'] != '1' ) {
319316 $this->reportQueryError( $e['message'], $e['code'], $sql, __FUNCTION__ );
320317 }
321318 }
322 - error_reporting( $olderr );
 319+
 320+ wfRestoreWarnings();
323321
324322 if ( $explain_count > 0 ) {
325323 return $this->doQuery( 'SELECT id, cardinality "ROWS" FROM plan_table WHERE statement_id = \'' . $explain_id . '\'' );
——@@ -511,7 +509,8 @@
512510 }
513511 }
514512
515 - $olderr = error_reporting( E_ERROR );
 513+ wfSuppressWarnings();
 514+
516515 if ( oci_execute( $stmt, OCI_DEFAULT ) === false ) {
517516 $e = oci_error( $stmt );
518517
——@@ -523,8 +522,9 @@
524523 } else {
525524 $this->mAffectedRows = oci_num_rows( $stmt );
526525 }
527 - error_reporting( $olderr );
528526
 527+ wfRestoreWarnings();
 528+
529529 if ( isset( $lob ) ) {
530530 foreach ( $lob as $lob_i => $lob_v ) {
531531 $lob_v->free();
——@@ -584,6 +584,13 @@
585585 }
586586
587587 function tableName( $name ) {
 588+ if (is_array($name)) {
 589+ foreach($name as &$single_name) {
 590+ $single_name = $this->tableName($single_name);
 591+ }
 592+ return $name;
 593+ }
 594+
588595 global $wgSharedDB, $wgSharedPrefix, $wgSharedTables;
589596 /*
590597 Replace reserved words with better ones
——@@ -606,7 +613,6 @@
607614 if ( $name[0] == '"' && substr( $name, - 1, 1 ) == '"' ) {
608615 return $name;
609616 }
610 -
611617 if ( preg_match( '/(^|\s)(DISTINCT|JOIN|ON|AS)(\s|$)/i', $name ) !== 0 ) {
612618 return $name;
613619 }
——@@ -831,25 +837,37 @@
832838 * based on prebuilt table to simulate MySQL field info and keep query speed minimal
833839 */
834840 function fieldExists( $table, $field, $fname = 'DatabaseOracle::fieldExists' ) {
835 - $table = trim( $table, '"' );
836 -
837 - if (isset($this->mFieldInfoCache["$table.$field"])) {
838 - return true;
839 - } elseif ( !isset( $this->fieldInfo_stmt ) ) {
840 - $this->fieldInfo_stmt = oci_parse( $this->mConn, 'SELECT * FROM wiki_field_info_full WHERE table_name = upper(:tab) and column_name = UPPER(:col)' );
 841+ $tableWhere = '';
 842+ if (is_array($table)) {
 843+ $tableWhere = 'IN (';
 844+ foreach($table as &$singleTable) {
 845+ $singleTable = trim( $singleTable, '"' );
 846+ if (isset($this->mFieldInfoCache["$singleTable.$field"])) {
 847+ return $this->mFieldInfoCache["$singleTable.$field"];
 848+ }
 849+ $tableWhere .= '\''.$singleTable.'\',';
 850+ }
 851+ $tableWhere = rtrim($tableWhere, ',').')';
 852+ } else {
 853+ $table = trim( $table, '"' );
 854+ if (isset($this->mFieldInfoCache["$table.$field"])) {
 855+ return $this->mFieldInfoCache["$table.$field"];
 856+ }
 857+ $tableWhere = '= upper(\''.$table.'\')';
841858 }
 859+
 860+ $fieldInfoStmt = oci_parse( $this->mConn, 'SELECT * FROM wiki_field_info_full WHERE table_name '.$tableWhere.' and column_name = UPPER(\''.$field.'\')' );
842861
843 - oci_bind_by_name( $this->fieldInfo_stmt, ':tab', trim( $table, '"' ) );
844 - oci_bind_by_name( $this->fieldInfo_stmt, ':col', $field );
845 -
846 - if ( oci_execute( $this->fieldInfo_stmt, OCI_DEFAULT ) === false ) {
847 - $e = oci_error( $this->fieldInfo_stmt );
 862+ if ( oci_execute( $fieldInfoStmt, OCI_DEFAULT ) === false ) {
 863+ $e = oci_error( $fieldInfoStmt );
848864 $this->reportQueryError( $e['message'], $e['code'], 'fieldInfo QUERY', __METHOD__ );
849865 return false;
850866 }
851 - $res = new ORAResult( $this, $this->fieldInfo_stmt );
 867+ $res = new ORAResult( $this, $fieldInfoStmt );
852868 if ($res->numRows() != 0) {
853 - $this->mFieldInfoCache["$table.$field"] = new ORAField( $res->fetchRow() );
 869+ $fieldInfoTemp = new ORAField( $res->fetchRow() );
 870+ $table = $fieldInfoTemp->tableName();
 871+ $this->mFieldInfoCache["$table.$field"] = $fieldInfoTemp;
854872 return true;
855873 } else {
856874 return false;
——@@ -857,25 +875,37 @@
858876 }
859877
860878 function fieldInfo( $table, $field ) {
861 - $table = trim( $table, '"' );
862 -
863 - if (isset($this->mFieldInfoCache["$table.$field"])) {
864 - return $this->mFieldInfoCache["$table.$field"];
865 - } elseif ( !isset( $this->fieldInfo_stmt ) ) {
866 - $this->fieldInfo_stmt = oci_parse( $this->mConn, 'SELECT * FROM wiki_field_info_full WHERE table_name = upper(:tab) and column_name = UPPER(:col)' );
 879+ $tableWhere = '';
 880+ if (is_array($table)) {
 881+ $tableWhere = 'IN (';
 882+ foreach($table as &$singleTable) {
 883+ $singleTable = trim( $singleTable, '"' );
 884+ if (isset($this->mFieldInfoCache["$singleTable.$field"])) {
 885+ return $this->mFieldInfoCache["$singleTable.$field"];
 886+ }
 887+ $tableWhere .= '\''.$singleTable.'\',';
 888+ }
 889+ $tableWhere = rtrim($tableWhere, ',').')';
 890+ } else {
 891+ $table = trim( $table, '"' );
 892+ if (isset($this->mFieldInfoCache["$table.$field"])) {
 893+ return $this->mFieldInfoCache["$table.$field"];
 894+ }
 895+ $tableWhere = '= upper(\''.$table.'\')';
867896 }
 897+
 898+ $fieldInfoStmt = oci_parse( $this->mConn, 'SELECT * FROM wiki_field_info_full WHERE table_name '.$tableWhere.' and column_name = UPPER(\''.$field.'\')' );
868899
869 - oci_bind_by_name( $this->fieldInfo_stmt, ':tab', $table );
870 - oci_bind_by_name( $this->fieldInfo_stmt, ':col', $field );
871 -
872 - if ( oci_execute( $this->fieldInfo_stmt, OCI_DEFAULT ) === false ) {
873 - $e = oci_error( $this->fieldInfo_stmt );
 900+ if ( oci_execute( $fieldInfoStmt, OCI_DEFAULT ) === false ) {
 901+ $e = oci_error( $fieldInfoStmt );
874902 $this->reportQueryError( $e['message'], $e['code'], 'fieldInfo QUERY', __METHOD__ );
875903 return false;
876904 }
877 - $res = new ORAResult( $this, $this->fieldInfo_stmt );
878 - $this->mFieldInfoCache["$table.$field"] = new ORAField( $res->fetchRow() );
879 - return $this->mFieldInfoCache["$table.$field"];
 905+ $res = new ORAResult( $this, $fieldInfoStmt );
 906+ $fieldInfoTemp = new ORAField( $res->fetchRow() );
 907+ $table = $fieldInfoTemp->tableName();
 908+ $this->mFieldInfoCache["$table.$field"] = $fieldInfoTemp;
 909+ return $fieldInfoTemp;
880910 }
881911
882912 function begin( $fname = '' ) {
——@@ -1015,6 +1045,7 @@
10161046 global $wgLang;
10171047
10181048 $conds2 = array();
 1049+ $conds = ($conds != null && !is_array($conds)) ? array($conds) : $conds;
10191050 foreach ( $conds as $col => $val ) {
10201051 $col_type = $this->fieldInfo( $this->tableName( $table ), $col )->type();
10211052 if ( $col_type == 'CLOB' ) {
——@@ -1085,6 +1116,7 @@
10861117
10871118 if ( $wgLang != null ) {
10881119 $conds2 = array();
 1120+ $conds = ($conds != null && !is_array($conds)) ? array($conds) : $conds;
10891121 foreach ( $conds as $col => $val ) {
10901122 $col_type = $this->fieldInfo( $this->tableName( $table ), $col )->type();
10911123 if ( $col_type == 'CLOB' ) {
Index: trunk/phase3/config/Installer.php
——@@ -1009,9 +1009,9 @@
10101010 } elseif ( $conf->DBtype == 'oracle' ) {
10111011 echo "<li>Attempting to connect to database \"" . htmlspecialchars( $wgDBname ) ."\"</li>";
10121012 $old_error_level = error_reporting();
1013 - error_reporting($old_error_level & ~E_WARNING); //disable E_WARNING for test connect (oci returns login denied as warning)
 1013+ wfSuppressWarnings();
10141014 $wgDatabase = $dbc->newFromParams('DUMMY', $wgDBuser, $wgDBpassword, $wgDBname, 1);
1015 - error_reporting($old_error_level);
 1015+ wfRestoreWarnings();
10161016 if (!$wgDatabase->isOpen()) {
10171017 $ok = true;
10181018 echo "<li>Connect failed.</li>";

Follow-up revisions

RevisionCommit summaryAuthorDate
r61636Fixed as per Tim's comments on r61179:...freakolowsky14:58, 28 January 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r58559Fixed some Oracle-specific installer and Strict Standards issuesfreakolowsky19:09, 4 November 2009

Comments

#Comment by Tim Starling (talk | contribs)   05:11, 28 January 2010

Why don't fieldExists() and fieldInfo() use addQuotes() or makeList() like normal DB code? The way you do it here, with no escaping, is scary-looking.

I'm not sure why you have an array form for fieldInfo() anyway, it's not documented in the parent and the other DBMSes don't support such an interface. The implementation here is clearly broken:

  • The cache hit case returns existence on a single random table instead of all of them
  • The result array processing is apparently missing, a single random table will be cached and returned instead.

The undocumented array form of tableName() introduced here seems to be unnecessary. We already have a tableNames() and tableNamesN() which you can call with call_user_func_array() if you wish.


#Comment by Tim Starling (talk | contribs)   05:13, 28 January 2010

Another option is array_map( array( $this, 'tableName' ), $tables ).

#Comment by Freakolowsky (talk | contribs)   15:03, 28 January 2010

Addressed in r61636

#Comment by Freakolowsky (talk | contribs)   11:43, 14 February 2010

I'm unable to change status ... this should have ok or resolved status

Status & tagging log