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 *