Expression assertion #23
Conversation
CS fixes. Testing ExpressionAssertion. 'regex' operator instead of 'contains'. Exception in case that context object property cannot be resolved. Exception in case that context object property cannot be resolved. Exception in case that context object property cannot be resolved.
| const OPERATOR_GT = '>'; | ||
| const OPERATOR_GTE = '>='; | ||
| const OPERATOR_IN = 'in'; | ||
| const OPERATOR_NIN = 'nin'; |
There was a problem hiding this comment.
I think this one would make more sense as !in, making it more akin to !=. I'll make that change on merge.
| /** | ||
| * @var array | ||
| */ | ||
| private $assertContext = []; |
There was a problem hiding this comment.
This should be removed. assert() should pass the context to methods that need it.
The reason is concurrency. In async systems, if this value gets reset in another process, it can affect the operands in the current process. By passing it around instead, the context of the current process can never be changed.
| self::OPERATOR_REGEX, | ||
| ]; | ||
|
|
||
| private function __construct($left, $operator, $right) |
There was a problem hiding this comment.
There really needs to be documentation of what constitutes valid operands.
| * @param string $operator | ||
| * @param mixed $right | ||
| * @return self | ||
| */ |
There was a problem hiding this comment.
This should indicate exceptions that can be thrown by validateOperand() and validateOperator().
| /** | ||
| * @param array $expression | ||
| * @throws InvalidAssertionException | ||
| * @return self |
There was a problem hiding this comment.
Same comment here, and also indicate the more specific exception this method raises.
| continue; | ||
| } | ||
|
|
||
| return $object->$accessor(); |
There was a problem hiding this comment.
The above could be more simply written as:
if (method_exists($object, $accessor)) {
return $object->accessor();
}| } | ||
|
|
||
| if (!property_exists($object, $field)) { | ||
| throw new \RuntimeException('Object property cannot be resolved'); |
There was a problem hiding this comment.
Why are you using a global exception case here, versus the package-specific one elsewhere?
| case self::OPERATOR_EQ : | ||
| return $left == $right; | ||
| case self::OPERATOR_NEQ : | ||
| return $left != $right; |
There was a problem hiding this comment.
Out of curiousity, why are these using the non-strict forms? Do you think we should have additional operators for strict in/equality?
| throw new RuntimeException(sprintf( | ||
| 'Unsupported expression assertion operator: %s', | ||
| $operator | ||
| )); |
There was a problem hiding this comment.
Technically, there is no possible way to get to the default case, as you've validated the operator previously.
| return $object->$accessor(); | ||
| } | ||
|
|
||
| if (!property_exists($object, $field)) { |
There was a problem hiding this comment.
This doesn't do what you think it does. Or, rather, it has an unintentional side-effect you may not be aware of.
Starting in PHP 5.3, property_exists started returning true regardless of visibility. This means that the above will return true even if the property is not public. Which makes line 227 a potential error. The only way to solve this is via reflection.
|
I've incorporated all feedback in #36; I would have pushed to your branch, but you've locked it. 😄 |
|
@weierophinney It can be closed now, as #36 is merged and 2.7.0 is released. |
I think it would be useful to have possibility for defining assertions in this way, as those can be very easily persisted or serialized in contrast to
CallbackAssertionfor example. Also, having such dynamic and generic type of assertion is much more convenient when it comes to creating some permissions CRUD functionality.Example usage