Replace EditFilterMergedContent hook with ContentHandler override

The reason for this hook is not the validation itself, because that
is already done by `GadgetDefinitionContent->isValid` which is part
of the core Content interface, already enforced by ContentHandler.

Instead, the hook was here to provide the custom interface message
GadgetDefinitionValidator, because the core Content interface is
limited to boolean isValid(), which provides a very generic error
message.

However, nowadays ContentHandler exposes this mechanism directly
such that we can directly attach a custom message to it without
needing to wait for the stack to reach the EditPage and then override
it after the fact from a global hook.

Also:

* Simplify validation logic towards "is" checks with only an
  expected description.

* Move schema.json file to top-level file.
  It has been unused for as long as it has been in the repo, despite
  appearing (due to its placement) to be used as part of the source.
  It was added, I believe, with the intent to be used by the validator,
  but it isn't. It also isn't validated or checked for correctness
  by anything right now.
  For now, keep it as informal schema in the top-level location for
  easy discovery where perhaps others can find a use for it.
  SD0001 mentions gadget developers may want to start using it for
  Git-maintained gadgets to help with validation in their IDE, after
  Gadgets 2.0 is launched.

Test Plan:
* Set `$wgGadgetsRepo = 'json+definition';`
* Create `MediaWiki:Gadgets/example.json`
* Attempt to save "x" in settings.namespaces item.
* Attempt to save "x.zip" in module.pages item.
* Fails with this patch, similar as on master.

Bug: T31272
Change-Id: I61bc3e40348a0aeb3bd3fa9ca86ccb7b93304095

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;
@@ -76,7 +70,6 @@ class Hooks implements
 	PreferencesGetLegendHook,
 	ResourceLoaderRegisterModulesHook,
 	BeforePageDisplayHook,
-	EditFilterMergedContentHook,
 	ContentHandlerDefaultModelForHook,
 	WgQueryPagesHook,
 	DeleteUnknownPreferencesHook,
@@ -338,37 +331,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
 	 *