Skip to content
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

Strict Enum/Const Object Checking #518

Merged
merged 7 commits into from
Jun 11, 2018
Merged

Strict Enum/Const Object Checking #518

merged 7 commits into from
Jun 11, 2018

Conversation

shmax
Copy link
Collaborator

@shmax shmax commented May 29, 2018

Fix for #517

@erayd correctly deduced that the non-strict object comparisons happening in EnumConstraint and ConstConstraint were opening type comparison holes on their member values. For example, the following test would pass:

array(
	'{
		"value": {
			"foo": "12"
		}
	}',
	'{
		"type": "object",
		"properties": {
			"value": {
				"type": "any", 
				"enum": [
					6, 
					"foo", 
					[], 
					true, 
					{
						"foo": 12
					}
				]
			}
		}
	}'
)

PHP rules governing object type equality checking are somewhat infamous. From the docs:

When using the comparison operator (==), object variables are compared in a simple manner, namely: Two object instances are equal if they have the same attributes and values (values are compared with ==), and are instances of the same class.

When using the identity operator (===), object variables are identical if and only if they refer to the same instance of the same class.

It provides no mechanism for doing strict member-by-member comparisons, so for this PR I drew in a library dedicated to this kind of thing, Parity.

@shmax
Copy link
Collaborator Author

shmax commented May 29, 2018

@erayd @martin-helmich please review

@erayd
Copy link
Contributor

erayd commented May 29, 2018

Needs a code style fix, but otherwise looks good 👍. Thanks!

Copy link
Contributor

@erayd erayd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to comply with code style rules.

@shmax
Copy link
Collaborator Author

shmax commented May 29, 2018

@erayd I took care of the linting, but I don't know what's going on with the hhvm build; I don't think it's anything to do with my changes.

@@ -53,7 +53,7 @@ public function addError(ConstraintError $constraint, JsonPointer $path = null,
'pointer' => ltrim(strval($path ?: new JsonPointer('')), '#'),
'message' => ucfirst(vsprintf($message, array_map(function ($val) {
if (is_scalar($val)) {
return $val;
return var_export($val, true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra credit: I noticed that the const error string was producing this message for bool type:

Does not have a value equal to 1

instead of

Does not have a value equal to true

@shmax
Copy link
Collaborator Author

shmax commented Jun 5, 2018

So what do we do with this?

@erayd
Copy link
Contributor

erayd commented Jun 5, 2018

@shmax I don't think the hhvm build is related, but it needs fixing. I'm happy for that to be in a different PR. However, the failing tests on all builds introduced in your latest commit do need fixing.

@shmax
Copy link
Collaborator Author

shmax commented Jun 5, 2018

Oh, heh, didn't realize the other tests had broken. Thanks much.

Copy link
Contributor

@erayd erayd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks 👍

@shmax
Copy link
Collaborator Author

shmax commented Jun 11, 2018

No prob. Anything else you want me to take a look at?

@bighappyface bighappyface merged commit 505b5e7 into jsonrainbow:master Jun 11, 2018
@shmax shmax deleted the enum-const-fix branch June 11, 2018 18:07
@erayd erayd mentioned this pull request Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants