Skip to content

Commit eb2ceda

Browse files
malbertsclaude
andcommitted
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 <noreply@anthropic.com>
1 parent 68dde37 commit eb2ceda

9 files changed

Lines changed: 41 additions & 123 deletions

‎src/ApplicationFactory.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ public function getNewAttributeManager( array $validAttributes, array $aliases )
107107
* @param string $id
108108
* @param string $trigger must be safe raw html (best run through {@see Parser::recursiveTagParse})
109109
* @param string $content must be safe raw html (best run through {@see Parser::recursiveTagParse})
110-
* @param ParserOutputHelper $parserOutputHelper
110+
* @param ParserOutputHelper $parserOutputHelper @deprecated unused since the inline-emission modal fix; will be removed in the next major release.
111111
*
112112
* @see ModalBuilder::__construct
113113
*

‎src/BootstrapComponents.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,6 @@
5656
*/
5757
class BootstrapComponents {
5858

59-
const EXTENSION_DATA_DEFERRED_CONTENT_KEY = 'bsc_deferredContent';
60-
6159
const EXTENSION_DATA_NO_IMAGE_MODAL = 'bsc_no_image_modal';
6260

6361

‎src/Hooks/OutputPageParserOutput.php‎

Lines changed: 5 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626

2727
namespace MediaWiki\Extension\BootstrapComponents\Hooks;
2828

29-
use MediaWiki\Extension\BootstrapComponents\BootstrapComponents;
3029
use MediaWiki\Extension\BootstrapComponents\BootstrapComponentsService;
3130
/*
3231
* TODO switch to these, wehen we drop support for mw < 1.40
@@ -41,25 +40,12 @@
4140
*
4241
* Called after parse, before the HTML is added to the output.
4342
*
44-
* Method delegated to separate class to fix missing (deferred) content in
45-
* {@see \MediaWiki\Extension\BootstrapComponents\Tests\Integration\BootstrapComponentsJSONScriptTestCaseRunnerTest::assertParserOutputForCase}
46-
*
4743
* @see https://www.mediawiki.org/wiki/Manual:Hooks/OutputPageParserOutput
4844
*
4945
* @since 1.2
5046
*/
5147
class OutputPageParserOutput {
5248

53-
/**
54-
* @var string
55-
*/
56-
const INJECTION_PREFIX = '<!-- injected by Extension:BootstrapComponents -->';
57-
58-
/**
59-
* @var string
60-
*/
61-
const INJECTION_SUFFIX = '<!-- /injected by Extension:BootstrapComponents -->';
62-
6349
/**
6450
* @var BootstrapComponentsService
6551
*/
@@ -72,14 +58,16 @@ class OutputPageParserOutput {
7258

7359
/**
7460
* @var ParserOutput $parserOutput
61+
*
62+
* @deprecated unused since the inline-emission modal fix; will be removed in the next major release.
7563
*/
7664
private ParserOutput $parserOutput;
7765

7866
/**
7967
* OutputPageParserOutput constructor.
8068
*
8169
* @param OutputPage $outputPage
82-
* @param ParserOutput $parserOutput
70+
* @param ParserOutput $parserOutput @deprecated unused since the inline-emission modal fix; will be removed in the next major release.
8371
* @param BootstrapComponentsService $service
8472
*/
8573
public function __construct(
@@ -94,36 +82,11 @@ public function __construct(
9482
* @return void
9583
*/
9684
public function process(): void {
97-
$deferredText = $this->getContentForLaterInjection( $this->getParserOutput() );
98-
if ( !empty( $deferredText ) ) {
99-
$this->getOutputPage()->addHTML( $deferredText );
100-
}
101-
10285
if ( $this->getBootstrapComponentsService()->vectorSkinInUse() ) {
10386
$this->getOutputPage()->addModules( [ 'ext.bootstrapComponents.vector-fix' ] );
10487
}
10588
}
10689

107-
/**
108-
* Returns the raw html that is to be inserted at the end of the page.
109-
*
110-
* @param ParserOutput $parserOutput
111-
*
112-
* @return string
113-
*/
114-
protected function getContentForLaterInjection( ParserOutput $parserOutput ): string {
115-
$deferredContent = $parserOutput
116-
->getExtensionData(BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY );
117-
118-
if ( empty( $deferredContent ) || !is_array( $deferredContent ) ) {
119-
return '';
120-
}
121-
122-
// clearing extension data for unit and integration tests to work
123-
$parserOutput->setExtensionData( BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY, null );
124-
return self::INJECTION_PREFIX . implode( array_values( $deferredContent ) ) . self::INJECTION_SUFFIX;
125-
}
126-
12790
protected function getBootstrapComponentsService(): BootstrapComponentsService {
12891
return $this->bootstrapComponentService;
12992
}
@@ -136,6 +99,8 @@ protected function getOutputPage(): OutputPage {
13699
}
137100

138101
/**
102+
* @deprecated unused since the inline-emission modal fix; will be removed in the next major release.
103+
*
139104
* @return ParserOutput
140105
*/
141106
protected function getParserOutput(): ParserOutput {

‎src/ModalBuilder.php‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ class ModalBuilder {
102102

103103
/**
104104
* @var ParserOutputHelper $parserOutputHelper
105+
*
106+
* @deprecated unused since the inline-emission modal fix; will be removed in the next major release.
105107
*/
106108
private $parserOutputHelper;
107109

@@ -146,7 +148,7 @@ public static function wrapTriggerElement( $element, $id ) {
146148
* @param string $id
147149
* @param string $trigger must be safe raw html (best run through {@see Parser::recursiveTagParse})
148150
* @param string $content must be fully parsed html (use {@see Parser::recursiveTagParseFully})
149-
* @param ParserOutputHelper $parserOutputHelper
151+
* @param ParserOutputHelper $parserOutputHelper @deprecated unused since the inline-emission modal fix; will be removed in the next major release.
150152
*
151153
* @see ApplicationFactory::getNewModalBuilder
152154
* @see Components\Modal::generateButton
@@ -163,14 +165,14 @@ public function __construct( $id, $trigger, $content, $parserOutputHelper ) {
163165
/**
164166
* Parses the modal.
165167
*
168+
* Emits the trigger followed by the modal container inline. The container is
169+
* `position: fixed` and is addressed via `data-target="#id"`, so its DOM
170+
* placement is not significant.
171+
*
166172
* @return string
167173
*/
168174
public function parse() {
169-
$this->parserOutputHelper->injectLater(
170-
$this->getId(),
171-
$this->buildModal()
172-
);
173-
return $this->buildTrigger();
175+
return $this->buildTrigger() . $this->buildModal();
174176
}
175177

176178
/**

‎src/ParserOutputHelper.php‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -138,30 +138,6 @@ public function areImageModalsSuppressed() {
138138
->getExtensionData( BootstrapComponents::EXTENSION_DATA_NO_IMAGE_MODAL );
139139
}
140140

141-
/**
142-
* 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.
143-
*
144-
* @param string $id
145-
* @param string $rawHtml
146-
*
147-
* @return ParserOutputHelper $this (fluid)
148-
*/
149-
public function injectLater( $id, $rawHtml ) {
150-
if ( empty( $this->getParser()->getOutput() ) ) {
151-
# fix issues with PageForms file upload (issue #20)
152-
return $this;
153-
}
154-
if ( !empty( $rawHtml ) ) {
155-
$deferredContent = $this->getParser()->getOutput()->getExtensionData( BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY );
156-
if ( empty( $deferredContent ) ) {
157-
$deferredContent = [];
158-
}
159-
$deferredContent[$id] = $rawHtml;
160-
$this->getParser()->getOutput()->setExtensionData( BootstrapComponents::EXTENSION_DATA_DEFERRED_CONTENT_KEY, $deferredContent );
161-
}
162-
return $this;
163-
}
164-
165141
/**
166142
* Formats a text as error text, so it can be added to the output.
167143
*

‎tests/phpunit/Unit/Components/ModalTest.php‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -49,15 +49,9 @@ public function testCanConstruct() {
4949
*/
5050
public function testCanRender( $input, $arguments, $expectedTriggerOutput, $expectedModalOutput ) {
5151

52-
$modalInjection = '';
5352
$parserOutputHelper = $this->getMockBuilder( 'MediaWiki\\Extension\\BootstrapComponents\\ParserOutputHelper' )
5453
->disableOriginalConstructor()
5554
->getMock();
56-
$parserOutputHelper->expects( $this->any() )
57-
->method( 'injectLater' )
58-
->will( $this->returnCallback( function( $id, $text ) use ( &$modalInjection ) {
59-
$modalInjection .= $text;
60-
} ) );
6155
$parserOutputHelper->expects( $this->any() )
6256
->method( 'renderErrorMessage' )
6357
->will( $this->returnArgument( 0 ) );
@@ -74,11 +68,7 @@ public function testCanRender( $input, $arguments, $expectedTriggerOutput, $expe
7468
/** @noinspection PhpParamsInspection */
7569
$generatedOutput = $instance->parseComponent( $parserRequest );
7670

77-
$this->assertEquals( $expectedTriggerOutput, $generatedOutput );
78-
$this->assertEquals(
79-
$expectedModalOutput,
80-
$modalInjection
81-
);
71+
$this->assertEquals( $expectedTriggerOutput . $expectedModalOutput, $generatedOutput );
8272
}
8373

8474
/**

‎tests/phpunit/Unit/Hooks/OutputPageParserOutputTest.php‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,40 +39,43 @@ public function testCanConstruct() {
3939
);
4040
}
4141

42-
public function testHookOutputPageParserOutput() {
43-
$content = 'CONTENT';
44-
42+
public function testHookOutputPageParserOutputLoadsVectorFixUnderVector() {
4543
$outputPage = $this->createMock( OutputPage::class );
46-
$outputPage->expects( $this->once() )
47-
->method( 'addHTML' )
48-
->will( $this->returnCallback( function( $injection ) use ( &$content ) {
49-
$content .= $injection;
50-
} ) );
44+
$outputPage->expects( $this->never() )->method( 'addHTML' );
5145
$outputPage->expects( $this->once() )
5246
->method( 'addModules' )
5347
->with(
5448
$this->equalTo( [ 'ext.bootstrapComponents.vector-fix' ] )
5549
);
5650

57-
$observerParserOutput = $this->createMock( ParserOutput::class );
58-
$observerParserOutput->expects( $this->exactly( 1 ) )
59-
->method( 'getExtensionData' )
60-
->with(
61-
$this->stringContains( 'bsc_deferredContent' )
62-
)
63-
->willReturn( [ 'test' ] );
64-
6551
$bootstrapService = $this->createMock( BootstrapComponentsService::class );
6652
$bootstrapService->expects( $this->once() )
6753
->method( 'vectorSkinInUse' )
6854
->willReturn( true );
6955

70-
$instance = new OutputPageParserOutput( $outputPage, $observerParserOutput, $bootstrapService );
56+
$instance = new OutputPageParserOutput(
57+
$outputPage,
58+
$this->createMock( ParserOutput::class ),
59+
$bootstrapService
60+
);
7161
$instance->process();
62+
}
7263

73-
$this->assertEquals(
74-
'CONTENT<!-- injected by Extension:BootstrapComponents -->test<!-- /injected by Extension:BootstrapComponents -->',
75-
$content
64+
public function testHookDoesNothingWhenNotVector() {
65+
$outputPage = $this->createMock( OutputPage::class );
66+
$outputPage->expects( $this->never() )->method( 'addHTML' );
67+
$outputPage->expects( $this->never() )->method( 'addModules' );
68+
69+
$bootstrapService = $this->createMock( BootstrapComponentsService::class );
70+
$bootstrapService->expects( $this->once() )
71+
->method( 'vectorSkinInUse' )
72+
->willReturn( false );
73+
74+
$instance = new OutputPageParserOutput(
75+
$outputPage,
76+
$this->createMock( ParserOutput::class ),
77+
$bootstrapService
7678
);
79+
$instance->process();
7780
}
7881
}

‎tests/phpunit/Unit/ImageModalTest.php‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -254,13 +254,7 @@ function( $component ) {
254254
}
255255
) );
256256

257-
$modalInjection = '';
258257
$parserOutputHelper = $this->createMock( ParserOutputHelper::class );
259-
$parserOutputHelper->expects( $this->any() )
260-
->method( 'injectLater' )
261-
->will( $this->returnCallback( function( $id, $text ) use ( &$modalInjection ) {
262-
$modalInjection .= $text;
263-
} ) );
264258

265259
$instance = $this->createImageModalWithMocks( null, $title, $file, $nestingController, null, $parserOutputHelper );
266260
$time = false;
@@ -283,9 +277,9 @@ function( $component ) {
283277
. '-- ' . ($resultOfParseCall ?: $res)
284278
);
285279
}
286-
$this->assertEquals(
280+
$this->assertStringContainsString(
287281
$expectedModal,
288-
$modalInjection,
282+
$resultOfParseCall ?: $res,
289283
'failed modal with test data:' . $this->generatePhpCodeForManualProviderDataOneCase( $fp, $hp )
290284
);
291285
}

‎tests/phpunit/Unit/ModalBuilderTest.php‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -49,15 +49,9 @@ public function testCanConstruct() {
4949
*/
5050
public function testCanParse( $id, $trigger, $content, $header, $footer, $outerClass, $outerStyle, $innerClass, $expectedTrigger, $expectedModal ) {
5151

52-
$modalInjection = '';
5352
$parserOutputHelper = $this->getMockBuilder( 'MediaWiki\\Extension\\BootstrapComponents\\ParserOutputHelper' )
5453
->disableOriginalConstructor()
5554
->getMock();
56-
$parserOutputHelper->expects( $this->any() )
57-
->method( 'injectLater' )
58-
->will( $this->returnCallback( function( $id, $text ) use ( &$modalInjection ) {
59-
$modalInjection .= $text;
60-
} ) );
6155

6256
/** @noinspection PhpParamsInspection */
6357
$instance = new ModalBuilder( $id, $trigger, $content, $parserOutputHelper );
@@ -77,13 +71,9 @@ public function testCanParse( $id, $trigger, $content, $header, $footer, $outerC
7771
$instance->setDialogClass( $innerClass );
7872
}
7973
$this->assertEquals(
80-
$expectedTrigger,
74+
$expectedTrigger . $expectedModal,
8175
$instance->parse()
8276
);
83-
$this->assertEquals(
84-
$expectedModal,
85-
$modalInjection
86-
);
8777
}
8878

8979
/**

0 commit comments

Comments
 (0)