From d2a37f5f69f19ea7d7b7a85f377d411a4ba0f017 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 26 Mar 2024 14:06:26 -0700 Subject: [PATCH] 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 --- ...chema.json => GadgetDefinition.schema.json | 0 extension.json | 5 +- i18n/en.json | 2 +- i18n/qqq.json | 2 +- includes/CodeEditorHooks.php | 3 +- includes/Content/GadgetDefinitionContent.php | 7 + .../GadgetDefinitionContentHandler.php | 16 +++ .../Content/GadgetDefinitionValidator.php | 127 ++++++++++-------- includes/GadgetRepo.php | 10 +- includes/Hooks.php | 38 ------ 10 files changed, 105 insertions(+), 105 deletions(-) rename includes/Content/schema.json => GadgetDefinition.schema.json (100%) diff --git a/includes/Content/schema.json b/GadgetDefinition.schema.json similarity index 100% rename from includes/Content/schema.json rename to GadgetDefinition.schema.json diff --git a/extension.json b/extension.json index cc3b4465..b20d83f9 100644 --- a/extension.json +++ b/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", diff --git a/i18n/en.json b/i18n/en.json index 2a3aaaed..6e6e898e 100644 --- a/i18n/en.json +++ b/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 $1 is not set.", - "gadgets-validate-wrongtype": "The property $1 must be $2 instead of $3.", + "gadgets-validate-wrongtype": "The property $1 must be of type $2.", "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.", diff --git a/i18n/qqq.json b/i18n/qqq.json index 131c77a8..0fe055eb 100644 --- a/i18n/qqq.json +++ b/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. settings.rights or module.messages[3].\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. settings.rights or module.messages[3].\n* $2 - the expected JSON type for this property, e.g. array or string", "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.", diff --git a/includes/CodeEditorHooks.php b/includes/CodeEditorHooks.php index a8ccac8e..f4ab3efb 100644 --- a/includes/CodeEditorHooks.php +++ b/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 { diff --git a/includes/Content/GadgetDefinitionContent.php b/includes/Content/GadgetDefinitionContent.php index 24d02b3f..e652915b 100644 --- a/includes/Content/GadgetDefinitionContent.php +++ b/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(); diff --git a/includes/Content/GadgetDefinitionContentHandler.php b/includes/Content/GadgetDefinitionContentHandler.php index bca95fed..44fcf0cf 100644 --- a/includes/Content/GadgetDefinitionContentHandler.php +++ b/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' => [ diff --git a/includes/Content/GadgetDefinitionValidator.php b/includes/Content/GadgetDefinitionValidator.php index 35113b46..102004f0 100644 --- a/includes/Content/GadgetDefinitionValidator.php +++ b/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'] ); } } diff --git a/includes/GadgetRepo.php b/includes/GadgetRepo.php index 367d56de..1668bb4b 100644 --- a/includes/GadgetRepo.php +++ b/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 { diff --git a/includes/Hooks.php b/includes/Hooks.php index 6f388d3d..6c6ce80c 100644 --- a/includes/Hooks.php +++ b/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/.json" pages with GadgetDefinitionContent *