jenkins-bot has submitted this change and it was merged.
Change subject: Require Set#params to be non-empty and improve error
......................................................................
Require Set#params to be non-empty and improve error
* An empty array for a Set is invalid and the implementation now
enforces this.
* Add new error message for invalid value. When Sets contain a
reference to a key that doesn't exist, it should say that that
reference in the Set is invalid instead of saying that Params
is invalid for not having it.
> { params: {}, sets: [ { params: [ "quux" ] } ] }
- Required property "params.quux" not found.
+ Invalid value for property "sets.0.params[1]".
Also:
* Abstracted handling of $case and the different assertions
on it in a helper function so that we don't have to repeat
it everywhere.
* Use the same hack as in other tests to display the status error
message in the phpunit output (true === isGood ?: getHtml).
Maybe use '' === getHtml instead, though that is also evil
since Status fatals if you call getMessage/getHtml for a good
Status object.
Change-Id: I06a6615f728cd287a4839e09eedc2d0eeb537949
---
M TemplateData.i18n.php
M TemplateDataBlob.php
M spec.templatedata.json
M tests/TemplateDataBlobTest.php
4 files changed, 51 insertions(+), 30 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/TemplateData.i18n.php b/TemplateData.i18n.php
index 9fa3703..0baf2ab 100644
--- a/TemplateData.i18n.php
+++ b/TemplateData.i18n.php
@@ -27,6 +27,7 @@
'templatedata-invalid-parse' => 'Syntax error in JSON.',
'templatedata-invalid-type' => 'Property "$1" is expected to be of type
"$2".',
'templatedata-invalid-missing' => 'Required property "$1" not found.',
+ 'templatedata-invalid-empty-array' => 'Property "$1" must have at least
one value in its array.',
'templatedata-invalid-unknown' => 'Unexpected property "$1".',
'templatedata-invalid-value' => 'Invalid value for property "$1".',
'templatedata-invalid-length' => 'Data too large to save
({{formatnum:$1}} {{PLURAL:$1|byte|bytes}}, {{PLURAL:$2|limit is}}
{{formatnum:$2}})',
@@ -76,6 +77,7 @@
'templatedata-invalid-missing' => 'Error message when a required
property is not found.
* $1 - name of property. e.g. "params"
* $2 - type of property (Unused)',
+ 'templatedata-invalid-empty-array' => 'Error message when a property
that must be non-empty is empty.',
'templatedata-invalid-unknown' => 'Error message when an unknown
property is found.
* $1 - name of property. e.g. "params.1.foobar"',
'templatedata-invalid-value' => 'Error message when a property that
cannot contain free-form text has an invalid value.
diff --git a/TemplateDataBlob.php b/TemplateDataBlob.php
index 0534011..6ddcdcb 100644
--- a/TemplateDataBlob.php
+++ b/TemplateDataBlob.php
@@ -290,7 +290,7 @@
foreach ( $data->sets as $setNr => $setObj ) {
if ( !is_object( $setObj ) ) {
- return Status::newFatal(
'templatedata-invalid-type', "sets.{$setNr}", 'object' );
+ return Status::newFatal(
'templatedata-invalid-value', "paramOrder[$i]" );
}
if ( !isset( $setObj->label ) ) {
@@ -320,9 +320,13 @@
return Status::newFatal(
'templatedata-invalid-type', "sets.{$setNr}.params", 'array' );
}
- foreach ( $setObj->params as $param ) {
+ if ( !count( $setObj->params ) ) {
+ return Status::newFatal(
'templatedata-invalid-empty-array', "sets.{$setNr}.params" );
+ }
+
+ foreach ( $setObj->params as $i => $param ) {
if ( !isset( $data->params->$param ) ) {
- return Status::newFatal(
'templatedata-invalid-missing', "params.{$param}" );
+ return Status::newFatal(
'templatedata-invalid-value', "sets.{$setNr}.params[$i]" );
}
}
}
diff --git a/spec.templatedata.json b/spec.templatedata.json
index 26e8cf3..3a59bf7 100644
--- a/spec.templatedata.json
+++ b/spec.templatedata.json
@@ -32,7 +32,7 @@
@structure {Object} Set
@property {InterfaceText} label Label of this set.
- @property {Array} params A subset of the parameter's names that belong
to this set.
+ @property {Array} params One or more parameter keys.
@structure {string} Type
One of the following:
diff --git a/tests/TemplateDataBlobTest.php b/tests/TemplateDataBlobTest.php
index 4648790..513bc95 100644
--- a/tests/TemplateDataBlobTest.php
+++ b/tests/TemplateDataBlobTest.php
@@ -253,7 +253,7 @@
}
]
}',
- 'status' => 'Required property "params.quux"
not found.'
+ 'status' => 'Invalid value for property
"sets.0.params[1]".'
),
array(
'input' => '{
@@ -340,47 +340,50 @@
return $calls;
}
- /**
- * @dataProvider provideParse
- */
- public function testParse( Array $cases ) {
-
+ protected function assertTemplateData( Array $case ) {
// Expand defaults
- if ( !isset( $cases['status'] ) ) {
- $cases['status'] = true;
+ if ( !isset( $case['status'] ) ) {
+ $case['status'] = true;
}
- if ( !isset( $cases['msg'] ) ) {
- $cases['msg'] = is_string( $cases['status'] ) ?
$cases['status'] : 'TemplateData assertion';
+ if ( !isset( $case['msg'] ) ) {
+ $case['msg'] = is_string( $case['status'] ) ?
$case['status'] : 'TemplateData assertion';
}
- if ( !isset( $cases['output'] ) ) {
- if ( is_string( $cases['status'] ) ) {
- $cases['output'] = '{}';
+ if ( !isset( $case['output'] ) ) {
+ if ( is_string( $case['status'] ) ) {
+ $case['output'] = '{}';
} else {
- $cases['output'] = $cases['input'];
+ $case['output'] = $case['input'];
}
}
- $t = TemplateDataBlob::newFromJSON( $cases['input'] );
+ $t = TemplateDataBlob::newFromJSON( $case['input'] );
$actual = $t->getJSON();
$status = $t->getStatus();
if ( !$status->isGood() ) {
$this->assertEquals(
- $cases['status'],
+ $case['status'],
$status->getHtml(),
- 'Status: ' . $cases['msg']
+ 'Status: ' . $case['msg']
);
} else {
$this->assertEquals(
- $cases['status'],
+ $case['status'],
$status->isGood(),
- 'Status: ' . $cases['msg']
+ 'Status: ' . $case['msg']
);
}
$this->assertJsonStringEqualsJsonString(
- $cases['output'],
+ $case['output'],
$actual,
- $cases['msg']
+ $case['msg']
);
+ }
+
+ /**
+ * @dataProvider provideParse
+ */
+ public function testParse( Array $case ) {
+ $this->assertTemplateData( $case );
}
/**
@@ -556,25 +559,37 @@
),
array(
'input' => '{
- "params": {},
+ "params": {
+ "foo": {}
+ },
"sets": [
{
"label": {
"es": "Spanish",
"de": "German"
},
- "params": []
+ "params": ["foo"]
}
]
}
',
'output' => '{
"description": null,
- "params": {},
+ "params": {
+ "foo": {
+ "label": null,
+ "required": false,
+ "description": null,
+ "deprecated": false,
+ "aliases": [],
+ "default": "",
+ "type": "unknown"
+ }
+ },
"sets": [
{
"label": "Spanish",
- "params": []
+ "params": ["foo"]
}
]
}
@@ -609,7 +624,7 @@
$t = TemplateDataBlob::newFromJSON( $case['input'] );
$status = $t->getStatus();
- $this->assertTrue( $status->isGood(), 'Status is good: ' .
$case['msg'] );
+ $this->assertTrue( $status->isGood() ? : $status->getHtml(),
'Status is good: ' . $case['msg'] );
$actual = $t->getDataInLanguage( $case['lang'] );
$this->assertJsonStringEqualsJsonString(
--
To view, visit https://gerrit.wikimedia.org/r/90127
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I06a6615f728cd287a4839e09eedc2d0eeb537949
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/extensions/TemplateData
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
Gerrit-Reviewer: Bartosz Dziewoński <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Jforrester <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Siebrand <[email protected]>
Gerrit-Reviewer: Trevor Parscal <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
