Skip to content

Fix filtered format multiple srf filtered config - #1052

Open
thomas-topway-it wants to merge 2 commits into
SemanticMediaWiki:masterfrom
Knowledge-Wiki:fix-filtered-format-multiple-srfFilteredConfig
Open

Fix filtered format multiple srf filtered config#1052
thomas-topway-it wants to merge 2 commits into
SemanticMediaWiki:masterfrom
Knowledge-Wiki:fix-filtered-format-multiple-srfFilteredConfig

Conversation

@thomas-topway-it

Copy link
Copy Markdown
Contributor

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.86%. Comparing base (f04a0f4) to head (6191071).

Files with missing lines Patch % Lines
formats/filtered/src/Hooks.php 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1052      +/-   ##
============================================
- Coverage     52.88%   52.86%   -0.03%     
- Complexity     2439     2440       +1     
============================================
  Files            81       81              
  Lines          9281     9283       +2     
============================================
- Hits           4908     4907       -1     
- Misses         4373     4376       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@krabina krabina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot comment on the code, but this PR fixed the problem we encountered.

@gesinn-it-gea

gesinn-it-gea commented Jul 6, 2026

Copy link
Copy Markdown
Member

Reviewed and verified locally against a real MediaWiki + Semantic MediaWiki instance.

Root cause confirmed: this regressed in 6169690 (24 Apr 2026, "clarify RequestContext fallback and fix missing JS config var"), which replaced ParserOutput::addJsConfigVars() (silently overwrites) with ParserOutput::setJsConfigVar() (throws InvalidArgumentException when called twice with different values for the same key). Before that commit this scenario worked fine, which matches what's reported in #1039 ("this was no problem in previous versions").

The fix here is correct: moving the srfFilteredConfig assignment into onOutputPageParserOutput means it now runs exactly once per page (after the parser has already merged all #ask instances into ParserOutput::setExtensionData()), and it uses OutputPage::addJsConfigVars(), which overwrites rather than throwing on conflicting values — so the multi-instance conflict is structurally impossible now.

I wrote and verified a regression test reproducing the exact crash from #1039 (two format=filtered instances on one page):

<?php

namespace SRF\Tests\Filtered;

use MediaWikiIntegrationTestCase;
use SRF\Filtered\Filtered;
use SRF\Filtered\Hooks;

/**
 * Regression test for https://github.com/SemanticMediaWiki/SemanticResultFormats/issues/1039
 *
 * When a page contains more than one {{#ask:format=filtered}} instance, each instance
 * used to call ParserOutput::setJsConfigVar( 'srfFilteredConfig', $accumulatedConfig )
 * with a different array value for the same key, which MediaWiki core rejects with
 * "Multiple conflicting values given for srfFilteredConfig" (InvalidArgumentException).
 *
 * @covers \SRF\Filtered\Filtered
 * @covers \SRF\Filtered\Hooks
 * @group semantic-result-formats
 */
class MultipleFilteredInstancesTest extends MediaWikiIntegrationTestCase {

	private function addConfigToOutput( Filtered $printer, $id, array $config ) {
		$method = new \ReflectionMethod( Filtered::class, 'addConfigToOutput' );
		$method->setAccessible( true );
		$method->invoke( $printer, $id, $config );
	}

	public function testTwoFilteredInstancesOnSamePage_doNotThrow() {
		$parserOutput = new \ParserOutput();
		$parser = $this->createMock( \Parser::class );
		$parser->method( 'getOutput' )->willReturn( $parserOutput );

		$firstInstance = new Filtered( null );
		$firstInstance->setParser( $parser );

		$secondInstance = new Filtered( null );
		$secondInstance->setParser( $parser );

		// Simulate two independent #ask calls with format=filtered on the same page.
		$this->addConfigToOutput( $firstInstance, 'filtered-1', [ 'views' => [ 'list' ] ] );
		$this->addConfigToOutput( $secondInstance, 'filtered-2', [ 'views' => [ 'table' ] ] );

		$mergedConfig = $parserOutput->getExtensionData( 'srf-filtered-config' );
		$this->assertSame(
			[
				'filtered-1' => [ 'views' => [ 'list' ] ],
				'filtered-2' => [ 'views' => [ 'table' ] ],
			],
			$mergedConfig
		);

		// This is the call that used to throw InvalidArgumentException before the fix,
		// because Filtered::addConfigToOutput no longer calls setJsConfigVar() directly.
		$outputPage = new \OutputPage( \RequestContext::getMain() );
		Hooks::onOutputPageParserOutput( $outputPage, $parserOutput );

		$this->assertSame( $mergedConfig, $outputPage->getProperty( 'srf-filtered-config' ) );
		$this->assertSame( $mergedConfig, $outputPage->getJsConfigVars()['srfFilteredConfig'] );
	}
}

Place at tests/phpunit/Unit/Filtered/MultipleFilteredInstancesTest.php. Verified:

One small nit unrelated to correctness: Filtered.php leaves the old setJsConfigVar call commented out (with a // *** do not set here... note) instead of deleting it — worth removing the dead line since the hook change fully explains the removal.

@gesinn-it-gea

gesinn-it-gea commented Jul 6, 2026

Copy link
Copy Markdown
Member

@thomas-topway-it would you like to add the test before merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[filtered] Multiple conflicting values given for srfFilteredConfig

3 participants