Page MenuHomePhabricator

Error when getting templatedata of all templates
Closed, ResolvedPublic

Description

When attempting to use a generator to get templatedata of all templates, I get the following error:

POST api.php?action=templatedata&format=json&generator=allpages&gapnamespace=10

api.php Error from line 86 of includes/api/ApiQueryGeneratorBase.php: Call to a member function addGeneratorContinueParam() on null
Backtrace:
#0 includes/api/ApiQueryAllPages.php(229): ApiQueryGeneratorBase->setContinueEnumParameter(string, string)
#1 includes/api/ApiQueryAllPages.php(52): ApiQueryAllPages->run(ApiPageSet)
#2 includes/api/ApiPageSet.php(176): ApiQueryAllPages->executeGenerator(ApiPageSet)
#3 includes/api/ApiPageSet.php(140): ApiPageSet->executeInternal(boolean)
#4 extensions/TemplateData/api/ApiTemplateData.php(59): ApiPageSet->execute()
#5 includes/api/ApiMain.php(1579): ApiTemplateData->execute()
#6 includes/api/ApiMain.php(535): ApiMain->executeAction()
#7 includes/api/ApiMain.php(506): ApiMain->executeActionWithErrorHandling()
#8 api.php(94): ApiMain->execute()
#9 {main}

The ApiStructureTest::testParameterConsistency test is also failing, and I think for a related reason (e.g.).

ApiPageSet contains the following declaration for the generator API parameter:

	public function getAllowedParams( $flags = 0 ) {
		/*…*/
			'generator' => [
				ApiBase::PARAM_TYPE => null,
				ApiBase::PARAM_HELP_MSG => 'api-pageset-param-generator',
				ApiBase::PARAM_SUBMODULE_PARAM_PREFIX => 'g',
			],

but the documentation for ApiBase::PARAM_SUBMODULE_PARAM_PREFIX says it should be used with ApiBase::PARAM_TYPE => 'submodule'. Is that correct? I've tried changing it but other things break.

Event Timeline

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

@Simetrical is this something you know about? (You added the above failing test I think.)

I don't think this has anything to do with VisualEditor, Herald. But whatever you reckon, you go for it. :)

I wrote the test in accordance with the comments in ApiBase.php:

/**
 * (string) When PARAM_TYPE is 'submodule', used to indicate the 'g' prefix
 * added by ApiQueryGeneratorBase (and similar if anything else ever does that).
 * @since 1.26
 */

But ApiPageSet is currently the only class in core that uses it, and it has PARAM_TYPE null. (It escapes this check because the parameter only exists conditionally.) And anyway I can't see anything that uses the value at all except ParamInfo. And nothing ever has, and it was never used for anything else, and the param in ApiPageSet was always null, so it seems the comment was always wrong. df80f1ea was the original commit where it was added, and no commit since then to core has ever had 'PARAM_SUBMODULE_PARAM_PREFIX' in an added or removed line except mine.

So it seems the comment and ApiStructureTest should just be fixed. Brad?

What's the param actually supposed to do? Just output the 'g' in paraminfo? What's the use-case? This should probably be documented in the comment.

Change 425784 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Remove required type for PARAM_SUBMODULE_PARAM_PREFIX

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

But please note that this has nothing to do with the first issue reported in this bug, which I understand was the real problem you had. That's because it's calling getContinuationManager() when the continuation manager is null.

What's the param actually supposed to do? Just output the 'g' in paraminfo? What's the use-case? This should probably be documented in the comment.

Yes, it just outputs the 'g' in paraminfo. The use case is to communicate to Special:ApiSandbox that submodules used via the generator parameter have all their parameters prefixed with 'g', so they don't collide if the same submodule gets used via query's list or prop parameters. Without this ApiSandbox would have to somehow hard-code that logic.

I see two bugs in TemplateData:

1) It adds ApiPageSet's parameters unconditionally in ApiTemplateData::getAllowedParams(), while all core modules only add them when $flags is non-zero. This makes sense: the module itself doesn't actually use the parameters, it only needs to include them when getAllowedParams() is being called to generate the help.

And that's why the core tests passed when @Simetrical's patch was merged: ApiPageSet only violates the $paramTypes constraint when in non-help mode, in which case the parameters aren't included in the modules' output to be tested.

2) ApiTemplateData is not setting up an ApiContinuationManager, which is needed if it's going to use generator modules that might have continuation, thus the "Call to a member function addGeneratorContinueParam() on null" error.

Doing so is pretty simple. At the top of execute() add

$continuationManager = new ApiContinuationManager( $this, [], [] );
$this->setContinuationManager( $continuationManager );

And at the end (and before any early return), call

$this->setContinuationManager( null );
$continuationManager->setContinuationIntoResult( $this->getResult() );

Change 425941 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/TemplateData@master] Add ContinuationManager and fix up AllowedParams

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

Thanks @Anomie that's super useful. I've made the changes you suggest https://gerrit.wikimedia.org/r/#/c/425941/ and this task's two problems are fixed (and the TemplateWizard patch is passing). :-)

See what you think.

And at the end (and before any early return), call

$this->setContinuationManager( null );
$continuationManager->setContinuationIntoResult( $this->getResult() );

Would it make sense to use a ScopedCallback in these cases then?

Change 425941 merged by jenkins-bot:
[mediawiki/extensions/TemplateData@master] Add ContinuationManager and fix up AllowedParams

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

Would it make sense to use a ScopedCallback in these cases then?

Perhaps. Most API modules don't return early though, except by throwing ApiUsageExceptions in which case we can safely skip the continuation stuff.

Change 425784 abandoned by simetrical:
Remove required type for PARAM_SUBMODULE_PARAM_PREFIX

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