Merge "Cleanup ServiceWiring/LBFactoryMW interaction"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Mon, 19 Sep 2016 17:29:53 +0000 (17:29 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Mon, 19 Sep 2016 17:29:53 +0000 (17:29 +0000)
autoload.php
includes/db/loadbalancer/LBFactorySingle.php [deleted file]
includes/exception/MWExceptionRenderer.php
includes/installer/DatabaseInstaller.php
includes/libs/rdbms/database/DatabaseSqlite.php
includes/libs/rdbms/database/resultwrapper/FakeResultWrapper.php
includes/libs/rdbms/database/resultwrapper/MssqlResultWrapper.php
includes/libs/rdbms/database/resultwrapper/ResultWrapper.php
includes/libs/rdbms/lbfactory/LBFactorySingle.php [new file with mode: 0644]
includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php

index 5c132fe..55c42c6 100644 (file)
@@ -661,7 +661,7 @@ $wgAutoloadLocalClasses = [
        'LBFactoryMW' => __DIR__ . '/includes/db/loadbalancer/LBFactoryMW.php',
        'LBFactoryMulti' => __DIR__ . '/includes/libs/rdbms/lbfactory/LBFactoryMulti.php',
        'LBFactorySimple' => __DIR__ . '/includes/libs/rdbms/lbfactory/LBFactorySimple.php',
-       'LBFactorySingle' => __DIR__ . '/includes/db/loadbalancer/LBFactorySingle.php',
+       'LBFactorySingle' => __DIR__ . '/includes/libs/rdbms/lbfactory/LBFactorySingle.php',
        'LCStore' => __DIR__ . '/includes/cache/localisation/LCStore.php',
        'LCStoreCDB' => __DIR__ . '/includes/cache/localisation/LCStoreCDB.php',
        'LCStoreDB' => __DIR__ . '/includes/cache/localisation/LCStoreDB.php',
diff --git a/includes/db/loadbalancer/LBFactorySingle.php b/includes/db/loadbalancer/LBFactorySingle.php
deleted file mode 100644 (file)
index b760723..0000000
+++ /dev/null
@@ -1,86 +0,0 @@
-<?php
-/**
- * Simple generator of database connections that always returns the same object.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
- * http://www.gnu.org/copyleft/gpl.html
- *
- * @file
- * @ingroup Database
- */
-
-/**
- * An LBFactory class that always returns a single database object.
- */
-class LBFactorySingle extends LBFactory {
-       /** @var LoadBalancerSingle */
-       private $lb;
-
-       /**
-        * @param array $conf An associative array with one member:
-        *  - connection: The IDatabase connection object
-        */
-       public function __construct( array $conf ) {
-               parent::__construct( $conf );
-
-               if ( !isset( $conf['connection'] ) ) {
-                       throw new InvalidArgumentException( "Missing 'connection' argument." );
-               }
-
-               $this->lb = new LoadBalancerSingle( array_merge( $this->baseLoadBalancerParams(), $conf ) );
-       }
-
-       /**
-        * @param bool|string $wiki
-        * @return LoadBalancerSingle
-        */
-       public function newMainLB( $wiki = false ) {
-               return $this->lb;
-       }
-
-       /**
-        * @param bool|string $wiki
-        * @return LoadBalancerSingle
-        */
-       public function getMainLB( $wiki = false ) {
-               return $this->lb;
-       }
-
-       /**
-        * @param string $cluster External storage cluster, or false for core
-        * @param bool|string $wiki Wiki ID, or false for the current wiki
-        * @return LoadBalancerSingle
-        */
-       protected function newExternalLB( $cluster, $wiki = false ) {
-               return $this->lb;
-       }
-
-       /**
-        * @param string $cluster External storage cluster, or false for core
-        * @param bool|string $wiki Wiki ID, or false for the current wiki
-        * @return LoadBalancerSingle
-        */
-       public function getExternalLB( $cluster, $wiki = false ) {
-               return $this->lb;
-       }
-
-       /**
-        * @param string|callable $callback
-        * @param array $params
-        */
-       public function forEachLB( $callback, array $params = [] ) {
-               call_user_func_array( $callback, array_merge( [ $this->lb ], $params ) );
-       }
-}
index bb7a01f..6fed7af 100644 (file)
@@ -28,11 +28,11 @@ class MWExceptionRenderer {
        const AS_PRETTY = 2; // show as HTML
 
        /**
-        * @param Exception $e Original exception
+        * @param Exception|Throwable $e Original exception
         * @param integer $mode MWExceptionExposer::AS_* constant
-        * @param Exception|null $eNew New exception from attempting to show the first
+        * @param Exception|Throwable|null $eNew New exception from attempting to show the first
         */
-       public static function output( Exception $e, $mode, Exception $eNew = null ) {
+       public static function output( $e, $mode, $eNew = null ) {
                global $wgMimeType;
 
                if ( $e instanceof DBConnectionError ) {
@@ -88,12 +88,12 @@ class MWExceptionRenderer {
         *
         * Called by MWException for b/c
         *
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @param string $name Class name of the exception
         * @param array $args Arguments to pass to the callback functions
         * @return string|null String to output or null if any hook has been called
         */
-       public static function runHooks( Exception $e, $name, $args = [] ) {
+       public static function runHooks( $e, $name, $args = [] ) {
                global $wgExceptionHooks;
 
                if ( !isset( $wgExceptionHooks ) || !is_array( $wgExceptionHooks ) ) {
@@ -129,10 +129,10 @@ class MWExceptionRenderer {
        }
 
        /**
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @return bool Should the exception use $wgOut to output the error?
         */
-       private static function useOutputPage( Exception $e ) {
+       private static function useOutputPage( $e ) {
                // Can the extension use the Message class/wfMessage to get i18n-ed messages?
                foreach ( $e->getTrace() as $frame ) {
                        if ( isset( $frame['class'] ) && $frame['class'] === 'LocalisationCache' ) {
@@ -150,9 +150,9 @@ class MWExceptionRenderer {
        /**
         * Output the exception report using HTML
         *
-        * @param Exception $e
+        * @param Exception|Throwable $e
         */
-       private static function reportHTML( Exception $e ) {
+       private static function reportHTML( $e ) {
                global $wgOut, $wgSitename;
 
                if ( self::useOutputPage( $e ) ) {
@@ -206,10 +206,10 @@ class MWExceptionRenderer {
         * backtrace to the error, otherwise show a message to ask to set it to true
         * to show that information.
         *
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @return string Html to output
         */
-       public static function getHTML( Exception $e ) {
+       public static function getHTML( $e ) {
                if ( self::showBackTrace( $e ) ) {
                        $html = "<div class=\"errorbox\"><p>" .
                                nl2br( htmlspecialchars( MWExceptionHandler::getLogMessage( $e ) ) ) .
@@ -254,10 +254,10 @@ class MWExceptionRenderer {
        }
 
        /**
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @return string
         */
-       private function getText( Exception $e ) {
+       private function getText( $e ) {
                if ( self::showBackTrace( $e ) ) {
                        return MWExceptionHandler::getLogMessage( $e ) .
                                "\nBacktrace:\n" .
@@ -269,10 +269,10 @@ class MWExceptionRenderer {
        }
 
        /**
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @return bool
         */
-       private static function showBackTrace( Exception $e ) {
+       private static function showBackTrace( $e ) {
                global $wgShowExceptionDetails, $wgShowDBErrorBacktrace;
 
                return (
@@ -324,9 +324,9 @@ class MWExceptionRenderer {
        }
 
        /**
-        * @param Exception $e
+        * @param Exception|Throwable $e
         */
-       private static function reportOutageHTML( Exception $e ) {
+       private static function reportOutageHTML( $e ) {
                global $wgShowDBErrorBacktrace, $wgShowHostnames, $wgShowSQLErrors;
 
                $sorry = htmlspecialchars( self::msg(
index ded2bd8..4f10367 100644 (file)
@@ -334,8 +334,7 @@ abstract class DatabaseInstaller {
 
                $connection = $status->value;
                $services->redefineService( 'DBLoadBalancerFactory', function() use ( $connection ) {
-                       return new LBFactorySingle( [
-                               'connection' => $connection ] );
+                       return LBFactorySingle::newFromConnection( $connection );
                } );
 
        }
index 817f8b4..8881983 100644 (file)
@@ -300,12 +300,12 @@ class DatabaseSqlite extends DatabaseBase {
                $res = $this->mConn->query( $sql );
                if ( $res === false ) {
                        return false;
-               } else {
-                       $r = $res instanceof ResultWrapper ? $res->result : $res;
-                       $this->mAffectedRows = $r->rowCount();
-                       $res = new ResultWrapper( $this, $r->fetchAll() );
                }
 
+               $r = $res instanceof ResultWrapper ? $res->result : $res;
+               $this->mAffectedRows = $r->rowCount();
+               $res = new ResultWrapper( $this, $r->fetchAll() );
+
                return $res;
        }
 
@@ -1053,4 +1053,4 @@ class DatabaseSqlite extends DatabaseBase {
                return 'SQLite ' . (string)$this->mConn->getAttribute( PDO::ATTR_SERVER_VERSION );
        }
 
-} // end DatabaseSqlite class
+}
index 774def8..1a046cf 100644 (file)
@@ -4,35 +4,19 @@
  * doesn't go anywhere near an actual database.
  */
 class FakeResultWrapper extends ResultWrapper {
-       /** @var array */
-       public $result = [];
-
-       /** @var null And it's going to stay that way :D */
-       protected $db = null;
-
-       /** @var int */
-       protected $pos = 0;
-
-       /** @var array|stdClass|bool */
-       protected $currentRow = null;
+       /** @var $result stdClass[] */
 
        /**
-        * @param array $array
+        * @param stdClass[] $rows
         */
-       function __construct( $array ) {
-               $this->result = $array;
+       function __construct( array $rows ) {
+               parent::__construct( null, $rows );
        }
 
-       /**
-        * @return int
-        */
        function numRows() {
                return count( $this->result );
        }
 
-       /**
-        * @return array|bool
-        */
        function fetchRow() {
                if ( $this->pos < count( $this->result ) ) {
                        $this->currentRow = $this->result[$this->pos];
@@ -54,10 +38,6 @@ class FakeResultWrapper extends ResultWrapper {
        function free() {
        }
 
-       /**
-        * Callers want to be able to access fields with $this->fieldName
-        * @return bool|stdClass
-        */
        function fetchObject() {
                $this->fetchRow();
                if ( $this->currentRow ) {
@@ -72,9 +52,6 @@ class FakeResultWrapper extends ResultWrapper {
                $this->currentRow = null;
        }
 
-       /**
-        * @return bool|stdClass
-        */
        function next() {
                return $this->fetchObject();
        }
index cccb8f1..768511b 100644 (file)
@@ -1,5 +1,6 @@
 <?php
 class MssqlResultWrapper extends ResultWrapper {
+       /** @var integer|null */
        private $mSeekTo = null;
 
        /**
index 4843d02..53109c8 100644 (file)
@@ -1,30 +1,43 @@
 <?php
 /**
- * Result wrapper for grabbing data queried by someone else
+ * Result wrapper for grabbing data queried from an IDatabase object
+ *
+ * Note that using the Iterator methods in combination with the non-Iterator
+ * DB result iteration functions may cause rows to be skipped or repeated.
+ *
+ * By default, this will use the iteration methods of the IDatabase handle if provided.
+ * Subclasses can override methods to make it solely work on the result resource instead.
+ * If no database is provided, and the subclass does not override the DB iteration methods,
+ * then a RuntimeException will be thrown when iteration is attempted.
+ *
+ * The result resource field should not be accessed from non-Database related classes.
+ * It is database class specific and is stored here to associate iterators with queries.
+ *
  * @ingroup Database
  */
 class ResultWrapper implements Iterator {
-       /** @var resource */
+       /** @var resource|array|null Optional underlying result handle for subclass usage */
        public $result;
 
-       /** @var IDatabase */
+       /** @var IDatabase|null */
        protected $db;
 
        /** @var int */
        protected $pos = 0;
-
-       /** @var object|null */
+       /** @var stdClass|null */
        protected $currentRow = null;
 
        /**
-        * Create a new result object from a result resource and a Database object
+        * Create a row iterator from a result resource and an optional Database object
+        *
+        * Only Database-related classes should construct ResultWrapper. Other code may
+        * use the FakeResultWrapper subclass for convenience or compatibility shims, however.
         *
-        * @param IDatabase $database
-        * @param resource|ResultWrapper $result
+        * @param IDatabase|null $db Optional database handle
+        * @param ResultWrapper|array|resource $result Optional underlying result handle
         */
-       function __construct( $database, $result ) {
-               $this->db = $database;
-
+       public function __construct( IDatabase $db = null, $result ) {
+               $this->db = $db;
                if ( $result instanceof ResultWrapper ) {
                        $this->result = $result->result;
                } else {
@@ -37,8 +50,8 @@ class ResultWrapper implements Iterator {
         *
         * @return int
         */
-       function numRows() {
-               return $this->db->numRows( $this );
+       public function numRows() {
+               return $this->getDB()->numRows( $this );
        }
 
        /**
@@ -49,8 +62,8 @@ class ResultWrapper implements Iterator {
         * @return stdClass|bool
         * @throws DBUnexpectedError Thrown if the database returns an error
         */
-       function fetchObject() {
-               return $this->db->fetchObject( $this );
+       public function fetchObject() {
+               return $this->getDB()->fetchObject( $this );
        }
 
        /**
@@ -60,38 +73,49 @@ class ResultWrapper implements Iterator {
         * @return array|bool
         * @throws DBUnexpectedError Thrown if the database returns an error
         */
-       function fetchRow() {
-               return $this->db->fetchRow( $this );
+       public function fetchRow() {
+               return $this->getDB()->fetchRow( $this );
        }
 
        /**
-        * Free a result object
+        * Change the position of the cursor in a result object.
+        * See mysql_data_seek()
+        *
+        * @param int $row
         */
-       function free() {
-               $this->db->freeResult( $this );
-               unset( $this->result );
-               unset( $this->db );
+       public function seek( $row ) {
+               $this->getDB()->dataSeek( $this, $row );
        }
 
        /**
-        * Change the position of the cursor in a result object.
-        * See mysql_data_seek()
+        * Free a result object
         *
-        * @param int $row
+        * This either saves memory in PHP (buffered queries) or on the server (unbuffered queries).
+        * In general, queries are not large enough in result sets for this to be worth calling.
         */
-       function seek( $row ) {
-               $this->db->dataSeek( $this, $row );
+       public function free() {
+               if ( $this->db ) {
+                       $this->db->freeResult( $this );
+                       $this->db = null;
+               }
+               $this->result = null;
        }
 
-       /*
-        * ======= Iterator functions =======
-        * Note that using these in combination with the non-iterator functions
-        * above may cause rows to be skipped or repeated.
+       /**
+        * @return IDatabase
+        * @throws RuntimeException
         */
+       private function getDB() {
+               if ( !$this->db ) {
+                       throw new RuntimeException( get_class( $this ) . ' needs a DB handle for iteration.' );
+               }
+
+               return $this->db;
+       }
 
        function rewind() {
                if ( $this->numRows() ) {
-                       $this->db->dataSeek( $this, 0 );
+                       $this->getDB()->dataSeek( $this, 0 );
                }
                $this->pos = 0;
                $this->currentRow = null;
@@ -125,9 +149,6 @@ class ResultWrapper implements Iterator {
                return $this->currentRow;
        }
 
-       /**
-        * @return bool
-        */
        function valid() {
                return $this->current() !== false;
        }
diff --git a/includes/libs/rdbms/lbfactory/LBFactorySingle.php b/includes/libs/rdbms/lbfactory/LBFactorySingle.php
new file mode 100644 (file)
index 0000000..4beb5d8
--- /dev/null
@@ -0,0 +1,96 @@
+<?php
+/**
+ * Simple generator of database connections that always returns the same object.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @ingroup Database
+ */
+
+/**
+ * An LBFactory class that always returns a single database object.
+ */
+class LBFactorySingle extends LBFactory {
+       /** @var LoadBalancerSingle */
+       private $lb;
+
+       /**
+        * @param array $conf An associative array with one member:
+        *  - connection: The IDatabase connection object
+        */
+       public function __construct( array $conf ) {
+               parent::__construct( $conf );
+
+               if ( !isset( $conf['connection'] ) ) {
+                       throw new InvalidArgumentException( "Missing 'connection' argument." );
+               }
+
+               $this->lb = new LoadBalancerSingle( array_merge( $this->baseLoadBalancerParams(), $conf ) );
+       }
+
+       /**
+        * @param IDatabase $db Live connection handle
+        * @param array $params Parameter map to LBFactorySingle::__constructs()
+        * @return LBFactorySingle
+        * @since 1.28
+        */
+       public static function newFromConnection( IDatabase $db, array $params = [] ) {
+               return new static( [ 'connection' => $db ] + $params );
+       }
+
+       /**
+        * @param bool|string $wiki
+        * @return LoadBalancerSingle
+        */
+       public function newMainLB( $wiki = false ) {
+               return $this->lb;
+       }
+
+       /**
+        * @param bool|string $wiki
+        * @return LoadBalancerSingle
+        */
+       public function getMainLB( $wiki = false ) {
+               return $this->lb;
+       }
+
+       /**
+        * @param string $cluster External storage cluster, or false for core
+        * @param bool|string $wiki Wiki ID, or false for the current wiki
+        * @return LoadBalancerSingle
+        */
+       protected function newExternalLB( $cluster, $wiki = false ) {
+               return $this->lb;
+       }
+
+       /**
+        * @param string $cluster External storage cluster, or false for core
+        * @param bool|string $wiki Wiki ID, or false for the current wiki
+        * @return LoadBalancerSingle
+        */
+       public function getExternalLB( $cluster, $wiki = false ) {
+               return $this->lb;
+       }
+
+       /**
+        * @param string|callable $callback
+        * @param array $params
+        */
+       public function forEachLB( $callback, array $params = [] ) {
+               call_user_func_array( $callback, array_merge( [ $this->lb ], $params ) );
+       }
+}
index 943fcf9..9de4850 100644 (file)
@@ -58,6 +58,16 @@ class LoadBalancerSingle extends LoadBalancer {
                }
        }
 
+       /**
+        * @param IDatabase $db Live connection handle
+        * @param array $params Parameter map to LoadBalancerSingle::__constructs()
+        * @return LoadBalancerSingle
+        * @since 1.28
+        */
+       public static function newFromConnection( IDatabase $db, array $params = [] ) {
+               return new static( [ 'connection' => $db ] + $params );
+       }
+
        /**
         *
         * @param string $server