From: Timo Tijhof <krinklemail@gmail.com>
Date: Thu, 19 Apr 2018 23:21:51 +0000 (+0100)
Subject: rdbms: Replace reportConnectionError() with direct throws
X-Git-Tag: 1.34.0-rc.0~5598^2
X-Git-Url: https://git.heureux-cyclage.org/?a=commitdiff_plain;h=c7fc4ef9bf3dc044e8da3497fdab226bc177dd88;p=lhc%2Fweb%2Fwiklou.git

rdbms: Replace reportConnectionError() with direct throws

When reading through DatabaseMysqlBase::open(), it was not
obvious that execution would not continue after the conditional
`!$this->conn` block, given it ends in a method call, without
return or throw. I considered adding a return statement after it
for clarity, but it seems in this case it might make more sense
to throw directly given $error here has already gone through a
fallback to getLastError() a few lines up.

Replace the other three calls to reportConnectionError() as well,
which previously passed a useful string that was overwritten
with lastError(). Instead, log both. And make their call to
queryLogger->error() match the previous ones to have an 'error' as well.

This leaves reportConnectionError() as being unused, except for
a call from LoadBalancer. That call was problematic because
it was inside a conditional for IDatabase, but the method isn't
part of that interface. Replace it with a direct throw as well.

Deprecate the method as its now unused in core, and also remove its
'# New method' comment which hasn't made sense since r75341 (16cded8b32).

Change-Id: I0f2ef00ba44bf7090a3ce54edeb8c7e8e543e46a
---

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 5549f28a80..c6b99efee8 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -991,17 +991,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	abstract protected function closeConnection();
 
 	/**
-	 * @param string $error Fallback error message, used if none is given by DB
+	 * @deprecated since 1.32
+	 * @param string $error Fallback message, if none is given by DB
 	 * @throws DBConnectionError
 	 */
 	public function reportConnectionError( $error = 'Unknown error' ) {
-		$myError = $this->lastError();
-		if ( $myError ) {
-			$error = $myError;
-		}
-
-		# New method
-		throw new DBConnectionError( $this, $error );
+		call_user_func( $this->deprecationLogger, 'Use of ' . __METHOD__ . ' is deprecated.' );
+		throw new DBConnectionError( $this, $this->lastError() ?: $error );
 	}
 
 	/**
diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php
index 3e6190ce9f..047213908e 100644
--- a/includes/libs/rdbms/database/DatabaseMysqlBase.php
+++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php
@@ -152,9 +152,7 @@ abstract class DatabaseMysqlBase extends Database {
 
 		# Always log connection errors
 		if ( !$this->conn ) {
-			if ( !$error ) {
-				$error = $this->lastError();
-			}
+			$error = $error ?: $this->lastError();
 			$this->connLogger->error(
 				"Error connecting to {db_server}: {error}",
 				$this->getLogContext( [
@@ -166,7 +164,7 @@ abstract class DatabaseMysqlBase extends Database {
 				"Server: $server, User: $user, Password: " .
 				substr( $password, 0, 3 ) . "..., error: " . $error . "\n" );
 
-			$this->reportConnectionError( $error );
+			throw new DBConnectionError( $this, $error );
 		}
 
 		if ( strlen( $dbName ) ) {
@@ -174,22 +172,29 @@ abstract class DatabaseMysqlBase extends Database {
 			$success = $this->selectDB( $dbName );
 			Wikimedia\restoreWarnings();
 			if ( !$success ) {
+				$error = $this->lastError();
 				$this->queryLogger->error(
-					"Error selecting database {db_name} on server {db_server}",
+					"Error selecting database {db_name} on server {db_server}: {error}",
 					$this->getLogContext( [
 						'method' => __METHOD__,
+						'error' => $error,
 					] )
 				);
-				$this->queryLogger->debug(
-					"Error selecting database $dbName on server {$this->server}" );
-
-				$this->reportConnectionError( "Error selecting database $dbName" );
+				throw new DBConnectionError( $this, "Error selecting database $dbName: $error" );
 			}
 		}
 
 		// Tell the server what we're communicating with
 		if ( !$this->connectInitCharset() ) {
-			$this->reportConnectionError( "Error setting character set" );
+			$error = $this->lastError();
+			$this->queryLogger->error(
+				"Error setting character set: {error}",
+				$this->getLogContext( [
+					'method' => __METHOD__,
+					'error' => $this->lastError(),
+				] )
+			);
+			throw new DBConnectionError( $this, "Error setting character set: $error" );
 		}
 
 		// Abstract over any insane MySQL defaults
@@ -212,14 +217,15 @@ abstract class DatabaseMysqlBase extends Database {
 			// Use doQuery() to avoid opening implicit transactions (DBO_TRX)
 			$success = $this->doQuery( 'SET ' . implode( ', ', $set ) );
 			if ( !$success ) {
+				$error = $this->lastError();
 				$this->queryLogger->error(
-					'Error setting MySQL variables on server {db_server} (check $wgSQLMode)',
+					'Error setting MySQL variables on server {db_server}: {error}',
 					$this->getLogContext( [
 						'method' => __METHOD__,
+						'error' => $error,
 					] )
 				);
-				$this->reportConnectionError(
-					'Error setting MySQL variables on server {db_server} (check $wgSQLMode)' );
+				throw new DBConnectionError( $this, "Error setting MySQL variables: $error" );
 			}
 		}
 
diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index f0175c934c..8de6064366 100644
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -1130,8 +1130,7 @@ class LoadBalancer implements ILoadBalancer {
 				$context
 			);
 
-			// throws DBConnectionError
-			$conn->reportConnectionError( "{$this->lastError} ({$context['db_server']})" );
+			throw new DBConnectionError( $conn, "{$this->lastError} ({$context['db_server']})" );
 		} else {
 			// No last connection, probably due to all servers being too busy
 			$this->connLogger->error(