DatabaseTest::testFactory test cases for mysqli using unnamespaced DatabaseMysqli (blocking remove of class alias)
Closed, ResolvedPublic
Actions

Description

While testing without aliases in the core phpunit tests (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/800799) the tests in DatabaseTest::testFactory returns the unnamespaced DatabaseMysqli, which does not exists in my patch set and the assertion gets null and a failure

I would assume that the optional driver class in the array is used instead

		static $builtinTypes = [
			'mysql' => [ 'mysqli' => DatabaseMysqli::class ],
			'sqlite' => DatabaseSqlite::class,
			'postgres' => DatabasePostgres::class,
		];

But the default fallback is used:

		if ( !isset( $builtinTypes[$dbType] ) ) {
			// Not a built in type, assume standard naming scheme
			return 'Database' . ucfirst( $dbType );
		}

This is not a problem in production because the alias exists, but it blocks a possible removable of the deprecated class alias

Details

	Subject	Repo	Branch	Lines +/-
	rdbms: Fix test to not rely on deprecated class aliases	mediawiki/core	master	+3 -3

Customize query in gerrit

Event Timeline

Umherirrender created this task.May 27 2022, 9:29 PM

Restricted Application added a project: Performance-Team. · View Herald TranscriptMay 27 2022, 9:29 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Umherirrender updated the task description. (Show Details)May 27 2022, 9:29 PM

Umherirrender added a project: Technical-Debt (Deprecation process).May 27 2022, 9:42 PM

The fallback needs can't use the namespace as this for extensions, not for built-ins. I believe you know that already, but I'm stating it here for context.

I'm a bit confused as to what the problem is. As I understand it, the $builtinTypes array is keyed by db types, not partial class names. mysqli would be an invalid type, and I do not see how such a value could end up here.

The value of the $builtinTypes array is either a class name string, or, in case we want to check for a php package being installed and enabled, there can be a subkey of a native PHP ext which if loaded we then use the class name of that.

DatabaseMysqli is already specified there, and expanded with ::class to its actual namespaced name, not the global alias. If this logic isnt working we should fix it, but adding mysqli as its own type seems incorrect and would be unused/impossible code, based on the above understanding.

The value mysqli ends up there in the tests:

DatabaseTest

		$this->assertInstanceOf( DatabaseMysqli::class, Database::factory( 'mysqli', $p, $m ) );
		$this->assertInstanceOf( DatabaseMysqli::class, Database::factory( 'MySqli', $p, $m ) );
		$this->assertInstanceOf( DatabaseMysqli::class, Database::factory( 'MySQLi', $p, $m ) );

I have assumed the values given in test to the factory are valid values and also used the same in production

That is not the case. It looks like that was added by mistake in https://gerrit.wikimedia.org/r/415467, it was once fixed in a later change but that was since reverted for other reasons. I suggest removing the i there from those test cases.

Umherirrender renamed this task from Wikimedia\Rdbms\Database::getClass returns unnamespaced DatabaseMysqli (blocking remove of class alias) to DatabaseTest::testFactory test cases for mysqli using unnamespaced DatabaseMysqli (blocking remove of class alias).May 28 2022, 1:49 PM

Krinkle moved this task from Inbox, needs triage to Backlog: Maintenance, non-prioritized on the Performance-Team board.May 31 2022, 6:10 PM

Umherirrender unsubscribed.Jun 19 2022, 7:32 PM

Krinkle triaged this task as Low priority.Oct 15 2022, 7:29 PM

Krinkle moved this task from Untriaged to Rdbms: MySQL support on the MediaWiki-libs-Rdbms board.

Change 842943 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] rdbms: Fix test to not rely on deprecated class aliases

https://gerrit.wikimedia.org/r/842943

gerritbot added a project: Patch-For-Review.Oct 15 2022, 10:10 PM

Krinkle claimed this task.Oct 15 2022, 10:10 PM

Krinkle closed this task as Resolved.Oct 18 2022, 6:47 PM

Krinkle removed a project: Patch-For-Review.

Change 842943 merged by jenkins-bot: