Page MenuHomePhabricator

OATHAuth use of global defined in extension.json isn't available at install time
Closed, ResolvedPublic

Description

Steps to reproduce:
Visit https://patchdemo.wmflabs.org/
Choose the minimal preset
Enable the OATHAuth
Try creating the wiki

Expected result:
Wiki is created

Actual result:

InvalidArgumentException from line 83 of includes/libs/rdbms/database/domain/DatabaseDomain.php: Domain must be a string or Wikimedia\Rdbms\DatabaseDomain
#0 includes/libs/rdbms/loadbalancer/LoadBalancer.php(297): Wikimedia\Rdbms\DatabaseDomain::newFromId(NULL)
#1 includes/libs/rdbms/loadbalancer/LoadBalancer.php(276): Wikimedia\Rdbms\LoadBalancer->resolveDomainInstance(NULL)
#2 includes/libs/rdbms/loadbalancer/LoadBalancer.php(1042): Wikimedia\Rdbms\LoadBalancer->resolveDomainID(NULL)
#3 extensions/OATHAuth/src/Hook/LoadExtensionSchemaUpdates/UpdateTables.php(95): Wikimedia\Rdbms\LoadBalancer->getConnectionRef(-2, Array, NULL)
#4 includes/installer/DatabaseUpdater.php(532): MediaWiki\Extension\OATHAuth\Hook\LoadExtensionSchemaUpdates\UpdateTables->schemaUpdateOldUsersFromInstaller(Object(MysqlUpdater))
#5 includes/installer/DatabaseUpdater.php(500): DatabaseUpdater->runUpdates(Array, true)
#6 includes/installer/DatabaseInstaller.php(348): DatabaseUpdater->doUpdates(Array)
#7 includes/installer/Installer.php(1723): DatabaseInstaller->createExtensionTables(Object(MysqlInstaller))
#8 includes/installer/CliInstaller.php(211): Installer->performInstallation(Array, Array)
#9 maintenance/install.php(140): CliInstaller->execute()
#10 maintenance/doMaintenance.php(106): CommandLineInstaller->execute()
#11 maintenance/install.php(201): require_once('/var/www/html/w...')
#12 {main}

Suspected cause: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/641822

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.
DannyS712 added subscribers: aaron, Krinkle.

https://github.com/MatmaRex/patchdemo/issues/182

The check failing works as expected. Unless other impact, ignoring for now. It indeed does not appear to have been working previously either. From a quick look it seems OATHAuth appears to be using this hook to migrate old data, which isn't applicable to new installs. It should probaly either do nothing if the variable is unset, or it should otherwise ensure the function is never called in the first place.

I've monkey-patched patchdemo.wmflabs.org by removing this extension so this isn't reproduceable there, but the problem persists.

$dbw = $lb->getConnectionRef( DB_MASTER, [], $wgOATHAuthDatabase );
	/**
	 * Get a live database handle reference for a real or virtual (DB_MASTER/DB_REPLICA) server index
	 *
	 * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
	 * (e.g. sqlite) in order to avoid deadlocks. getServerAttributes()
	 * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
	 * on any CONN_TRX_AUTOCOMMIT connections.
	 *
	 * @see ILoadBalancer::getConnection() for parameter information
	 *
	 * @param int $i Server index or DB_MASTER/DB_REPLICA
	 * @param string[]|string $groups Query group(s) in preference order; [] for the default group
	 * @param string|bool $domain DB domain ID or false for the local domain
	 * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT)
	 * @return DBConnRef
	 */
	public function getConnectionRef( $i, $groups = [], $domain = false, $flags = 0 );

I guess this is the problem

		"OATHAuthDatabase": {
			"value": false
		},

However, when the default in the PHP interface is false.. And the fact having a default does basically make the parameter optional... Which makes https://gerrit.wikimedia.org/r/c/mediawiki/core/+/641822 not the most sensible (ignoring the below)

Sure, it has DatabaseDomain|string in the function docs of DatabaseDomain::newFromId()... But when the upstream callers are defaulting to false... It feels like the intermediate plumbing should be converting false into something else

Also, other code such as MediaWiki-extensions-UrlShortener have basically the same code

	public static function getDB( int $type ) : IDatabase {
		global $wgUrlShortenerDBName, $wgUrlShortenerDBCluster;
		$lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
		$lb = $wgUrlShortenerDBCluster
			? $lbFactory->getExternalLB( $wgUrlShortenerDBCluster )
			: $lbFactory->getMainLB( $wgUrlShortenerDBName );

		return $lb->getConnectionRef( $type, [], $wgUrlShortenerDBName );
	}

and then config

		"UrlShortenerDBName": {
			"value": false
		},

But in the same way, if it's false, it uses the local/current database, rather than a different one. OATHAuth and UrlShortener are the same in that regard

Looking at the stack I see it's not false, it's NULL

#3 extensions/OATHAuth/src/Hook/LoadExtensionSchemaUpdates/UpdateTables.php(95): Wikimedia\Rdbms\LoadBalancer->getConnectionRef(-2, Array, NULL)

I'm guessing something with the config not being actually loaded? Same as using any global that hasn't had a value set...

There is code that handles the default of false presumably (see above), but doesn't handle null... Because it's not an expected value

So if it's not being loaded, or globals turned/exported into globals.. Does that make it an MediaWiki-Installer bug?

@Reedy I came to the same conclusion while debugging. Switching null to false ($db ?? false) seemed to fix things, so I can push a patch to do that for now?

Change 649943 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/OATHAuth@master] Convert null global to false

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

Yeah, you'll want to do it for the other "updater" functions too, but I'm sure you already noticed that (why we have the same code 3 times... is a different question. I'll fix that)

Obviously should be reverted out when whatever underlying bug is actually solved

Change 649943 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/OATHAuth@master] Convert null global to false

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

I've just rebased your patch ontop of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OATHAuth/+/649945/1

And of course, mine now fails with the same error as seen on patch demo :)

17:53:10 Creating oathauth_users table ...InvalidArgumentException from line 83 of /workspace/src/includes/libs/rdbms/database/domain/DatabaseDomain.php: Domain must be a string or Wikimedia\Rdbms\DatabaseDomain
17:53:10 #0 /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php(297): Wikimedia\Rdbms\DatabaseDomain::newFromId(NULL)
17:53:10 #1 /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php(276): Wikimedia\Rdbms\LoadBalancer->resolveDomainInstance(NULL)
17:53:10 #2 /workspace/src/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1042): Wikimedia\Rdbms\LoadBalancer->resolveDomainID(NULL)
17:53:10 #3 /workspace/src/extensions/OATHAuth/src/Hook/LoadExtensionSchemaUpdates/UpdateTables.php(91): Wikimedia\Rdbms\LoadBalancer->getConnectionRef(-2, Array, NULL)
17:53:10 #4 /workspace/src/extensions/OATHAuth/src/Hook/LoadExtensionSchemaUpdates/UpdateTables.php(102): MediaWiki\Extension\OATHAuth\Hook\LoadExtensionSchemaUpdates\UpdateTables::getDatabase()
17:53:10 #5 /workspace/src/includes/installer/DatabaseUpdater.php(532): MediaWiki\Extension\OATHAuth\Hook\LoadExtensionSchemaUpdates\UpdateTables->schemaUpdateOldUsersFromInstaller(Object(MysqlUpdater))
17:53:10 #6 /workspace/src/includes/installer/DatabaseUpdater.php(500): DatabaseUpdater->runUpdates(Array, true)
17:53:10 #7 /workspace/src/includes/installer/DatabaseInstaller.php(348): DatabaseUpdater->doUpdates(Array)
17:53:10 #8 /workspace/src/includes/installer/Installer.php(1723): DatabaseInstaller->createExtensionTables(Object(MysqlInstaller))
17:53:10 #9 /workspace/src/includes/installer/CliInstaller.php(211): Installer->performInstallation(Array, Array)
17:53:10 #10 /workspace/src/maintenance/install.php(140): CliInstaller->execute()
17:53:10 #11 /workspace/src/maintenance/doMaintenance.php(106): CommandLineInstaller->execute()
17:53:10 #12 /workspace/src/maintenance/install.php(201): require_once('/workspace/src/...')
17:53:10 #13 {main}
17:53:10 done.
Reedy renamed this task from InvalidArgumentException when installing OATHAuth to OATHAuth use of global defined in extension.json isn't availalble at install time.Dec 16 2020, 6:20 PM
Reedy added a project: MediaWiki-Installer.

Change 649943 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Convert null global to false

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

Reedy renamed this task from OATHAuth use of global defined in extension.json isn't availalble at install time to OATHAuth use of global defined in extension.json isn't available at install time.Dec 16 2020, 6:28 PM
Reedy lowered the priority of this task from High to Medium.
Reedy removed a project: Patch-For-Review.

Verified this is working on my local patchdemo install. Thanks all.

matmarex assigned this task to Esanders.

Looks resolved from here. Thanks.

Do we want to turn this into another bug for the MW installer part?