Hooks: further limit where CodeMirror RL modules are loaded

This fixes a preexisting issue where we were loading CodeMirror on pages where it would never be used. We use the EditPage__showEditForm_initial hook so we don't need to check the action. This commit introduces the CodeMirrorContentModels extension attribute, used to limit where CodeMirror is loaded automatically. By default, this includes only CONTENT_MODEL_WIKITEXT ('wikitext'). This extension attribute serves as a stepping stone to CM being used on content types other than just wikitext. Bug: T359206 Change-Id: Ibefc028c5ef6275393202fe773c26162715e1bca
2024-11-23 13:56:44 +00:00 · 2024-02-29 19:36:39 -05:00 · 2024-02-29 19:36:39 -05:00 · 894d2c33e9
parent e578597263
commit 894d2c33e9
3 changed files with 77 additions and 41 deletions
--- a/extension.json
+++ b/extension.json
@ -249,7 +249,7 @@
 		]
 	},
 	"Hooks": {
-		"BeforePageDisplay": "main",
+		"EditPage::showEditForm:initial": "main",
 		"GetPreferences": "main",
 		"ResourceLoaderGetConfigVars": "main"
 	},
@ -264,6 +264,9 @@
 	},
 	"attributes": {
 		"CodeMirror": {
+			"ContentModels": [
+				"wikitext"
+			],
 			"PluginModules": [
 				"ext.CodeMirror.addons"
 			],
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@ -5,17 +5,20 @@ namespace MediaWiki\Extension\CodeMirror;
 use ExtensionRegistry;
 use InvalidArgumentException;
 use MediaWiki\Config\Config;
+use MediaWiki\EditPage\EditPage;
 use MediaWiki\Extension\Gadgets\GadgetRepo;
-use MediaWiki\Hook\BeforePageDisplayHook;
+use MediaWiki\Hook\EditPage__showEditForm_initialHook;
 use MediaWiki\Output\OutputPage;
 use MediaWiki\Preferences\Hook\GetPreferencesHook;
 use MediaWiki\ResourceLoader\Hook\ResourceLoaderGetConfigVarsHook;
 use MediaWiki\User\Options\UserOptionsLookup;
 use MediaWiki\User\User;
-use Skin;

+/**
+ * @phpcs:disable MediaWiki.NamingConventions.LowerCamelFunctionsName.FunctionName
+ */
 class Hooks implements
-	BeforePageDisplayHook,
+	EditPage__showEditForm_initialHook,
 	ResourceLoaderGetConfigVarsHook,
 	GetPreferencesHook
 {
@ -45,7 +48,6 @@ class Hooks implements
 	 * @return bool
 	 */
 	public function shouldLoadCodeMirror( OutputPage $out, ?ExtensionRegistry $extensionRegistry = null ): bool {
-		$extensionRegistry = $extensionRegistry ?: ExtensionRegistry::getInstance();
 		// Disable CodeMirror when CodeEditor is active on this page
 		// Depends on ext.codeEditor being added by \MediaWiki\EditPage\EditPage::showEditForm:initial
 		if ( in_array( 'ext.codeEditor', $out->getModules(), true ) ) {
@ -55,12 +57,16 @@ class Hooks implements
 		if ( !$this->userOptionsLookup->getOption( $out->getUser(), 'usebetatoolbar' ) ) {
 			return false;
 		}
+		$extensionRegistry = $extensionRegistry ?: ExtensionRegistry::getInstance();
+		$contentModels = $extensionRegistry->getAttribute( 'CodeMirrorContentModels' );
 		$isRTL = $out->getTitle()->getPageLanguage()->isRTL();
-		return in_array( $out->getActionName(), [ 'edit', 'submit' ], true ) &&
-			// Disable CodeMirror if we're on an edit page with a conflicting gadget. See T178348.
-			!$this->conflictingGadgetsEnabled( $extensionRegistry, $out->getUser() ) &&
+		// Disable CodeMirror if we're on an edit page with a conflicting gadget. See T178348.
+		return !$this->conflictingGadgetsEnabled( $extensionRegistry, $out->getUser() ) &&
 			// CodeMirror 5 on textarea wikitext editors doesn't support RTL (T170001)
-			( !$isRTL || $this->shouldUseV6( $out ) );
+			( !$isRTL || $this->shouldUseV6( $out ) ) &&
+			// Limit to supported content models that use wikitext.
+			// See https://www.mediawiki.org/wiki/Content_handlers#Extension_content_handlers
+			in_array( $out->getTitle()->getContentModel(), $contentModels );
 	}

 	/**
@ -89,15 +95,14 @@ class Hooks implements
 	}

 	/**
-	 * BeforePageDisplay hook handler
+	 * Load CodeMirror if necessary.
 	 *
-	 * @see https://www.mediawiki.org/wiki/Manual:Hooks/BeforePageDisplay
+	 * @see https://www.mediawiki.org/wiki/Manual:Hooks/EditPage::showEditForm:initial
 	 *
+	 * @param EditPage $editor
 	 * @param OutputPage $out
-	 * @param Skin $skin
-	 * @return void This hook must not abort, it must return no value
 	 */
-	public function onBeforePageDisplay( $out, $skin ): void {
+	public function onEditPage__showEditForm_initial( $editor, $out ): void {
 		if ( !$this->shouldLoadCodeMirror( $out ) ) {
 			return;
 		}
@ -163,5 +168,4 @@ class Hooks implements
 			'section' => 'editing/accessibility',
 		];
 	}
-
 }
--- a/tests/phpunit/HookTest.php
+++ b/tests/phpunit/HookTest.php
@ -3,7 +3,10 @@
 namespace MediaWiki\Extension\CodeMirror\Tests;

 use ExtensionRegistry;
+use Generator;
+use Language;
 use MediaWiki\Context\RequestContext;
+use MediaWiki\EditPage\EditPage;
 use MediaWiki\Extension\CodeMirror\Hooks;
 use MediaWiki\Extension\Gadgets\Gadget;
 use MediaWiki\Extension\Gadgets\GadgetRepo;
@ -14,7 +17,6 @@ use MediaWiki\User\Options\UserOptionsLookup;
 use MediaWiki\User\User;
 use MediaWikiIntegrationTestCase;
 use PHPUnit\Framework\MockObject\MockObject;
-use Skin;

 /**
 * @group CodeMirror
@ -25,14 +27,14 @@ class HookTest extends MediaWikiIntegrationTestCase {

 	/**
 	 * @covers ::shouldLoadCodeMirror
-	 * @covers ::onBeforePageDisplay
+	 * @covers ::onEditPage__showEditForm_initial
 	 * @param bool $useCodeMirrorV6
 	 * @param int $expectedAddModuleCalls
-	 * @param string $expectedFirstModule
-	 * @dataProvider provideOnBeforePageDisplay
+	 * @param string|null $expectedFirstModule
+	 * @dataProvider provideOnEditPageShowEditFormInitial
 	 */
-	public function testOnBeforePageDisplay(
-		bool $useCodeMirrorV6, int $expectedAddModuleCalls, string $expectedFirstModule
+	public function testOnEditPageShowEditFormInitial(
+		bool $useCodeMirrorV6, int $expectedAddModuleCalls, ?string $expectedFirstModule
 	) {
 		$this->overrideConfigValues( [
 			'CodeMirrorV6' => $useCodeMirrorV6,
@ -46,24 +48,23 @@ class HookTest extends MediaWikiIntegrationTestCase {
 		$out->expects( $this->exactly( $expectedAddModuleCalls ) )
 			->method( 'addModules' )
 			->willReturnCallback( function ( $modules ) use ( $expectedFirstModule, &$isFirstCall ) {
-				if ( $isFirstCall ) {
+				if ( $isFirstCall && $modules !== null ) {
 					$this->assertSame( $expectedFirstModule, $modules );
 				}
 				$isFirstCall = false;
 			} );

-		( new Hooks( $userOptionsLookup, $this->getServiceContainer()->getMainConfig() ) )
-			->onBeforePageDisplay( $out, $this->createMock( Skin::class ) );
+		$hooks = new Hooks( $userOptionsLookup, $this->getServiceContainer()->getMainConfig() );
+		$hooks->onEditPage__showEditForm_initial( $this->createMock( EditPage::class ), $out );
 	}

 	/**
-	 * @return array[]
+	 * @return Generator
 	 */
-	public function provideOnBeforePageDisplay(): array {
-		return [
-			[ false, 2, 'ext.CodeMirror.WikiEditor' ],
-			[ true, 1, 'ext.CodeMirror.v6.WikiEditor' ]
-		];
+	public static function provideOnEditPageShowEditFormInitial(): Generator {
+		// useCodeMirrorV6, expectedAddModuleCalls, expectedFirstModule
+		yield 'CM5' => [ false, 2, 'ext.CodeMirror.WikiEditor' ];
+		yield 'CM6' => [ true, 1, 'ext.CodeMirror.v6.WikiEditor' ];
 	}

 	/**
@ -81,9 +82,25 @@ class HookTest extends MediaWikiIntegrationTestCase {
 	/**
 	 * @covers ::shouldLoadCodeMirror
 	 * @dataProvider provideShouldLoadCodeMirror
+	 * @param string|null $module
+	 * @param string|null $gadget
+	 * @param bool $expectation
+	 * @param string $contentModel
+	 * @param bool $useCodeMirrorV6
+	 * @param bool $isRTL
 	 */
-	public function testShouldLoadCodeMirror( ?string $module, ?string $gadget, bool $expectation ): void {
-		$out = $this->getMockOutputPage();
+	public function testShouldLoadCodeMirror(
+		?string $module,
+		?string $gadget,
+		bool $expectation,
+		string $contentModel = CONTENT_MODEL_WIKITEXT,
+		bool $useCodeMirrorV6 = false,
+		bool $isRTL = false
+	): void {
+		$this->overrideConfigValues( [
+			'CodeMirrorV6' => $useCodeMirrorV6,
+		] );
+		$out = $this->getMockOutputPage( $contentModel, $isRTL );
 		$out->method( 'getModules' )->willReturn( $module ? [ $module ] : [] );
 		$userOptionsLookup = $this->createMock( UserOptionsLookup::class );
 		$userOptionsLookup->method( 'getOption' )->willReturn( true );
@ -93,6 +110,9 @@ class HookTest extends MediaWikiIntegrationTestCase {
 		}

 		$extensionRegistry = $this->getMockExtensionRegistry( (bool)$gadget );
+		$extensionRegistry->method( 'getAttribute' )
+			->with( 'CodeMirrorContentModels' )
+			->willReturn( [ CONTENT_MODEL_WIKITEXT ] );

 		if ( $gadget ) {
 			$gadgetMock = $this->createMock( Gadget::class );
@ -117,24 +137,33 @@ class HookTest extends MediaWikiIntegrationTestCase {
 	}

 	/**
-	 * @return array[]
+	 * @return Generator
 	 */
-	public function provideShouldLoadCodeMirror(): array {
-		return [
-			[ null, null, true ],
-			[ 'ext.codeEditor', null, false ],
-			[ null, 'wikEd', false ]
-		];
+	public function provideShouldLoadCodeMirror(): Generator {
+		// module, gadget, expectation, contentModel, shouldUseV6, isRTL
+		yield 'no modules, no gadgets, wikitext' => [ null, null, true ];
+		yield 'codeEditor, no gadgets, wikitext' => [ 'ext.codeEditor', null, false ];
+		yield 'no modules, wikEd, wikitext' => [ null, 'wikEd', false ];
+		yield 'no modules, no gadgets, CSS' => [ null, null, false, CONTENT_MODEL_CSS ];
+		yield 'CM5 wikitext RTL' => [ null, null, false, CONTENT_MODEL_WIKITEXT, false, true ];
+		yield 'CM6 wikitext RTL' => [ null, null, true, CONTENT_MODEL_WIKITEXT, true, true ];
 	}

 	/**
+	 * @param string $contentModel
+	 * @param bool $isRTL
 	 * @return OutputPage|MockObject
 	 */
-	private function getMockOutputPage() {
+	private function getMockOutputPage( string $contentModel = CONTENT_MODEL_WIKITEXT, bool $isRTL = false ) {
 		$out = $this->createMock( OutputPage::class );
 		$out->method( 'getUser' )->willReturn( $this->createMock( User::class ) );
 		$out->method( 'getActionName' )->willReturn( 'edit' );
-		$out->method( 'getTitle' )->willReturn( Title::makeTitle( NS_MAIN, __METHOD__ ) );
+		$title = $this->createMock( Title::class );
+		$title->method( 'getContentModel' )->willReturn( $contentModel );
+		$language = $this->createMock( Language::class );
+		$language->method( 'isRTL' )->willReturn( $isRTL );
+		$title->method( 'getPageLanguage' )->willReturn( $language );
+		$out->method( 'getTitle' )->willReturn( $title );
 		$request = $this->createMock( WebRequest::class );
 		$request->method( 'getRawVal' )->willReturn( null );
 		$out->method( 'getRequest' )->willReturn( $request );