r78450 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78449‎ | r78450 | r78451 >
Date:20:59, 15 December 2010
Author:aaron
Status:resolved (Comments)
Tags:
Comment:
Changed doQuery() -> query()
Modified paths:
  • /trunk/phase3/includes/installer/OracleInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/OracleUpdater.php (modified) (history)
  • /trunk/phase3/includes/installer/PostgresInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/PostgresUpdater.php (modified) (history)
  • /trunk/phase3/includes/search/SearchPostgres.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/search/SearchPostgres.php
——@@ -148,7 +148,7 @@
149149
150150 ## We need a separate query here so gin does not complain about empty searches
151151 $SQL = "SELECT to_tsquery($prefix $searchstring)";
152 - $res = $this->db->doQuery($SQL);
 152+ $res = $this->db->query($SQL);
153153 if (!$res) {
154154 ## TODO: Better output (example to catch: one 'two)
155155 die ("Sorry, that was not a valid search string. Please go back and try again");
——@@ -206,7 +206,7 @@
207207 $SQL = "UPDATE pagecontent SET textvector = NULL WHERE old_id IN ".
208208 "(SELECT rev_text_id FROM revision WHERE rev_page = " . intval( $pageid ) .
209209 " ORDER BY rev_text_id DESC OFFSET 1)";
210 - $this->db->doQuery($SQL);
 210+ $this->db->query($SQL);
211211 return true;
212212 }
213213
Index: trunk/phase3/includes/installer/OracleUpdater.php
——@@ -93,7 +93,7 @@
9494 public function doUpdates( $purge = true ) {
9595 parent::doUpdates();
9696
97 - $this->db->doQuery( 'BEGIN fill_wiki_info; END;' );
 97+ $this->db->query( 'BEGIN fill_wiki_info; END;' );
9898 }
9999
100100 }
Index: trunk/phase3/includes/installer/PostgresUpdater.php
——@@ -523,7 +523,7 @@
524524
525525 $safeuser = $this->db->addQuotes( $wgDBuser );
526526 $SQL = "SELECT array_to_string(useconfig,'*') FROM pg_catalog.pg_user WHERE usename = $safeuser";
527 - $config = pg_fetch_result( $this->db->doQuery( $SQL ), 0, 0 );
 527+ $config = pg_fetch_result( $this->db->query( $SQL ), 0, 0 );
528528 $conf = array();
529529 foreach ( explode( '*', $config ) as $c ) {
530530 list( $x, $y ) = explode( '=', $c );
——@@ -546,8 +546,8 @@
547547 }
548548 $search_path = str_replace( ', ,', ',', $search_path );
549549 if ( array_key_exists( 'search_path', $conf ) === false || $search_path != $conf['search_path'] ) {
550 - $this->db->doQuery( "ALTER USER $wgDBuser SET search_path = $search_path" );
551 - $this->db->doQuery( "SET search_path = $search_path" );
 550+ $this->db->query( "ALTER USER $wgDBuser SET search_path = $search_path" );
 551+ $this->db->query( "SET search_path = $search_path" );
552552 } else {
553553 $path = $conf['search_path'];
554554 $this->output( "... search_path for user \"$wgDBuser\" looks correct ($path)\n" );
——@@ -562,8 +562,8 @@
563563 foreach ( $goodconf as $key => $value ) {
564564 if ( !array_key_exists( $key, $conf ) or $conf[$key] !== $value ) {
565565 $this->output( "Setting $key to '$value' for user \"$wgDBuser\"\n" );
566 - $this->db->doQuery( "ALTER USER $wgDBuser SET $key = '$value'" );
567 - $this->db->doQuery( "SET $key = '$value'" );
 566+ $this->db->query( "ALTER USER $wgDBuser SET $key = '$value'" );
 567+ $this->db->query( "SET $key = '$value'" );
568568 } else {
569569 $this->output( "... default value of \"$key\" is correctly set to \"$value\" for user \"$wgDBuser\"\n" );
570570 }
Index: trunk/phase3/includes/installer/OracleInstaller.php
——@@ -195,7 +195,7 @@
196196 public function createTables() {
197197 $status = parent::createTables();
198198
199 - $this->db->doQuery( 'BEGIN fill_wiki_info; END;' );
 199+ $this->db->query( 'BEGIN fill_wiki_info; END;' );
200200
201201 return $status;
202202 }
Index: trunk/phase3/includes/installer/PostgresInstaller.php
——@@ -129,14 +129,14 @@
130130 $ctest = 'mediawiki_test_table';
131131 $safeschema = $conn->addIdentifierQuotes( $schema );
132132 if ( $conn->tableExists( $ctest, $schema ) ) {
133 - $conn->doQuery( "DROP TABLE $safeschema.$ctest" );
 133+ $conn->query( "DROP TABLE $safeschema.$ctest" );
134134 }
135 - $res = $conn->doQuery( "CREATE TABLE $safeschema.$ctest(a int)" );
 135+ $res = $conn->query( "CREATE TABLE $safeschema.$ctest(a int)" );
136136 if ( !$res ) {
137137 $status->fatal( 'config-install-pg-schema-failed',
138138 $this->getVar( 'wgDBuser'), $schema );
139139 }
140 - $conn->doQuery( "DROP TABLE $safeschema.$ctest" );
 140+ $conn->query( "DROP TABLE $safeschema.$ctest" );
141141
142142 return $status;
143143 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r79056* Followup r78450: just use selectField() instead of pg_fetch_result() with a...demon13:56, 27 December 2010
r801901.17: MFT first batch of installer changes: r78043, r78231, r78259, r78300, r...catrope20:47, 13 January 2011
r80612Partial revert r78450: doQuery() and query() are not the same. You can't just...demon02:32, 20 January 2011

Comments

#Comment by 😂 (talk | contribs)   12:52, 16 December 2010

While we're at it, we should probably pass __METHOD__ to all of these as well.

#Comment by Krinkle (talk | contribs)   14:46, 18 December 2010

There's more "doQuery" functions in the trunk database related files:

See also: Recursive search for "doQuery" in /trunk/phase3 as of revision 78559.

Should those be updated to query() too ?

#Comment by MaxSem (talk | contribs)   16:23, 18 December 2010

Most of these doQuery() call are calling unrelated methods of QueryPage and Pager. However, some DB backends (namely, Oracle, PG and MSSQL) seem to use this function liberally. However, there should be reasons for low-level code to call it, for example when it doesn't want to open a new transaction or when it want to handle errors manually. So all instances of this function being called from Database* classes should be decided upon on a case by case basis.

#Comment by OverlordQ (talk | contribs)   03:25, 27 December 2010

Not ok.

PHP Warning: pg_fetch_result() expects parameter 1 to be resource, object given in /var/www/w/includes/installer/PostgresUpdater.php on line 526

#Comment by Aaron Schulz (talk | contribs)   07:55, 27 December 2010

Is there a reason this can't just use selectField()?

#Comment by 😂 (talk | contribs)   13:09, 5 January 2011

This was resolved in r79056, setting back to new.

#Comment by Reedy (talk | contribs)   22:00, 7 July 2011

bug 29761 Postgres-based searching breaks between 1.13.1 and 1.17.0

These changes look to have been made before 1.18...

But Roan merged it in r80190 from r78450 originally

http://svn.wikimedia.org/viewvc/mediawiki/branches/REL1_17/phase3/includes/search/SearchPostgres.php?view=annotate

Status & tagging log