r75343 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r75342‎ \| r75343 \| r75344 >
Date:	21:27, 24 October 2010
Author:	reedy
Status:	resolved (Comments)
Tags:
Comment:	(bug 24853) Kill failFunction - Fixed! :D
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/DatabaseOracle.php (modified) (history) /trunk/phase3/includes/db/DatabasePostgres.php (modified) (history) /trunk/phase3/includes/db/DatabaseSqlite.php (modified) (history) /trunk/phase3/includes/db/LoadBalancer.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseOracle.php
—	—	@@ -185,10 +185,10 @@
186	186	var $mFieldInfoCache = array();
187	187
188	188	function __construct( $server = false, $user = false, $password = false, $dbName = false,
189		~~- $failFunction = false, $flags = 0, $tablePrefix = 'get from global' )~~
	189	+ $flags = 0, $tablePrefix = 'get from global' )
190	190	{
191	191	$tablePrefix = $tablePrefix == 'get from global' ? $tablePrefix : strtoupper( $tablePrefix );
192		~~- parent::__construct( $server, $user, $password, $dbName, $failFunction, $flags, $tablePrefix );~~
	192	+ parent::__construct( $server, $user, $password, $dbName, $flags, $tablePrefix );
193	193	wfRunHooks( 'DatabaseOraclePostInit', array( &$this ) );
194	194	}
195	195
—	—	@@ -218,14 +218,12 @@
219	219	return true;
220	220	}
221	221
222		~~- static function newFromParams( $server, $user, $password, $dbName, $failFunction = false, $flags = 0, $tablePrefix )~~
223		~~- {~~
224		~~- return new DatabaseOracle( $server, $user, $password, $dbName, $failFunction, $flags, $tablePrefix );~~
	222	+ static function newFromParams( $server, $user, $password, $dbName, $flags = 0, $tablePrefix ) {
	223	+ return new DatabaseOracle( $server, $user, $password, $dbName, $flags, $tablePrefix );
225	224	}
226	225
227	226	/**
228	227	* Usually aborts on failure
229		~~- * If the failFunction is set to a non-zero integer, returns success~~
230	228	*/
231	229	function open( $server, $user, $password, $dbName ) {
232	230	if ( !function_exists( 'oci_connect' ) ) {
Index: trunk/phase3/includes/db/DatabasePostgres.php
—	—	@@ -101,7 +101,7 @@
102	102	var $mAffectedRows = null;
103	103
104	104	function __construct( $server = false, $user = false, $password = false, $dbName = false,
105		~~- $failFunction = false, $flags = 0 )~~
	105	+ $flags = 0 )
106	106	{
107	107	$this->mFlags = $flags;
108	108	$this->open( $server, $user, $password, $dbName );
—	—	@@ -144,13 +144,12 @@
145	145	return $this->numRows( $res );
146	146	}
147	147
148		~~- static function newFromParams( $server, $user, $password, $dbName, $failFunction = false, $flags = 0 ) {~~
149		~~- return new DatabasePostgres( $server, $user, $password, $dbName, $failFunction, $flags );~~
	148	+ static function newFromParams( $server, $user, $password, $dbName, $flags = 0 ) {
	149	+ return new DatabasePostgres( $server, $user, $password, $dbName, $flags );
150	150	}
151	151
152	152	/**
153	153	* Usually aborts on failure
154		~~- * If the failFunction is set to a non-zero integer, returns success~~
155	154	*/
156	155	function open( $server, $user, $password, $dbName ) {
157	156	# Test for Postgres support, to avoid suppressed fatal error
Index: trunk/phase3/includes/db/DatabaseIbm_db2.php
—	—	@@ -116,7 +116,6 @@
117	117	protected $mServer, $mUser, $mPassword, $mConn = null, $mDBname;
118	118	protected $mOut, $mOpened = false;
119	119
120		~~- protected $mFailFunction;~~
121	120	protected $mTablePrefix;
122	121	protected $mFlags;
123	122	protected $mTrxLevel = 0;
—	—	@@ -257,13 +256,12 @@
258	257	* @param $user String: username
259	258	* @param $password String: password
260	259	* @param $dbName String: database name on the server
261		~~- * @param $failFunction Callback (optional)~~
262	260	* @param $flags Integer: database behaviour flags (optional, unused)
263	261	* @param $schema String
264	262	*/
265	263	public function DatabaseIbm_db2( $server = false, $user = false,
266	264	$password = false,
267		~~- $dbName = false, $failFunction = false, $flags = 0,~~
	265	+ $dbName = false, $flags = 0,
268	266	$schema = self::USE_GLOBAL )
269	267	{
270	268
—	—	@@ -437,15 +435,14 @@
438	436	* @param $user String: username
439	437	* @param $password String
440	438	* @param $dbName String: database name on the server
441		~~- * @param $failFunction Callback (optional)~~
442	439	* @param $flags Integer: database behaviour flags (optional, unused)
443	440	* @return DatabaseIbm_db2 object
444	441	*/
445	442	static function newFromParams( $server, $user, $password, $dbName,
446		~~- $failFunction = false, $flags = 0 )~~
	443	+ $flags = 0 )
447	444	{
448	445	return new DatabaseIbm_db2( $server, $user, $password, $dbName,
449		~~- $failFunction, $flags );~~
	446	+ $flags );
450	447	}
451	448
452	449	/**
Index: trunk/phase3/includes/db/Database.php
—	—	@@ -32,7 +32,6 @@
33	33
34	34	/**
35	35	* Open a connection to the database. Usually aborts on failure
36		~~- * If the failFunction is set to a non-zero integer, returns success~~
37	36	*
38	37	* @param $server String: database server host
39	38	* @param $user String: database user name
—	—	@@ -210,7 +209,6 @@
211	210	protected $mServer, $mUser, $mPassword, $mConn = null, $mDBname;
212	211	protected $mOpened = false;
213	212
214		~~- protected $mFailFunction;~~
215	213	protected $mTablePrefix;
216	214	protected $mFlags;
217	215	protected $mTrxLevel = 0;
—	—	@@ -479,12 +477,11 @@
480	478	* @param $user String: database user name
481	479	* @param $password String: database user password
482	480	* @param $dbName String: database name
483		~~- * @param $failFunction~~
484	481	* @param $flags
485	482	* @param $tablePrefix String: database table prefixes. By default use the prefix gave in LocalSettings.php
486	483	*/
487	484	function __construct( $server = false, $user = false, $password = false, $dbName = false,
488		~~- $failFunction = false, $flags = 0, $tablePrefix = 'get from global'~~
	485	+ $flags = 0, $tablePrefix = 'get from global'
489	486	) {
490	487	global $wgOut, $wgDBprefix, $wgCommandLineMode;
491	488
—	—	@@ -527,12 +524,11 @@
528	525	* @param $user String: database user name
529	526	* @param $password String: database user password
530	527	* @param $dbName String: database name
531		~~- * @param failFunction~~
532	528	* @param $flags
533	529	*/
534		~~- static function newFromParams( $server, $user, $password, $dbName, $failFunction = false, $flags = 0 ) {~~
	530	+ static function newFromParams( $server, $user, $password, $dbName, $flags = 0 ) {
535	531	wfDeprecated( __METHOD__ );
536		~~- return new DatabaseMysql( $server, $user, $password, $dbName, $failFunction, $flags );~~
	532	+ return new DatabaseMysql( $server, $user, $password, $dbName, $flags );
537	533	}
538	534
539	535	protected function installErrorHandler() {
Index: trunk/phase3/includes/db/DatabaseMssql.php
—	—	@@ -18,7 +18,7 @@
19	19	var $mAffectedRows = NULL;
20	20
21	21	function __construct( $server = false, $user = false, $password = false, $dbName = false,
22		~~- $failFunction = false, $flags = 0 )~~
	22	+ $flags = 0 )
23	23	{
24	24	$this->mFlags = $flags;
25	25	$this->open( $server, $user, $password, $dbName );
—	—	@@ -49,14 +49,12 @@
50	50	return false;
51	51	}
52	52
53		~~- static function newFromParams( $server, $user, $password, $dbName, $failFunction = false, $flags = 0 )~~
54		~~- {~~
55		~~- return new DatabaseMssql( $server, $user, $password, $dbName, $failFunction, $flags );~~
	53	+ static function newFromParams( $server, $user, $password, $dbName, $flags = 0 ) {
	54	+ return new DatabaseMssql( $server, $user, $password, $dbName, $flags );
56	55	}
57	56
58	57	/**
59	58	* Usually aborts on failure
60		~~- * If the failFunction is set to a non-zero integer, returns success~~
61	59	*/
62	60	function open( $server, $user, $password, $dbName ) {
63	61	# Test for driver support, to avoid suppressed fatal error
Index: trunk/phase3/includes/db/DatabaseSqlite.php
—	—	@@ -23,7 +23,7 @@
24	24	* Constructor.
25	25	* Parameters $server, $user and $password are not used.
26	26	*/
27		~~- function __construct( $server = false, $user = false, $password = false, $dbName = false, $failFunction = false, $flags = 0 ) {~~
	27	+ function __construct( $server = false, $user = false, $password = false, $dbName = false, $flags = 0 ) {
28	28	global $wgSharedDB;
29	29	$this->mFlags = $flags;
30	30	$this->mName = $dbName;
—	—	@@ -42,8 +42,8 @@
43	43	*/
44	44	function implicitGroupby() { return false; }
45	45
46		~~- static function newFromParams( $server, $user, $password, $dbName, $failFunction = false, $flags = 0 ) {~~
47		~~- return new DatabaseSqlite( $server, $user, $password, $dbName, $failFunction, $flags );~~
	46	+ static function newFromParams( $server, $user, $password, $dbName, $flags = 0 ) {
	47	+ return new DatabaseSqlite( $server, $user, $password, $dbName, $flags );
48	48	}
49	49
50	50	/** Open an SQLite database and return a resource handle to it
Index: trunk/phase3/includes/db/LoadBalancer.php
—	—	@@ -14,7 +14,7 @@
15	15	*/
16	16	class LoadBalancer {
17	17	/* private */ var $mServers, $mConns, $mLoads, $mGroupLoads;
18		~~- /* private */ var $mFailFunction, $mErrorConnection;~~
	18	+ /* private */ var $mErrorConnection;
19	19	/* private */ var $mReadIndex, $mAllowLagged;
20	20	/* private */ var $mWaitForPos, $mWaitTimeout;
21	21	/* private */ var $mLaggedSlaveMode, $mLastError = 'Unknown error';
—	—	@@ -24,7 +24,6 @@
25	25	/**
26	26	* @param $params Array with keys:
27	27	* servers Required. Array of server info structures.
28		~~- * failFunction Deprecated, use exceptions instead.~~
29	28	* masterWaitTimeout Replication lag wait timeout
30	29	* loadMonitor Name of a class used to fetch server lag and load.
31	30	*/
—	—	@@ -68,9 +67,8 @@
69	68	}
70	69	}
71	70
72		~~- static function newFromParams( $servers, $failFunction = false, $waitTimeout = 10 )~~
73		~~- {~~
74		~~- return new LoadBalancer( $servers, $failFunction, $waitTimeout );~~
	71	+ static function newFromParams( $servers, $waitTimeout = 10 ) {
	72	+ return new LoadBalancer( $servers, $waitTimeout );
75	73	}
76	74
77	75	/**
—	—	@@ -643,7 +641,7 @@
644	642
645	643	# Create object
646	644	wfDebug( "Connecting to $host $dbname...\n" );
647		~~- $db = new $class( $host, $user, $password, $dbname, 1, $flags );~~
	645	+ $db = new $class( $host, $user, $password, $dbname, $flags );
648	646	if ( $db->isOpen() ) {
649	647	wfDebug( "Connected to $host $dbname.\n" );
650	648	} else {

Follow-up revisions

Revision	Commit summary	Author	Date
r75350	* fixed accidental revert of r75343	freakolowsky	12:06, 25 October 2010
r75444	Followup r75431, bug 24853. Add to RELEASE-NOTES	reedy	17:12, 26 October 2010
r79591	Simplest possible fix for *(bug 26552) ForeignDBRepo broken?...	reedy	19:33, 4 January 2011
r79593	Remove another setting of failFunction in phase3 in a database constructor	reedy	19:54, 4 January 2011
r79595	Kill some FailFunction remenants	reedy	20:04, 4 January 2011
r90266	* (bug 29233) Temporary fix which roughly restores the 1.16 behaviour of open...	tstarling	12:59, 17 June 2011
r96517	Merge r90266 to trunk...	reedy	23:21, 7 September 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r75341	Start of bug 24853, killing off 'functional' parts of failfunction code. Seem...	reedy	20:48, 24 October 2010

Comments

#Comment by Reedy (talk | contribs) 21:31, 24 October 2010

Finally killed after being deprecated in r14625

#Comment by Bryan (talk | contribs) 21:43, 3 January 2011

This breaks ForeignDBRepo. More importantly it breaks all extensions and core code that constructs a Database object. I suggest leaving the $failFunction parameter in the constructor forever, instead of dealing with the unknown side-effects that changing the constructor function signature. (This is why long lists of unnamed parameters sucks)

#Comment by Reedy (talk | contribs) 21:57, 3 January 2011

I was wondering why you'd assigned that bug to me...

#Comment by Bryan (talk | contribs) 21:59, 3 January 2011

Forgot to cross reference :) bug 26552

#Comment by Tim Starling (talk | contribs) 07:09, 17 June 2011

This broke failover in LoadBalancer and caused 15 minutes of downtime on the English Wikipedia (bug 29233).

#Comment by Reedy (talk | contribs) 09:19, 17 June 2011

Surely it's more likely to have actually been r75341?

-			if ( $this->mFailFunction ) {
-				$conn->failFunction( $this->mFailFunction );
-				$conn->reportConnectionError( $this->mLastError );
-			} else {
-				// If all servers were busy, mLastError will contain something sensible
-				throw new DBConnectionError( $conn, $this->mLastError );
-			}
+			// If all servers were busy, mLastError will contain something sensible
+			throw new DBConnectionError( $conn, $this->mLastError );

And unless I'm missing something, it seems rare/I'm struggling to see where it wasn't set to false, and hence, would have actually have been called somewhere...

#Comment by Tim Starling (talk | contribs) 10:31, 17 June 2011

I guess so, I'll mark that one fixme as well. The one place where it wasn't false was the place where almost all database objects are constructed:

-		$db = new $class( $host, $user, $password, $dbname, 1, $flags );
+		$db = new $class( $host, $user, $password, $dbname, $flags );

#Comment by Brion VIBBER (talk | contribs) 23:38, 20 July 2011

Is there anything in this revision that still needs to be fixed, or does the r90266 solution just need to be improved? (My recommendation there: refactor the Database* constructor to not open the connection automatically, and finish cleaning up all the things that call Database* constructors manually when they should probably not be.)

#Comment by 😂 (talk | contribs) 23:28, 7 September 2011

Per the discussion, nothing with this rev is really the problem anymore. r90266 could use improvement, but will suffice for REL1_18. A proper fix in trunk would be ideal.

Marking RESOLVED.

Status & tagging log

23:28, 7 September 2011 😂 (talk | contribs) changed the status of r75343 [removed: fixme added: resolved]
07:09, 17 June 2011 Tim Starling (talk | contribs) changed the status of r75343 [removed: resolved added: fixme]
23:24, 5 January 2011 Reedy (talk | contribs) changed the status of r75343 [removed: fixme added: resolved]
21:43, 3 January 2011 Bryan (talk | contribs) changed the status of r75343 [removed: ok added: fixme]
18:38, 21 November 2010 Hashar (talk | contribs) changed the status of r75343 [removed: new added: ok]