Page MenuHomePhabricator

ConfigRegistry in extension.json doesn't work for CirrusSearch
Closed, ResolvedPublic

Description

For https://gerrit.wikimedia.org/r/#/c/304111

"ConfigRegistry": {
        "CirrusSearch": "CirrusSearch\\SearchConfig::newFromGlobals"
},

gives

mwscript extensions/CirrusSearch/maintenance/updateSearchIndexConfig.php  --wiki cirrustestwiki --reindexAndRemoveOk --indexIdentifier now
PHP Fatal error:  Uncaught exception 'ConfigException' with message 'No registered builder available for CirrusSearch.' in /vagrant/mediawiki/includes/config/ConfigFactory.php:131
Stack trace:
#0 /vagrant/mediawiki/extensions/CirrusSearch/includes/Maintenance/Maintenance.php(52): ConfigFactory->makeConfig('CirrusSearch')
#1 /vagrant/mediawiki/maintenance/doMaintenance.php(95): CirrusSearch\Maintenance\Maintenance->finalSetup()
#2 /vagrant/mediawiki/extensions/CirrusSearch/maintenance/updateSearchIndexConfig.php(65): require_once('/vagrant/mediaw...')
#3 /var/www/w/MWScript.php(95): require_once('/vagrant/mediaw...')
#4 {main}
  thrown in /vagrant/mediawiki/includes/config/ConfigFactory.php on line 131
Fatal error: Uncaught exception 'ConfigException' with message 'No registered builder available for CirrusSearch.' in /vagrant/mediawiki/includes/config/ConfigFactory.php on line 131
ConfigException: No registered builder available for CirrusSearch. in /vagrant/mediawiki/includes/config/ConfigFactory.php on line 131
Call Stack:
    0.0000     240384   1. {main}() /var/www/w/MWScript.php:0
    0.0013     305016   2. require_once('/vagrant/mediawiki/extensions/CirrusSearch/maintenance/updateSearchIndexConfig.php') /var/www/w/MWScript.php:95
    0.0029     737648   3. require_once('/vagrant/mediawiki/maintenance/doMaintenance.php') /vagrant/mediawiki/extensions/CirrusSearch/maintenance/updateSearchIndexConfig.php:65
    0.0263    4547480   4. CirrusSearch\Maintenance\Maintenance->finalSetup() /vagrant/mediawiki/maintenance/doMaintenance.php:95
    0.0285    4848888   5. ConfigFactory->makeConfig() /vagrant/mediawiki/extensions/CirrusSearch/includes/Maintenance/Maintenance.php:52

Event Timeline

Reedy created this task.Aug 10 2016, 10:42 PM
Restricted Application added projects: Discovery, Discovery-Search. · View Herald TranscriptAug 10 2016, 10:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I think it's bug in cirrus maint scripts.

$this->searchConfig = MediaWikiServices::getInstance()
        ->getConfigFactory()
        ->makeConfig( 'CirrusSearch' );

is called in Maintainance::finalSetup() but it looks like the extension.json file is not yet applied.
Moving the call to MediaWikiServices at a later stage fixes the issue.
Unfortunately moving it to getConnection() seems to work for simple maint scripts but will fail on more complex one (the ones that call each others, e.g. updateSearchIndexConfig calls updateOneSearchIndexConfig)

demon added a subscriber: demon.Aug 11 2016, 3:06 AM

Unfortunately moving it to getConnection() seems to work for simple maint scripts but will fail on more complex one (the ones that call each others, e.g. updateSearchIndexConfig calls updateOneSearchIndexConfig)

That updateSearchIndexConfig/updateOneSearchIndexConfig has been in desperate need of refactoring anyway for years now, that's *always* been kludgy since the day it was written.

dcausse added a comment.EditedAug 11 2016, 9:02 AM

After a small refactor I can get it to work and go few steps further but now I encounter the following issue:
$wgCirrusSearchIndexBaseName was set dynamically in CirrusSearch.php with

$wgCirrusSearchIndexBaseName = wfWikiID();

but when extension.json was created I think it was fixed to the default db name

"CirrusSearchIndexBaseName": {
        "value": "wikidb-mw_"
},

Perhaps we need to cleanup CirrusSearch.php from all these "dynamic" settings before generating extension.json?

Not sure to understand what to do...
Maybe we can remove these settings and infer them when we need them, we have a SearchConfig we could add some special cases there to dynamically load default values when this setting is not set in extension.json?

private $defaultSettingsCallback = [
    'CirrusSearchIndexBaseName' => wfWikiID
];
public function get($name) {
        if ( !$this->source->has( $this->prefix . $name ) ) {
                if ( isset( $this->defaultSettingsCallback[$name] ) {
                   return $defaultSettingsCallback[$name];
                }
                return null;
        }
        return $this->source->get( $this->prefix . $name );
}

After a small refactor I can get it to work and go few steps further but now I encounter the following issue:
$wgCirrusSearchIndexBaseName was set dynamically in CirrusSearch.php with

$wgCirrusSearchIndexBaseName = wfWikiID();

but when extension.json was created I think it was fixed to the default db name

"CirrusSearchIndexBaseName": {
        "value": "wikidb-mw_"
},

Perhaps we need to cleanup CirrusSearch.php from all these "dynamic" settings before generating extension.json?

Probably, that looks like it's picked up my local wiki config. Dynamic stuff makes it harder... Same for the autoload and profiles config. Easier to copy them into the entry point first, and then run the migration script

dcausse added a comment.EditedAug 11 2016, 11:25 AM

What do you mean by copy them into the entry point first ?
I think I'll make a couple patches to remove some settings that are not suited for extension.json, e.g.
CirrusSearch.php:

// Will generate a copy of the profile in extension.json, better to use a string to identify and load the profile
$wgCirrusSearchCompletionGeoContextSettings = $wgCirrusSearchCompletionGeoContextProfiles['default'];

or

/**
 * Mapping of result types to CirrusSearch classes.
 */
$wgCirrusSearchFieldTypes = array(
	SearchIndexField::INDEX_TYPE_TEXT => \CirrusSearch\Search\TextIndexField::class,
	SearchIndexField::INDEX_TYPE_KEYWORD => \CirrusSearch\Search\KeywordIndexField::class,
	SearchIndexField::INDEX_TYPE_INTEGER => \CirrusSearch\Search\IntegerIndexField::class,
	SearchIndexField::INDEX_TYPE_NUMBER => \CirrusSearch\Search\NumberIndexField::class,
	SearchIndexField::INDEX_TYPE_DATETIME => \CirrusSearch\Search\DatetimeIndexField::class,
	SearchIndexField::INDEX_TYPE_BOOL => \CirrusSearch\Search\BooleanIndexField::class,
	SearchIndexField::INDEX_TYPE_NESTED => \CirrusSearch\Search\NestedIndexField::class,
);

PHP constants are resolved and hard to read

		"CirrusSearchFieldTypes": {
			"value": {
				"0": "CirrusSearch\\Search\\TextIndexField",
				"1": "CirrusSearch\\Search\\KeywordIndexField",
				"2": "CirrusSearch\\Search\\IntegerIndexField",
				"3": "CirrusSearch\\Search\\NumberIndexField",
				"4": "CirrusSearch\\Search\\DatetimeIndexField",
				"6": "CirrusSearch\\Search\\BooleanIndexField",
				"5": "CirrusSearch\\Search\\NestedIndexField"
			}
		},

The later is not really a config. It seems more appropriate to move this elsewhere, but where? Is it this entry point you're talking about?

What do you mean by copy them into the entry point first ?

I mean, into CirrusSearch.php before running the conversion maintenance script. The require(_once) of other files doesn't work well, so this is the simplest work around

Change 304208 had a related patch set uploaded (by DCausse):
Defer SearchConfig instantiation when it's actually needed

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

debt triaged this task as Normal priority.Aug 11 2016, 4:47 PM
debt moved this task from needs triage to later on... on the Discovery-Search board.

Change 304208 merged by jenkins-bot:
Defer SearchConfig instantiation when it's actually needed

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

Smalyshev added a subscriber: Smalyshev.

Is this done?

Florian closed this task as Resolved.Jan 12 2017, 6:45 AM
Florian assigned this task to dcausse.
Florian added a subscriber: Florian.

I would call this done. The change is merged and no-one responded to @Smalyshev so far. So, if this isn't done, feel free to reopen :P