Page MenuHomePhabricator
Create Task
Maniphest T270147

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

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

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
Convert null global to falseEsandersmediawiki/extensions/OATHAuthmaster+4 -2
Customize query in gerrit

Related Objects

Event Timeline

DannyS712 created this task.Dec 15 2020, 1:51 AM
Restricted Application added a project: User-DannyS712. · View Herald TranscriptDec 15 2020, 1:51 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 triaged this task as High priority.Dec 15 2020, 1:51 AM
DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.
DannyS712 added subscribers: aaron, Krinkle.
Krinkle added a comment.Dec 15 2020, 1:59 AM

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.

Krinkle unsubscribed.Dec 15 2020, 1:59 AM
• matmarex subscribed.Dec 15 2020, 10:41 PM
Esanders subscribed.Dec 16 2020, 5:06 PM

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

Esanders added subscribers: Umherirrender, Reedy, Legoktm.Dec 16 2020, 5:10 PM

According to https://www.mediawiki.org/wiki/Developers/Maintainers this extension is unmaintained.

Pinging all-time contributors (according to https://github.com/wikimedia/mediawiki-extensions-OATHAuth/graphs/contributors) @Reedy @Umherirrender @Legoktm

Esanders updated the task description. (Show Details)Dec 16 2020, 5:11 PM
Reedy added a comment.EditedDec 16 2020, 5:37 PM
$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?

Esanders added a comment.Dec 16 2020, 5:42 PM

@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?

gerritbot added a comment.Dec 16 2020, 5:44 PM

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

gerritbot added a project: Patch-For-Review.Dec 16 2020, 5:44 PM
Reedy added a comment.Dec 16 2020, 5:46 PM

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

Reedy added a comment.Dec 16 2020, 5:54 PM
In T270147#6696269, @gerritbot wrote:

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.
gerritbot added a comment.Dec 16 2020, 6:22 PM

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.
ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.25; 2021-01-05).Dec 16 2020, 7:00 PM
Esanders added a comment.Dec 16 2020, 7:14 PM

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

• matmarex closed this task as Resolved.Dec 17 2020, 5:27 PM
• matmarex assigned this task to Esanders.

Looks resolved from here. Thanks.

Reedy added a comment.Dec 17 2020, 5:33 PM

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

Reedy mentioned this in T270968: Config defined in extension.json isn't available at install time.Dec 31 2020, 1:20 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits