From f8d43ea9f93c114adb0e047a6e5b0b837916e1df Mon Sep 17 00:00:00 2001 From: Morne Alberts Date: Wed, 20 May 2026 19:32:34 +0200 Subject: [PATCH] Fix modal trigger not opening modals on MediaWiki 1.43+ The deferred-content mechanism (ParserOutput::setExtensionData on the parse side, ParserOutput::getExtensionData in the OutputPageParserOutput hook) no longer preserves data across the parser hook -> render hook transition under MediaWiki 1.43's ParserOutputAccess lifecycle changes. The set-side and get-side operate on different ParserOutput instances, so the modal container markup never reaches the page. The modal trigger still renders but has nothing to open. Switch to inline emission: ModalBuilder::parse() returns the trigger HTML and the modal container HTML concatenated. The modal container is position: fixed and addressed via data-target="#id", so its DOM placement is no longer significant. This works identically on every supported MediaWiki version because the underlying Bootstrap behaviour is the same and RemexHtml (default since MW 1.36) cleanly handles a block element inside a wikitext paragraph. Removes the now-unused ParserOutputHelper::injectLater() helper and the EXTENSION_DATA_DEFERRED_CONTENT_KEY constant. Updates the four parallel test files (ModalBuilderTest, ImageModalTest, ModalTest, OutputPageParserOutputTest) to assert the new concatenated return shape and drop the deferred-content mock expectations. Verified on a MW 1.39 + Vector + BC stack: modal opens identically pre- and post-patch (no regression on the oldest supported MW version). PHPUnit unit suite stays green relative to baseline; the 6 pre-existing ImageModalTriggerTest failures are present in both runs and unrelated to this change. Co-Authored-By: Claude Opus 4.7 --- src/ApplicationFactory.php | 2 +- src/BootstrapComponents.php | 2 - src/Hooks/OutputPageParserOutput.php | 45 +++---------------- src/ModalBuilder.php | 14 +++--- src/ParserOutputHelper.php | 24 ---------- tests/phpunit/Unit/Components/ModalTest.php | 12 +---- .../Unit/Hooks/OutputPageParserOutputTest.php | 43 +++++++++--------- tests/phpunit/Unit/ImageModalTest.php | 10 +---- tests/phpunit/Unit/ModalBuilderTest.php | 12 +---- 9 files changed, 41 insertions(+), 123 deletions(-) diff --git a/src/ApplicationFactory.php b/src/ApplicationFactory.php index 2048a69..852e38d 100644 --- a/src/ApplicationFactory.php +++ b/src/ApplicationFactory.php @@ -107,7 +107,7 @@ public function getNewAttributeManager( array $validAttributes, array $aliases ) * @param string $id * @param string $trigger must be safe raw html (best run through {@see Parser::recursiveTagParse}) * @param string $content must be safe raw html (best run through {@see Parser::recursiveTagParse}) - * @param ParserOutputHelper $parserOutputHelper + * @param ParserOutputHelper $parserOutputHelper @deprecated unused since the inline-emission modal fix; will be removed in the next major release. * * @see ModalBuilder::__construct * diff --git a/src/BootstrapComponents.php b/src/BootstrapComponents.php index 628c5d8..9320e15 100644 --- a/src/BootstrapComponents.php +++ b/src/BootstrapComponents.php @@ -56,8 +56,6 @@ */ class BootstrapComponents { - const EXTENSION_DATA_DEFERRED_CONTENT_KEY = 'bsc_deferredContent'; - const EXTENSION_DATA_NO_IMAGE_MODAL = 'bsc_no_image_modal'; diff --git a/src/Hooks/OutputPageParserOutput.php b/src/Hooks/OutputPageParserOutput.php index 64dae49..7dd7350 100644 --- a/src/Hooks/OutputPageParserOutput.php +++ b/src/Hooks/OutputPageParserOutput.php @@ -26,7 +26,6 @@ namespace MediaWiki\Extension\BootstrapComponents\Hooks; -use MediaWiki\Extension\BootstrapComponents\BootstrapComponents; use MediaWiki\Extension\BootstrapComponents\BootstrapComponentsService; /* * TODO switch to these, wehen we drop support for mw < 1.40 @@ -41,25 +40,12 @@ * * Called after parse, before the HTML is added to the output. * - * Method delegated to separate class to fix missing (deferred) content in - * {@see \MediaWiki\Extension\BootstrapComponents\Tests\Integration\BootstrapComponentsJSONScriptTestCaseRunnerTest::assertParserOutputForCase} - * * @see https://www.mediawiki.org/wiki/Manual:Hooks/OutputPageParserOutput * * @since 1.2 */ class OutputPageParserOutput { - /** - * @var string - */ - const INJECTION_PREFIX = ''; - - /** - * @var string - */ - const INJECTION_SUFFIX = ''; - /** * @var BootstrapComponentsService */ @@ -72,6 +58,8 @@ class OutputPageParserOutput { /** * @var ParserOutput $parserOutput + * + * @deprecated unused since the inline-emission modal fix; will be removed in the next major release. */ private ParserOutput $parserOutput; @@ -79,7 +67,7 @@ class OutputPageParserOutput { * OutputPageParserOutput constructor. * * @param OutputPage $outputPage - * @param ParserOutput $parserOutput + * @param ParserOutput $parserOutput @deprecated unused since the inline-emission modal fix; will be removed in the next major release. * @param BootstrapComponentsService $service */ public function __construct( @@ -94,36 +82,11 @@ public function __construct( * @return void */ public function process(): void { - $deferredText = $this->getContentForLaterInjection( $this->getParserOutput() ); - if ( !empty( $deferredText ) ) { - $this->getOutputPage()->addHTML( $deferredText ); - } - if ( $this->getBootstrapComponentsService()->vectorSkinInUse() ) { $this->getOutputPage()->addModules( [ 'ext.bootstrapComponents.vector-fix' ] ); } } - /** - * Returns the raw html that is to be inserted at the end of the page. - * - * @param ParserOutput $parserOutput - * - * @return string - */ - protected function getContentForLaterInjection( ParserOutput $parserOutput ): string { - $deferredContent = $parserOutput - ->getExtensionData(BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY ); - - if ( empty( $deferredContent ) || !is_array( $deferredContent ) ) { - return ''; - } - - // clearing extension data for unit and integration tests to work - $parserOutput->setExtensionData( BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY, null ); - return self::INJECTION_PREFIX . implode( array_values( $deferredContent ) ) . self::INJECTION_SUFFIX; - } - protected function getBootstrapComponentsService(): BootstrapComponentsService { return $this->bootstrapComponentService; } @@ -136,6 +99,8 @@ protected function getOutputPage(): OutputPage { } /** + * @deprecated unused since the inline-emission modal fix; will be removed in the next major release. + * * @return ParserOutput */ protected function getParserOutput(): ParserOutput { diff --git a/src/ModalBuilder.php b/src/ModalBuilder.php index d1b6f23..8c7506c 100644 --- a/src/ModalBuilder.php +++ b/src/ModalBuilder.php @@ -102,6 +102,8 @@ class ModalBuilder { /** * @var ParserOutputHelper $parserOutputHelper + * + * @deprecated unused since the inline-emission modal fix; will be removed in the next major release. */ private $parserOutputHelper; @@ -146,7 +148,7 @@ public static function wrapTriggerElement( $element, $id ) { * @param string $id * @param string $trigger must be safe raw html (best run through {@see Parser::recursiveTagParse}) * @param string $content must be fully parsed html (use {@see Parser::recursiveTagParseFully}) - * @param ParserOutputHelper $parserOutputHelper + * @param ParserOutputHelper $parserOutputHelper @deprecated unused since the inline-emission modal fix; will be removed in the next major release. * * @see ApplicationFactory::getNewModalBuilder * @see Components\Modal::generateButton @@ -163,14 +165,14 @@ public function __construct( $id, $trigger, $content, $parserOutputHelper ) { /** * Parses the modal. * + * Emits the trigger followed by the modal container inline. The container is + * `position: fixed` and is addressed via `data-target="#id"`, so its DOM + * placement is not significant. + * * @return string */ public function parse() { - $this->parserOutputHelper->injectLater( - $this->getId(), - $this->buildModal() - ); - return $this->buildTrigger(); + return $this->buildTrigger() . $this->buildModal(); } /** diff --git a/src/ParserOutputHelper.php b/src/ParserOutputHelper.php index 9877ff0..af7542c 100644 --- a/src/ParserOutputHelper.php +++ b/src/ParserOutputHelper.php @@ -138,30 +138,6 @@ public function areImageModalsSuppressed() { ->getExtensionData( BootstrapComponents::EXTENSION_DATA_NO_IMAGE_MODAL ); } - /** - * Allows for html fragments for a given container id to be stored, so that it can be added to the page at a later time. - * - * @param string $id - * @param string $rawHtml - * - * @return ParserOutputHelper $this (fluid) - */ - public function injectLater( $id, $rawHtml ) { - if ( empty( $this->getParser()->getOutput() ) ) { - # fix issues with PageForms file upload (issue #20) - return $this; - } - if ( !empty( $rawHtml ) ) { - $deferredContent = $this->getParser()->getOutput()->getExtensionData( BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY ); - if ( empty( $deferredContent ) ) { - $deferredContent = []; - } - $deferredContent[$id] = $rawHtml; - $this->getParser()->getOutput()->setExtensionData( BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY, $deferredContent ); - } - return $this; - } - /** * Formats a text as error text, so it can be added to the output. * diff --git a/tests/phpunit/Unit/Components/ModalTest.php b/tests/phpunit/Unit/Components/ModalTest.php index f0d6991..3dd6f4b 100644 --- a/tests/phpunit/Unit/Components/ModalTest.php +++ b/tests/phpunit/Unit/Components/ModalTest.php @@ -49,15 +49,9 @@ public function testCanConstruct() { */ public function testCanRender( $input, $arguments, $expectedTriggerOutput, $expectedModalOutput ) { - $modalInjection = ''; $parserOutputHelper = $this->getMockBuilder( 'MediaWiki\\Extension\\BootstrapComponents\\ParserOutputHelper' ) ->disableOriginalConstructor() ->getMock(); - $parserOutputHelper->expects( $this->any() ) - ->method( 'injectLater' ) - ->will( $this->returnCallback( function( $id, $text ) use ( &$modalInjection ) { - $modalInjection .= $text; - } ) ); $parserOutputHelper->expects( $this->any() ) ->method( 'renderErrorMessage' ) ->will( $this->returnArgument( 0 ) ); @@ -74,11 +68,7 @@ public function testCanRender( $input, $arguments, $expectedTriggerOutput, $expe /** @noinspection PhpParamsInspection */ $generatedOutput = $instance->parseComponent( $parserRequest ); - $this->assertEquals( $expectedTriggerOutput, $generatedOutput ); - $this->assertEquals( - $expectedModalOutput, - $modalInjection - ); + $this->assertEquals( $expectedTriggerOutput . $expectedModalOutput, $generatedOutput ); } /** diff --git a/tests/phpunit/Unit/Hooks/OutputPageParserOutputTest.php b/tests/phpunit/Unit/Hooks/OutputPageParserOutputTest.php index e7d97fb..99f4bd6 100644 --- a/tests/phpunit/Unit/Hooks/OutputPageParserOutputTest.php +++ b/tests/phpunit/Unit/Hooks/OutputPageParserOutputTest.php @@ -39,40 +39,43 @@ public function testCanConstruct() { ); } - public function testHookOutputPageParserOutput() { - $content = 'CONTENT'; - + public function testHookOutputPageParserOutputLoadsVectorFixUnderVector() { $outputPage = $this->createMock( OutputPage::class ); - $outputPage->expects( $this->once() ) - ->method( 'addHTML' ) - ->will( $this->returnCallback( function( $injection ) use ( &$content ) { - $content .= $injection; - } ) ); + $outputPage->expects( $this->never() )->method( 'addHTML' ); $outputPage->expects( $this->once() ) ->method( 'addModules' ) ->with( $this->equalTo( [ 'ext.bootstrapComponents.vector-fix' ] ) ); - $observerParserOutput = $this->createMock( ParserOutput::class ); - $observerParserOutput->expects( $this->exactly( 1 ) ) - ->method( 'getExtensionData' ) - ->with( - $this->stringContains( 'bsc_deferredContent' ) - ) - ->willReturn( [ 'test' ] ); - $bootstrapService = $this->createMock( BootstrapComponentsService::class ); $bootstrapService->expects( $this->once() ) ->method( 'vectorSkinInUse' ) ->willReturn( true ); - $instance = new OutputPageParserOutput( $outputPage, $observerParserOutput, $bootstrapService ); + $instance = new OutputPageParserOutput( + $outputPage, + $this->createMock( ParserOutput::class ), + $bootstrapService + ); $instance->process(); + } - $this->assertEquals( - 'CONTENTtest', - $content + public function testHookDoesNothingWhenNotVector() { + $outputPage = $this->createMock( OutputPage::class ); + $outputPage->expects( $this->never() )->method( 'addHTML' ); + $outputPage->expects( $this->never() )->method( 'addModules' ); + + $bootstrapService = $this->createMock( BootstrapComponentsService::class ); + $bootstrapService->expects( $this->once() ) + ->method( 'vectorSkinInUse' ) + ->willReturn( false ); + + $instance = new OutputPageParserOutput( + $outputPage, + $this->createMock( ParserOutput::class ), + $bootstrapService ); + $instance->process(); } } diff --git a/tests/phpunit/Unit/ImageModalTest.php b/tests/phpunit/Unit/ImageModalTest.php index a4ef194..b103247 100644 --- a/tests/phpunit/Unit/ImageModalTest.php +++ b/tests/phpunit/Unit/ImageModalTest.php @@ -254,13 +254,7 @@ function( $component ) { } ) ); - $modalInjection = ''; $parserOutputHelper = $this->createMock( ParserOutputHelper::class ); - $parserOutputHelper->expects( $this->any() ) - ->method( 'injectLater' ) - ->will( $this->returnCallback( function( $id, $text ) use ( &$modalInjection ) { - $modalInjection .= $text; - } ) ); $instance = $this->createImageModalWithMocks( null, $title, $file, $nestingController, null, $parserOutputHelper ); $time = false; @@ -283,9 +277,9 @@ function( $component ) { . '-- ' . ($resultOfParseCall ?: $res) ); } - $this->assertEquals( + $this->assertStringContainsString( $expectedModal, - $modalInjection, + $resultOfParseCall ?: $res, 'failed modal with test data:' . $this->generatePhpCodeForManualProviderDataOneCase( $fp, $hp ) ); } diff --git a/tests/phpunit/Unit/ModalBuilderTest.php b/tests/phpunit/Unit/ModalBuilderTest.php index c03893f..e31e284 100644 --- a/tests/phpunit/Unit/ModalBuilderTest.php +++ b/tests/phpunit/Unit/ModalBuilderTest.php @@ -49,15 +49,9 @@ public function testCanConstruct() { */ public function testCanParse( $id, $trigger, $content, $header, $footer, $outerClass, $outerStyle, $innerClass, $expectedTrigger, $expectedModal ) { - $modalInjection = ''; $parserOutputHelper = $this->getMockBuilder( 'MediaWiki\\Extension\\BootstrapComponents\\ParserOutputHelper' ) ->disableOriginalConstructor() ->getMock(); - $parserOutputHelper->expects( $this->any() ) - ->method( 'injectLater' ) - ->will( $this->returnCallback( function( $id, $text ) use ( &$modalInjection ) { - $modalInjection .= $text; - } ) ); /** @noinspection PhpParamsInspection */ $instance = new ModalBuilder( $id, $trigger, $content, $parserOutputHelper ); @@ -77,13 +71,9 @@ public function testCanParse( $id, $trigger, $content, $header, $footer, $outerC $instance->setDialogClass( $innerClass ); } $this->assertEquals( - $expectedTrigger, + $expectedTrigger . $expectedModal, $instance->parse() ); - $this->assertEquals( - $expectedModal, - $modalInjection - ); } /**