-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for "const" #507
Add support for "const" #507
Conversation
9a3d12a
to
6d04026
Compare
'{ | ||
"type":"object", | ||
"properties":{ | ||
"value":{"type":"string","const":"bar"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little curious about the potential use cases covered by this implementation. Ten minutes ago I had never heard of the const
keyword, but from what I've read since then its main utility comes from interacting with the $data
concept proposed in v5. Given that we don't currently support $data
or relative JSON pointers, how much use is there in supporting a keyword that guarantees that an input value is precisely equal to some other value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find any mention of the $data
concept in the recent spec v7. From my understanding, const
is presumably nothing more than a more expressive alternative to a one-element enum
:
6.1.3. const
The value of this keyword MAY be of any type, including null.An instance validates successfully against this keyword if its value is equal to the value of the keyword.
My use case for const
is when using a "constant" property as a type discriminator when using polymorphic types:
{
"$oneOf": [
{
"properties": {
"paymentType": {"type": "string", "const": "directDebit"},
"iban": {"type": ...}
}
},
{
"properties": {
"paymentType": {"type: "string", "const": "creditCard"},
"cardNumber": {...}
}
]
}
In this case, documents like {"payment": {"paymentType": "directDebit", "iban": "DE1234567890"}}
and {"payment": {"paymentType": "creditCard", "cardNumber": "123456789"}}
would match the schema, while something like {"payment": {"paymentType": "creditCard", "iban": "DE1234567890"}}
would not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Okay. 👍
Does this play nicely with coercion? It would be great if you could add a section somewhere that does a few checks (possibly in |
@shmax Done; I've added a few test cases on how I think Btw, I noticed that the |
tests/Constraints/CoerciveTest.php
Outdated
'integer', 'string', "42", true | ||
); | ||
|
||
// #46 check coercion with "const" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// #47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Egad, it's been that way for a year! Nice catch, thanks much. Not to be a pest, but can you add one more coercive test to check boolean type? Just to be sure--php is fine with |
👍 Done |
); | ||
|
||
// #50 check boolean coercion with "const" | ||
$tests[] = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to see a false...
("true" == true) // true
("false" == true) // true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful, thank you
|
||
if ($type === gettype($const)) { | ||
if ($type == 'object') { | ||
if ($element == $const) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not correctly implement deep comparison of objects - it results in members being compared using ==
.
Could you please change this implementation to ensure type-strict comparison of object properties. You will probably need to iterate them; PHP lacks a suitable native comparison operator that can do it all in one go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following that argumentation, the enum
implementation (which was my inspiration for this class) should probably be changed as well -- it uses the same comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If enum is similarly broken, then yes - I agree with you that it needs fixing.
'{"properties":{"propertyOne":{"type":"boolean","const":true}}}', | ||
'{"propertyOne":1}', | ||
'integer', 'boolean', true, true | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add some deep object tests. With and without mismatched property types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Will defer to @erayd and @bighappyface for version target considerations.
@shmax I reckon 6.0.0 (which this PR is against), and I'll also backport it for 5.3.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bighappyface I notice that you have merged this, without a fix for the deep object comparison issue I raised above. Are you intending for that to be fixed in a separate PR? @martin-helmich seems to have abandoned this one without making the necessary change. |
Is that normal the version 5.2.8 and 5.X.X do not have this feature ?! |
@Raphy That is correct, yes. This is a draft-06 feature, but is currently incomplete, and not backported. This library officially supports draft-03 and draft-04, but does not yet fully support newer versions of the spec. Draft-06 is largely similar to draft-04, but there are a few differences, and this is one of them. |
As discussed in #506, this PR adds support for the
const
keyword as first specified in section 6.24 of draft 06.Fixes #506