Add property name in draft-3 required error #430
Conversation
Add property name in draft-3 required error
|
I guess #429 need to be merged for the tests to succeed in HHVM |
src/JsonSchema/ConstraintError.php
Outdated
| @@ -93,7 +93,7 @@ public function getMessage() | |||
| self::NOT => 'Matched a schema which it should not', | |||
| self::ONE_OF => 'Failed to match exactly one schema', | |||
| self::REQUIRED => 'The property %s is required', | |||
| self::REQUIRED_D3 => 'Is missing and it is required', | |||
| self::REQUIRED_D3 => 'The property %s is required', | |||
There was a problem hiding this comment.
This line is redundant - there's already an identical message template on line 95 (REQUIRED), so IMO it makes more sense to just use that.
| @@ -144,7 +144,8 @@ protected function validateCommonProperties(&$value, $schema = null, JsonPointer | |||
| } elseif (isset($schema->required) && !is_array($schema->required)) { | |||
| // Draft 3 - Required attribute - e.g. "foo": {"type": "string", "required": true} | |||
| if ($schema->required && $value instanceof self) { | |||
| $this->addError(ConstraintError::REQUIRED_D3(), $path); | |||
| $propertyName = current($path->getPropertyPaths()); | |||
There was a problem hiding this comment.
Please use end() rather than current() - current() makes assumptions about the state of the internal pointer on the returned array, which could result in hard-to-find bugs if the pointer code is changed at a later date.
Yep. I'm not seeing any show-stoppers though; there's nothing in your PR that will cause HHVM to fail. |
| @@ -144,7 +144,8 @@ protected function validateCommonProperties(&$value, $schema = null, JsonPointer | |||
| } elseif (isset($schema->required) && !is_array($schema->required)) { | |||
| // Draft 3 - Required attribute - e.g. "foo": {"type": "string", "required": true} | |||
| if ($schema->required && $value instanceof self) { | |||
| $this->addError(ConstraintError::REQUIRED_D3(), $path); | |||
| $propertyName = end(array_values($path->getPropertyPaths())); | |||
There was a problem hiding this comment.
Why are you using array_values here? Unless I'm missing something, calling array_values will do absolutely nothing in this situation.
There was a problem hiding this comment.
Are you using it as a shortcut to solve end() wanting a reference?
There was a problem hiding this comment.
Yes, I did it to avoid an intermediate variable. But unfortunately it throws a warning saying 'only variables can be passed to end as argument'
There was a problem hiding this comment.
Yeah, end() and friends are a bit annoying that way, although I can understand why it's required - sometimes you need to modify the referenced element, which can't be done without a reference. Would be nice if there was a handy way to refer to the last element without requiring this kind of shenanigans, but sadly PHP doesn't provide that functionality.
You definitely get points for creativity - using array_values to resolve it is certainly thinking outside the box! The intent is much less obvious than just using the intermediate variable though. The extra line is annoying, but IMO worth it for making the code obvious to whoever else reads it.
There was a problem hiding this comment.
It's days like these when I wish PHP had proper pointers (as per C).
|
@erayd thank you so much for your quick feedbacks! |
|
No worries - thanks for your contributions! |
|
@bighappyface Travis failure for HHVM can be ignored for this PR; it's caused by Travis removing HHVM support from their default stable platform. The code is fine, and all PHP versions pass OK. The platform issue is addressed in #429. |
|
#429 has been merged, so this PR will now pass Travis after being rebased. @sunspikes - could you please rebase this, as other PRs (in particular #429) have been merged since you opened it. |
|
@bighappyface Unfortunately i deleted my branch :( Anyway i have created a new PR against the updated master #432 |
In case of required error with draft-3, the error message is missing property names. (Similar to #91)
Creating a new PR as mentioned in #428 against 6.0.0-dev