Page MenuHomePhabricator

[GoogleLogin] includes/config/ConfigFactory.php: No registered builder available for googlelogin.
Closed, ResolvedPublic

Description

With MediaWiki master and GoogleLogin, when I run the installer with install.php --with-extensions the install dies:

Creating main page with default content
done
Creating tables for enabled extensions
ConfigException from line 136 of /workspace/src/includes/config/ConfigFactory.php: No registered builder available for googlelogin.
Backtrace:
#0 extensions/GoogleLogin/includes/GoogleLogin.php(46): ConfigFactory->makeConfig(string)
#1 extensions/GoogleLogin/includes/GoogleLoginHooks.php(20): GoogleLogin\GoogleLogin::getGLConfig()
#2 includes/Hooks.php(174): GoogleLogin\GoogleLoginHooks::onLoadExtensionSchemaUpdates(MysqlUpdater)
#3 includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL) 
#4 includes/installer/DatabaseUpdater.php(127): Hooks::run(string, array)
#5 includes/installer/DatabaseUpdater.php(192): DatabaseUpdater->__construct(Wikimedia\Rdbms\DatabaseMysqli, boolean, NULL)
#6 includes/installer/DatabaseInstaller.php(312): DatabaseUpdater::newForDB(Wikimedia\Rdbms\DatabaseMysqli)
#7 includes/installer/Installer.php(1567): DatabaseInstaller->createExtensionTables(MysqlInstaller)
#8 includes/installer/CliInstaller.php(138): Installer->performInstallation(array, array)
#9 maintenance/install.php(125): CliInstaller->execute()
#10 maintenance/doMaintenance.php(94): CommandLineInstaller->execute()
#11 maintenance/install.php(175): require_once(string)
#12 {main}

That can be reproduced on a Gerrit change by commenting on a change: check experimental.

Event Timeline

Hmm, I'm not sure, if this is something that should be fixed in GoogleLogin. The question here is: Can an extension assume, that it is being fully loaded when the LoadExtensionSchemaUpdates hook is executed? It is, when the update.php script is executed, however, the install.php script kind of goes around the default extension load mechanism, if I see it correctly. It does add all extensions found in the extension directory to a new ExtensionRegistry queue, but instead of loading the data from these extensions with loadFromQueue, it "just" reads the data from these extensions with readFromQueue and manually sets partial data of it into the current MediaWiki session (only wgAutoloadClasses and the LoadExtensionSchemaUpdates hook):

<?php
protected function includeExtensions() {
	global $IP;
	$exts = $this->getVar( '_Extensions' );
	$IP = $this->getVar( 'IP' );

	// Marker for DatabaseUpdater::loadExtensions so we don't
	// double load extensions
	define( 'MW_EXTENSIONS_LOADED', true );

	/**
	 * We need to include DefaultSettings before including extensions to avoid
	 * warnings about unset variables. However, the only thing we really
	 * want here is $wgHooks['LoadExtensionSchemaUpdates']. This won't work
	 * if the extension has hidden hook registration in $wgExtensionFunctions,
	 * but we're not opening that can of worms
	 * @see https://phabricator.wikimedia.org/T28857
	 */
	global $wgAutoloadClasses;
	$wgAutoloadClasses = [];
	$queue = [];

	require "$IP/includes/DefaultSettings.php";

	foreach ( $exts as $e ) {
		if ( file_exists( "$IP/extensions/$e/extension.json" ) ) {
			$queue["$IP/extensions/$e/extension.json"] = 1;
		} else {
			require_once "$IP/extensions/$e/$e.php";
		}
	}

	$registry = new ExtensionRegistry();
	$data = $registry->readFromQueue( $queue );
	$wgAutoloadClasses += $data['autoload'];

	$hooksWeWant = isset( $wgHooks['LoadExtensionSchemaUpdates'] ) ?
		/** @suppress PhanUndeclaredVariable $wgHooks is set by DefaultSettings */
		$wgHooks['LoadExtensionSchemaUpdates'] : [];

	if ( isset( $data['globals']['wgHooks']['LoadExtensionSchemaUpdates'] ) ) {
		$hooksWeWant = array_merge_recursive(
			$hooksWeWant,
			$data['globals']['wgHooks']['LoadExtensionSchemaUpdates']
		);
	}
	// Unset everyone else's hooks. Lord knows what someone might be doing
	// in ParserFirstCallInit (see T29171)
	$GLOBALS['wgHooks'] = [ 'LoadExtensionSchemaUpdates' => $hooksWeWant ];

	return Status::newGood();
}

Shouldn't it instead fully initialize the extensions (which would ensure, that, e.g., the config registries are set correctly)? Adding MediaWiki-Configuration to clarify this question :) Thanks @hashar for bringing this up! :)

The installer explicitly does not load the full extension during the install process. This means the extension has been broken with the web installer for quite a while now :( (also this is not ideal, but it's the rather fragile status quo, that hopefully we can move away from soon)

In any case, I think the code that is failing is entirely unneeded. The comment says // Don't create tables on a shared database. But what the extension should do is create all the database tables, always. Whether the tables get used is up to configuration at run time. But from the installer/updater perspective they should always be there, even if they're going to always be empty.

Ok, good to know, I'm preparing a patch to do this in GoogleLogin :) Thanks @Legoktm!

@Florian when you have a patch you can comment on it check experimental. That will trigger a job that roughly:

  • clone the extensions
  • install.php --with-extensions

--with-extensions is a parameter I have added recently ( 836890daf0efee8b452e432fcd52d024d21d5db2 ). It uses the same logic used by the WebInstaller. That ends up invoking the code you pasted (Installer::includeExtensions).

We might want to better document what is available to LoadExtensionSchemaUpdates. Namely that extensions are read but not loaded. I had a patch to load them in the installer ( https://gerrit.wikimedia.org/r/#/c/430887/ ) which would at least provide the config, but that would break other things:

@Legoktm wrote:

Unfortunately this is going to break a lot of things. Extensions currently operate expecting to run inside of MediaWiki itself. E.g. parser hooks would start triggering and that would be bad.

Or in short, extensions have bunch of hooks that might affect the installation. But maybe we could at least load the configuration settings.


Back on topic, what does that No registered builder available mean? Though I could dig in the code I could use a layman explanation :] ReadingLists has a similar issue ( T196567 ).

Back on topic, what does that No registered builder available mean? Though I could dig in the code I could use a layman explanation :]

The changes to the configuration management of MediaWiki introduced the Config interface, which can be implemented by different configuration management backends (like global vars, which we have today, or, e.g., in-memory databases or the mysql database). This also introduced, that configuration may be scoped in the future, which means, that an extension can and should only have access to the configuration settings it introduced itself, other things shouldn't be a configuration but rather an attribute or whatever.

tl;dr:
Each extension needs to register it's own instance of an implementation of Config (usually it's GlobalVarConfig, which simply falls back to $wgWhatEverConfig) which should be used to access configuration. The instance can be retrieved through the ConfigFactory, which needs these registered "builders" (aka factory methods) to create the config object.

In this case: GoogleLogin requested to have an instance of the GoogleLogin-Config, however, the ConfigRegistry (as the extension is not fully loaded) didn't know a builder for it and can't create such config object than.

I hope that was understandable and not too much text :D

That Config interface is quite amazing. Thank you very much for the very nice explanation :] It all makes sense now.

Change 437792 had a related patch set uploaded (by Legoktm; owner: Florianschmidtwelzow):
[mediawiki/extensions/GoogleLogin@master] Do not conditionally add database tables to the database

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

Change 437792 merged by jenkins-bot:
[mediawiki/extensions/GoogleLogin@master] Do not conditionally add database tables to the database

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

Vvjjkkii renamed this task from [GoogleLogin] includes/config/ConfigFactory.php: No registered builder available for googlelogin. to 2tbaaaaaaa.Jul 1 2018, 1:06 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed Florian as the assignee of this task.
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from 2tbaaaaaaa to [GoogleLogin] includes/config/ConfigFactory.php: No registered builder available for googlelogin..Jul 2 2018, 1:41 AM
CommunityTechBot closed this task as Resolved.
CommunityTechBot assigned this task to Florian.
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.

Change 487080 had a related patch set uploaded (by Florianschmidtwelzow; owner: Florianschmidtwelzow):
[mediawiki/extensions/GoogleLogin@REL1_31] Do not conditionally add database tables to the database

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

Change 487080 merged by jenkins-bot:
[mediawiki/extensions/GoogleLogin@REL1_31] Do not conditionally add database tables to the database

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