Moving constraints min- and maxProperties to ObjectConstraint

[rtucek]

As requested in #224, I've moved the min-/maxProperties constraint from UndefinedConstraint to ObjectConstraint.

[bighappyface]

I think we can leave this in to maintain better BC

[bighappyface]

@rtucek thanks for the improvement!

I have verified that the test suite runs as expected.

To be clear: this PR solves issue #215, correct?

[rtucek]

Yeah, both of them (this PR and #233) fix the issue.
You can use this file to verify the result.

[bighappyface]

@rtucek thanks for verifying the fix.

How about we revert the initial is_object changes at the top of UndefinedConstraint and leave only the changes from line 61 on?

I have applied the patch of this PR to my local copy and only the later changes have any bearing on the min/maxProperties behavior, and I would prefer to leave the exception in for BC.

After that, we are good to go.

[rtucek]

@bighappyface unfortunately it's far more complicated:
The problem is how UndefinedConstraint determines if an objectConstraint should be run on the $element.

So far it's only triggered if $schema has at least one of the following keywords:
"properties", "patternProperties" or "additionalProperties" (and they're all hardcoded in the source code).
Now we've learned that there are more object-related constraints (like min-/maxProperties which is according to the definition only for JSON objects - so I agree with you that min-/mapProperties check is misplaced in UndefinedConstraint).

However, based its current implementation, in order to run the objectConstraint against the current $element I had to decide either (A) bloating up the if statement on line 68 like so:

if (is_object($value) && (isset($schema->properties) || isset($schema->patternProperties) || isset($schema->additionalProperties) || isset($schema->minProperties) || isset($schema->maxProperties))) {

or (B) always trying to run the objectConstraint against $value if the data type of $value is an object (in fact this is how all the others checkX constraints are triggered in UndefinedConstraint::validateTypes).
This however requires to ignore any non-object schema definition (which has been implemented on line 28) and remove the the throwing of an InvalidArgumentException (line 32 to 38).

I think the underlying problem is that objectConstraint was only (and really only) build for the purpose of checking "properties", "patternProperties" and "additionalProperties" keywords if at least one of them is present. So if the objectConstraint ever gets extended by additional checks (like min-/maxProperties), you'd have to add additional logic...
Probably the best approach is to rewrite ObjectConstraint to behave more like String-, Collection- or NumberConstraint.

[bighappyface]

I am still unclear why the exception has to be removed and I think it is important to reiterate the importance of backward compatibility considering consumer code out there may be looking to catch exceptions to indicate a bad schema.

[rtucek]

Yeah, I agree that this may break BC for people who're probably expecting bad schema.

I still would suggest #233 which has way less influence on BC. On the flip side the check still belongs to UndefinedConstraint.

If you'd like to enforce having the min-/maxProperties check in the objectConstraint - refactoring that class is unfortunately required. From my point of view, there is no other way around.

[bighappyface]

IMO the check for anything other than properties doesn't make much sense as additionalProperties (etc) is an aspect of the constraint instead of an indicator of a constraint.

#233 is definitely minimal but I am not completely convinced that there isn't some unintended consequence, but that is likely true for this PR as well.

Overall I think getting the min/maxProperties validation in ObjectConstraint is the best long-term improvement to build upon as it makes more sense and is less cryptic than continuing with misplaced object validation in UndefinedConstraint.

+1