Merge "Replace EditFilterMergedContent hook with ContentHandler override"

extension.json

@@ -2,7 +2,9 @@
 	"name": "Gadgets",
 	"author": [
 		"Daniel Kinzler",
-		"Max Semenik"
+		"Max Semenik",
+		"Timo Tijhof",
+		"Siddharth VP"
 	],
 	"url": "https://www.mediawiki.org/wiki/Extension:Gadgets",
 	"descriptionmsg": "gadgets-desc",
@@ -84,7 +86,6 @@
 		"BeforePageDisplay": "GadgetHooks",
 		"CodeEditorGetPageLanguage": "GadgetCodeEditorHooks",
 		"ContentHandlerDefaultModelFor": "GadgetHooks",
-		"EditFilterMergedContent": "GadgetHooks",
 		"UserGetDefaultOptions": "GadgetHooks",
 		"GetPreferences": "GadgetHooks",
 		"PreferencesGetLegend": "GadgetHooks",

i18n/en.json

@@ -40,7 +40,7 @@
 	"gadgets-export-download": "Download",
 	"gadgets-requires-es6": "This gadget is only supported on ES6-compliant browsers",
 	"gadgets-validate-notset": "The property <code>$1</code> is not set.",
-	"gadgets-validate-wrongtype": "The property <code>$1</code> must be <code>$2</code> instead of <code>$3</code>.",
+	"gadgets-validate-wrongtype": "The property <code>$1</code> must be of type <code>$2</code>.",
 	"gadgets-validate-json": "JSON files are specified but not used. They are only valid in packaged gadgets.",
 	"gadgets-validate-es6default": "Gadgets requiring ES6 cannot be enabled by default.",
 	"gadgets-validate-noentrypoint": "Package flag ignored as no script files are specified.",

i18n/qqq.json

@@ -55,7 +55,7 @@
 	"gadgets-export-download": "Use the verb for this message. Submit button.\n{{Identical|Download}}",
 	"gadgets-requires-es6": "Message shown on [[Special:Gadgets]] for gadgets that only run on browsers that support ES6 (ES6 is a version of the JavaScript programming language).",
 	"gadgets-validate-notset": "Error message shown if a required property is not set. Parameters:\n* $1 - name of the property, e.g. settings.rights .",
-	"gadgets-validate-wrongtype": "Error message shown if a property is set to the wrong type.\n* $1 is the name of the property, e.g. <code>settings.rights</code> or <code>module.messages[3]</code>.\n* $2 is the type that this property is expected to have\n* $3 is the type it actually had",
+	"gadgets-validate-wrongtype": "Error message shown if a property is set to the wrong type. Parameters:\n* $1 - name of the property, e.g. <code>settings.rights</code> or <code>module.messages[3]</code>.\n* $2 - the expected JSON type for this property, e.g. <code>array</code> or <code>string</code>",
 	"gadgets-validate-json": "Warning message to indicate that JSON files cannot be used as they are only valid in package gadgets",
 	"gadgets-validate-es6default": "Warning message to indicate that gadget requiring ES6 cannot be default.",
 	"gadgets-validate-noentrypoint": "Warning message to indicate that package flag will be ignored as no script files are specified.",

includes/CodeEditorHooks.php

@@ -6,8 +6,7 @@ use MediaWiki\Extension\CodeEditor\Hooks\CodeEditorGetPageLanguageHook;
 use MediaWiki\Title\Title;
 
 /**
- * Hooks from CodeEditor extension,
- * which is optional to use with this extension.
+ * Hooks for optional integration with the CodeEditor extension.
  */
 class CodeEditorHooks implements CodeEditorGetPageLanguageHook {
 

includes/Content/GadgetDefinitionContent.php

@@ -54,12 +54,19 @@ class GadgetDefinitionContent extends JsonContent {
 	}
 
 	/**
+	 * Helper for isValid
+	 *
+	 * This placed into a separate method so that the detailed error can be communicated
+	 * to editors via GadgetDefinitionContentHandler::validateSave, instead of the generic
+	 * 'invalid-content-data' message from ContentHandler::validateSave based on isValid.
+	 *
 	 * @return Status
 	 */
 	public function validate() {
 		// Cache the validation result to avoid re-computations
 		if ( !$this->validation ) {
 			if ( !parent::isValid() ) {
+				// Invalid JSON, use the detailed Status from JsonContent::getData for syntax errors.
 				$this->validation = $this->getData();
 			} else {
 				$validator = new GadgetDefinitionValidator();

includes/Content/GadgetDefinitionContentHandler.php

@@ -24,11 +24,13 @@ use Content;
 use FormatJson;
 use JsonContentHandler;
 use MediaWiki\Content\Renderer\ContentParseParams;
+use MediaWiki\Content\ValidationParams;
 use MediaWiki\Extension\Gadgets\GadgetRepo;
 use MediaWiki\Extension\Gadgets\MediaWikiGadgetsJsonRepo;
 use MediaWiki\Linker\Linker;
 use MediaWiki\Parser\ParserOutput;
 use MediaWiki\Title\Title;
+use StatusValue;
 
 class GadgetDefinitionContentHandler extends JsonContentHandler {
 	private GadgetRepo $gadgetRepo;
@@ -56,6 +58,20 @@ class GadgetDefinitionContentHandler extends JsonContentHandler {
 		return new $class( FormatJson::encode( $this->getEmptyDefinition(), "\t" ) );
 	}
 
+	/**
+	 * @param Content $content
+	 * @param ValidationParams $validationParams
+	 * @return StatusValue
+	 */
+	public function validateSave( Content $content, ValidationParams $validationParams ) {
+		$status = parent::validateSave( $content, $validationParams );
+		'@phan-var GadgetDefinitionContent $content';
+		if ( !$status->isOK() ) {
+			return $content->validate();
+		}
+		return $status;
+	}
+
 	public function getEmptyDefinition() {
 		return [
 			'settings' => [

includes/Content/GadgetDefinitionValidator.php

@@ -5,61 +5,77 @@ namespace MediaWiki\Extension\Gadgets\Content;
 use MediaWiki\Status\Status;
 
 /**
- * Class responsible for validating Gadget definition contents
+ * Validate the content of a gadget definition page.
  *
- * @todo maybe this should use a formal JSON schema validator or something
+ * @see MediaWikiGadgetsJsonRepo
+ * @see GadgetDefinitionContent
+ * @internal
  */
 class GadgetDefinitionValidator {
-	/**
-	 * @var array Validation metadata.
-	 * 'foo.bar.baz' => [ 'type check callback',
-	 *   'type name' [, 'member type check callback', 'member type name'] ]
-	 */
-	protected static $propertyValidation = [
-		'settings' => [ 'is_array', 'array' ],
-		'settings.actions' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'settings.categories' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'settings.category' => [ 'is_string', 'string' ],
-		'settings.contentModels' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'settings.default' => [ 'is_bool', 'boolean' ],
-		'settings.hidden' => [ 'is_bool', 'boolean' ],
-		'settings.namespaces' => [ 'is_array', 'array', 'is_int', 'integer' ],
-		'settings.package' => [ 'is_bool', 'boolean' ],
-		'settings.requiresES6' => [ 'is_bool', 'boolean' ],
-		'settings.rights' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'settings.skins' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'settings.supportsUrlLoad' => [ 'is_bool', 'boolean' ],
-
-		'module' => [ 'is_array', 'array' ],
-		'module.dependencies' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'module.messages' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'module.pages' => [ 'is_array', 'array', [ __CLASS__, 'isValidTitleSuffix' ], '.js, .css or .json page' ],
-		'module.peers' => [ 'is_array', 'array', 'is_string', 'string' ],
-		'module.type' => [ [ __CLASS__, 'isValidType' ], 'general or styles' ],
+	private const TYPE_ARRAY = [ 'callback' => 'is_array', 'expect' => 'array' ];
+	private const TYPE_BOOL = [ 'callback' => 'is_bool', 'expect' => 'boolean' ];
+	private const TYPE_INT = [ 'callback' => 'is_int', 'expect' => 'number' ];
+	private const TYPE_STRING = [ 'callback' => 'is_string', 'expect' => 'string' ];
+	private const TYPE_PAGE_SUFFIX = [
+		'callback' => [ __CLASS__, 'isResourcePageSuffix' ], 'expect' => '.js, .css, .json'
+	];
+	private const TYPE_MODULE_TYPE = [
+		'callback' => [ __CLASS__, 'isModuleType' ], 'expect' => '"", "general", "styles"',
 	];
 
-	public static function isValidTitleSuffix( string $title ): bool {
-		return str_ends_with( $title, '.js' ) || str_ends_with( $title, '.css' ) || str_ends_with( $title, '.json' );
+	/**
+	 * @var array Schema for the definition.
+	 *
+	 * - callback: boolean check.
+	 * - expect: human-readable description for the gadgets-validate-wrongtype message.
+	 * - child: optional type+expect for each array item.
+	 */
+	protected static $schema = [
+		'settings' => self::TYPE_ARRAY,
+		'settings.actions' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'settings.categories' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'settings.category' => self::TYPE_STRING,
+		'settings.contentModels' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'settings.default' => self::TYPE_BOOL,
+		'settings.hidden' => self::TYPE_BOOL,
+		'settings.namespaces' => self::TYPE_ARRAY + [ 'child' => self::TYPE_INT ],
+		'settings.package' => self::TYPE_BOOL,
+		'settings.requiresES6' => self::TYPE_BOOL,
+		'settings.rights' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'settings.skins' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'settings.supportsUrlLoad' => self::TYPE_BOOL,
+
+		'module' => self::TYPE_ARRAY,
+		'module.dependencies' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'module.messages' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'module.pages' => self::TYPE_ARRAY + [ 'child' => self::TYPE_PAGE_SUFFIX ],
+		'module.peers' => self::TYPE_ARRAY + [ 'child' => self::TYPE_STRING ],
+		'module.type' => self::TYPE_MODULE_TYPE,
+	];
+
+	public static function isResourcePageSuffix( $title ): bool {
+		return is_string( $title ) && (
+			str_ends_with( $title, '.js' ) || str_ends_with( $title, '.css' ) || str_ends_with( $title, '.json' )
+		);
 	}
 
-	public static function isValidType( string $type ): bool {
+	public static function isModuleType( string $type ): bool {
 		return $type === '' || $type === 'general' || $type === 'styles';
 	}
 
 	/**
-	 * Check the validity of the given properties array
-	 * @param array $properties Return value of FormatJson::decode( $blob, true )
+	 * Check the validity of known properties in a gadget definition array.
+	 *
+	 * @param array $definition
 	 * @param bool $tolerateMissing If true, don't complain about missing keys
 	 * @return Status object with error message if applicable
 	 */
-	public function validate( array $properties, $tolerateMissing = false ) {
-		foreach ( self::$propertyValidation as $property => $validation ) {
-			$path = explode( '.', $property );
-			$val = $properties;
-
-			// Walk down and verify that the path from the root to this property exists
-			foreach ( $path as $p ) {
-				if ( !array_key_exists( $p, $val ) ) {
+	public function validate( array $definition, $tolerateMissing = false ) {
+		foreach ( self::$schema as $property => $validation ) {
+			// Access the property by walking from the root to the specified property
+			$val = $definition;
+			foreach ( explode( '.', $property ) as $propName ) {
+				if ( !array_key_exists( $propName, $val ) ) {
 					if ( $tolerateMissing ) {
 						// Skip validation of this property altogether
 						continue 2;
@@ -67,30 +83,27 @@ class GadgetDefinitionValidator {
 
 					return Status::newFatal( 'gadgets-validate-notset', $property );
 				}
-				$val = $val[$p];
+				$val = $val[$propName];
 			}
 
-			// Do the actual validation of this property
-			$func = $validation[0];
-			if ( !call_user_func( $func, $val ) ) {
+			// Validate this property
+			$isValid = ( $validation['callback'] )( $val );
+			if ( !$isValid ) {
 				return Status::newFatal(
-					'gadgets-validate-wrongtype',
-					$property,
-					$validation[1],
-					gettype( $val )
+					'gadgets-validate-wrongtype', $property,
+					$validation['expect']
 				);
 			}
 
-			if ( isset( $validation[2] ) && isset( $validation[3] ) && is_array( $val ) ) {
-				// Descend into the array and check the type of each element
-				$func = $validation[2];
-				foreach ( $val as $i => $v ) {
-					if ( !call_user_func( $func, $v ) ) {
+			// Descend into the array and validate each array item
+			if ( isset( $validation['child'] ) && is_array( $val ) ) {
+				foreach ( $val as $key => $item ) {
+					$isValid = ( $validation['child']['callback'] )( $item );
+					if ( !$isValid ) {
 						return Status::newFatal(
 							'gadgets-validate-wrongtype',
-							"{$property}[{$i}]",
-							$validation[3],
-							gettype( $v )
+							"{$property}[{$key}]",
+							$validation['child']['expect']
 						);
 					}
 				}

includes/GadgetRepo.php

@@ -90,7 +90,8 @@ abstract class GadgetRepo {
 	 * @return string
 	 */
 	public function titleWithoutPrefix( string $titleText, string $gadgetId ): string {
-		$numReplaces = 1; // there will only one occurrence of the prefix
+		// there is only one occurrence of the prefix
+		$numReplaces = 1;
 		return str_replace( self::RESOURCE_TITLE_PREFIX, '', $titleText, $numReplaces );
 	}
 
@@ -138,10 +139,11 @@ abstract class GadgetRepo {
 	}
 
 	/**
-	 * Check titles used in gadget to verify existence and correct content model.
-	 * @param array $pages
+	 * Verify gadget resource pages exist and use the correct content model.
+	 *
+	 * @param string[] $pages Full page names
 	 * @param string $expectedContentModel
-	 * @param string $msg
+	 * @param string $msg Interface message key
 	 * @return Message[]
 	 */
 	private function checkTitles( array $pages, string $expectedContentModel, string $msg ): array {

includes/Hooks.php

@@ -23,17 +23,12 @@
 namespace MediaWiki\Extension\Gadgets;
 
 use ApiMessage;
-use Content;
-use Exception;
 use HTMLForm;
-use IContextSource;
 use InvalidArgumentException;
 use ManualLogEntry;
-use MediaWiki\Extension\Gadgets\Content\GadgetDefinitionContent;
 use MediaWiki\Extension\Gadgets\Special\SpecialGadgetUsage;
 use MediaWiki\Hook\BeforePageDisplayHook;
 use MediaWiki\Hook\DeleteUnknownPreferencesHook;
-use MediaWiki\Hook\EditFilterMergedContentHook;
 use MediaWiki\Hook\PreferencesGetIconHook;
 use MediaWiki\Hook\PreferencesGetLegendHook;
 use MediaWiki\Html\Html;
@@ -49,7 +44,6 @@ use MediaWiki\Revision\Hook\ContentHandlerDefaultModelForHook;
 use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\SpecialPage\Hook\WgQueryPagesHook;
 use MediaWiki\SpecialPage\SpecialPage;
-use MediaWiki\Status\Status;
 use MediaWiki\Storage\Hook\PageSaveCompleteHook;
 use MediaWiki\Title\Title;
 use MediaWiki\Title\TitleValue;
@@ -75,7 +69,6 @@ class Hooks implements
 	PreferencesGetLegendHook,
 	ResourceLoaderRegisterModulesHook,
 	BeforePageDisplayHook,
-	EditFilterMergedContentHook,
 	ContentHandlerDefaultModelForHook,
 	WgQueryPagesHook,
 	DeleteUnknownPreferencesHook,
@@ -337,37 +330,6 @@ class Hooks implements
 		);
 	}
 
-	/**
-	 * Valid gadget definition page after content is modified
-	 *
-	 * @param IContextSource $context
-	 * @param Content $content
-	 * @param Status $status
-	 * @param string $summary
-	 * @param User $user
-	 * @param bool $minoredit
-	 * @throws Exception
-	 * @return bool
-	 */
-	public function onEditFilterMergedContent(
-		IContextSource $context,
-		Content $content,
-		Status $status,
-		$summary,
-		User $user,
-		$minoredit
-	) {
-		if ( $content instanceof GadgetDefinitionContent ) {
-			$validateStatus = $content->validate();
-			if ( !$validateStatus->isGood() ) {
-				$status->merge( $validateStatus );
-				return false;
-			}
-		}
-
-		return true;
-	}
-
 	/**
 	 * Create "MediaWiki:Gadgets/<id>.json" pages with GadgetDefinitionContent
 	 *