From def6645cdcaeec7db891ccacf84cd7c44241becf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Massart?= Date: Mon, 19 Dec 2022 15:45:30 +0800 Subject: [PATCH 1/4] MDL-76723 core_external: Test invalid required value for format fields --- lib/tests/exporter_test.php | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/tests/exporter_test.php b/lib/tests/exporter_test.php index bbcf5b43331..edb1e5ae93f 100644 --- a/lib/tests/exporter_test.php +++ b/lib/tests/exporter_test.php @@ -202,6 +202,26 @@ class exporter_test extends \advanced_testcase { // Assert nested elements are formatted correctly. $this->assertEquals('id', $properties['nestedarray']['type']['id']['description']); } + + public function test_format_properties_with_optional() { + $testable = new class([]) extends \core\external\exporter { + public static function define_properties() { + return [ + 'content' => [ + 'type' => PARAM_RAW, + 'optional' => true, + ], + 'contentformat' => [ + 'type' => PARAM_INT, + 'optional' => true, + ] + ]; + } + }; + $definition = $testable::get_read_structure(); + $this->assertEquals(VALUE_OPTIONAL, $definition->keys['content']->required); + $this->assertEquals(VALUE_OPTIONAL, $definition->keys['contentformat']->required); + } } /** From bbfbc8abd1cef2a40be022d2123ab5ae23339aaf Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Wed, 22 Nov 2023 23:39:23 +0800 Subject: [PATCH 2/4] MDL-76723 core_external: Handle format properties properly - If a text format property defines the 'optional' attribute as true, then the $required parameter that is passed to \core\external\exporter::get_format_structure() should be VALUE_OPTIONAL. - If a default value is provided for a format property, \core\external\exporter::get_format_structure() should pass the default value to the external_format_value constructor. - Added validation for external_format_value for the default parameter. Debugging will be shown in case an invalid default value is passed. - Amended unit tests for better coverage. --- lib/classes/external/exporter.php | 11 +- .../classes/external_format_value.php | 9 +- lib/tests/exporter_test.php | 102 +++++++++++++++--- 3 files changed, 105 insertions(+), 17 deletions(-) diff --git a/lib/classes/external/exporter.php b/lib/classes/external/exporter.php index bfaa6ef0918..71cd67e9277 100644 --- a/lib/classes/external/exporter.php +++ b/lib/classes/external/exporter.php @@ -425,10 +425,12 @@ abstract class exporter { * @return external_format_value */ final protected static function get_format_structure($property, $definition, $required = VALUE_REQUIRED) { + $default = null; if (array_key_exists('default', $definition)) { $required = VALUE_DEFAULT; + $default = $definition['default']; } - return new external_format_value($property, $required); + return new external_format_value($property, $required, $default); } /** @@ -547,7 +549,12 @@ abstract class exporter { if (isset($returns[$formatproperty])) { throw new coding_exception('The format for \'' . $property . '\' is already defined.'); } - $returns[$formatproperty] = self::get_format_structure($property, $properties[$formatproperty]); + $formatpropertydef = $properties[$formatproperty]; + $formatpropertyrequired = VALUE_REQUIRED; + if (!empty($formatpropertydef['optional'])) { + $formatpropertyrequired = VALUE_OPTIONAL; + } + $returns[$formatproperty] = self::get_format_structure($property, $formatpropertydef, $formatpropertyrequired); } } } diff --git a/lib/external/classes/external_format_value.php b/lib/external/classes/external_format_value.php index e13c68e1120..71610aa0520 100644 --- a/lib/external/classes/external_format_value.php +++ b/lib/external/classes/external_format_value.php @@ -38,12 +38,19 @@ class external_format_value extends external_value { * @param int $default Default value. */ public function __construct($textfieldname, $required = VALUE_REQUIRED, $default = null) { + // Make sure the default format's value is correct. + if ($default !== null && !in_array($default, [FORMAT_MOODLE, FORMAT_HTML, FORMAT_PLAIN, FORMAT_MARKDOWN])) { + debugging("Invalid default format for $textfieldname: $default. " . + "It must be either FORMAT_MOODLE, FORMAT_HTML, FORMAT_PLAIN, or FORMAT_MARKDOWN.", DEBUG_DEVELOPER); + $default = null; + } + if ($default == null && $required == VALUE_DEFAULT) { $default = FORMAT_HTML; } $desc = sprintf( - "%s format (%s = HTML, %s = MOODLE, %s = PLAIN, or %s = MARKDOWN", + "%s format (%s = HTML, %s = MOODLE, %s = PLAIN, or %s = MARKDOWN)", $textfieldname, FORMAT_HTML, FORMAT_MOODLE, diff --git a/lib/tests/exporter_test.php b/lib/tests/exporter_test.php index edb1e5ae93f..baed4f8de69 100644 --- a/lib/tests/exporter_test.php +++ b/lib/tests/exporter_test.php @@ -203,24 +203,98 @@ class exporter_test extends \advanced_testcase { $this->assertEquals('id', $properties['nestedarray']['type']['id']['description']); } - public function test_format_properties_with_optional() { + /** + * Test the processing of format properties. + * + * @covers \core\external\exporter::get_read_structure + * @return void + */ + public function test_format_properties_with_optional(): void { $testable = new class([]) extends \core\external\exporter { - public static function define_properties() { - return [ - 'content' => [ - 'type' => PARAM_RAW, - 'optional' => true, - ], - 'contentformat' => [ - 'type' => PARAM_INT, - 'optional' => true, - ] - ]; - } + /** + * Properties definition. + * + * @return array[] + */ + public static function define_properties(): array { + return [ + 'content' => [ + 'type' => PARAM_RAW, + ], + 'contentformat' => [ + 'type' => PARAM_INT, + 'optional' => true, + ], + 'description' => [ + 'type' => PARAM_RAW, + 'optional' => true, + ], + 'descriptionformat' => [ + 'type' => PARAM_INT, + 'default' => FORMAT_MARKDOWN, + ], + 'summary' => [ + 'type' => PARAM_RAW, + ], + 'summaryformat' => [ + 'type' => PARAM_INT, + 'default' => null, + ], + ]; + } }; + $definition = $testable::get_read_structure(); - $this->assertEquals(VALUE_OPTIONAL, $definition->keys['content']->required); + // Check content and its format. + $this->assertEquals(VALUE_REQUIRED, $definition->keys['content']->required); $this->assertEquals(VALUE_OPTIONAL, $definition->keys['contentformat']->required); + $this->assertEquals(null, $definition->keys['contentformat']->default); + + // Check description and its format. + $this->assertEquals(VALUE_OPTIONAL, $definition->keys['description']->required); + $this->assertEquals(VALUE_DEFAULT, $definition->keys['descriptionformat']->required); + $this->assertEquals(FORMAT_MARKDOWN, $definition->keys['descriptionformat']->default); + + // Check summary and its format. + $this->assertEquals(VALUE_REQUIRED, $definition->keys['summary']->required); + $this->assertEquals(null, $definition->keys['summary']->default); + $this->assertEquals(VALUE_DEFAULT, $definition->keys['summaryformat']->required); + $this->assertEquals(FORMAT_HTML, $definition->keys['summaryformat']->default); + } + + /** + * Test the processing of format properties when an invalid default format is passed. + * + * @covers \core\external\exporter::get_read_structure + * @return void + */ + public function test_optional_format_property_with_invalid_default(): void { + $testable = new class([]) extends \core\external\exporter { + /** + * Properties definition. + * + * @return array[] + */ + public static function define_properties(): array { + return [ + 'description' => [ + 'type' => PARAM_RAW, + ], + 'descriptionformat' => [ + 'type' => PARAM_INT, + 'default' => 999, + ], + ]; + } + }; + + $definition = $testable::get_read_structure(); + $this->assertDebuggingCalled(null, DEBUG_DEVELOPER); + + // Check description and its format. + $this->assertEquals(VALUE_REQUIRED, $definition->keys['description']->required); + $this->assertEquals(VALUE_DEFAULT, $definition->keys['descriptionformat']->required); + $this->assertEquals(FORMAT_HTML, $definition->keys['descriptionformat']->default); } } From 77a5658f0e903d99852897feacf7234418066513 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Thu, 23 Nov 2023 16:02:02 +0800 Subject: [PATCH 3/4] MDL-76723 mod_page: Use external_format_value constructor properly * 'Content format' is passed to the $default parameter which is incorrect. * Also removed passing of VALUE_REQUIRED because it's already the default value for the $required parameter. --- mod/page/classes/external.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/page/classes/external.php b/mod/page/classes/external.php index 9f948ca601f..036a5086ee9 100644 --- a/mod/page/classes/external.php +++ b/mod/page/classes/external.php @@ -193,7 +193,7 @@ class mod_page_external extends external_api { helper_for_get_mods_by_courses::standard_coursemodule_elements_returns(), [ 'content' => new external_value(PARAM_RAW, 'Page content'), - 'contentformat' => new external_format_value('content', VALUE_REQUIRED, 'Content format'), + 'contentformat' => new external_format_value('content'), 'contentfiles' => new external_files('Files in the content'), 'legacyfiles' => new external_value(PARAM_INT, 'Legacy files flag'), 'legacyfileslast' => new external_value(PARAM_INT, 'Legacy files last control flag'), From 9d291854a562ddb848ba26747ea2ee4dfe9623b6 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Thu, 23 Nov 2023 16:03:39 +0800 Subject: [PATCH 4/4] MDL-76723 core_tag: Use external_format_value constructor properly * First parameter should be the field name that the format property points to. * No need for VALUE_REQUIRED since it's the default for the $required parameter. * Third parameter is the default value and not a description string. --- tag/classes/external.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tag/classes/external.php b/tag/classes/external.php index 06b6c96e21b..23267605de7 100644 --- a/tag/classes/external.php +++ b/tag/classes/external.php @@ -242,7 +242,7 @@ class core_tag_external extends external_api { 'name' => new external_value(PARAM_TAG, 'name'), 'rawname' => new external_value(PARAM_RAW, 'tag raw name (may contain capital letters)'), 'description' => new external_value(PARAM_RAW, 'tag description'), - 'descriptionformat' => new external_format_value(PARAM_INT, VALUE_REQUIRED, 'tag description format'), + 'descriptionformat' => new external_format_value('description'), 'flag' => new external_value(PARAM_INT, 'flag', VALUE_OPTIONAL), 'official' => new external_value(PARAM_INT, 'whether this flag is standard (deprecated, use isstandard)', VALUE_OPTIONAL),