-
Notifications
You must be signed in to change notification settings - Fork 136
Update validators to allow instantiation without options #15
Update validators to allow instantiation without options #15
Conversation
alexdenvir
commented
Jul 7, 2015
- Rather than throw an exception in the constructor, throw an exception in isValid
- Updates Between, GreaterThan, IsInstanceOf, LessThan and Regex
* Rather than throw an exception in the constructor, throw an exception in isValid * Updates Between, GreaterThan, IsInstanceOf, LessThan and Regex
This is an alternative solution to, and supercedes #14 From the discussion there, it would be nicer to be able to instantiate validators without any options in the constructor, but still throw an exception when the validator is used is the required options have not been set |
Pinging @gianarb @freax @manuakasam @Mezzle for comments on this as well, as they were involved in the original discussion on #14 |
Given the previous version required you to pass the params, not sure why it even had defaults. |
👍 thanks for this contribution. I think this is a much better approach than what was started in #14 I like this one presented here! |
@@ -154,7 +154,6 @@ public function testPassingNullWhenSettingPluginManagerResetsPluginManager() | |||
|
|||
public function testExecuteValidWithParameters() | |||
{ | |||
$this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10])); |
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.
don't remove it, this should be still valid.
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.
Actually, I'm not sure it ever was. It definitely isn't valid with my changes at least.
As far as I can tell, passing this array in to the constructor for Between results in the following set as options (in the master branch):
protected $options = [
// default options in master branch
'inclusive' => true,
'min' => 0,
'max' => PHP_INT_MAX,
// options created by parent:__construct
0 => 1,
1 => 10
];
Because the class has default options (in master), no error is encountered in this test because 5 is between 0 and PHP_INT_MAX
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.
% php -a code/zend-validator (master) two
Interactive mode enabled
php > require_once "vendor/autoload.php";
php > $validator = new \Zend\Validator\Between([0, 10]);
php > var_dump($validator);
class Zend\Validator\Between#2 (5) {
protected $messageTemplates =>
array(2) {
'notBetween' =>
string(57) "The input is not between '%min%' and '%max%', inclusively"
'notBetweenStrict' =>
string(53) "The input is not strictly between '%min%' and '%max%'"
}
protected $messageVariables =>
array(2) {
'min' =>
array(1) {
'options' =>
string(3) "min"
}
'max' =>
array(1) {
'options' =>
string(3) "max"
}
}
protected $options =>
array(5) {
'inclusive' =>
bool(true)
'min' =>
int(0)
'max' =>
int(9223372036854775807)
[0] =>
int(0)
[1] =>
int(10)
}
protected $value =>
NULL
protected $abstractOptions =>
array(7) {
'messages' =>
array(0) {
}
'messageTemplates' =>
array(2) {
'notBetween' =>
string(57) "The input is not between '%min%' and '%max%', inclusively"
'notBetweenStrict' =>
string(53) "The input is not strictly between '%min%' and '%max%'"
}
'messageVariables' =>
array(2) {
'min' =>
array(1) {
...
}
'max' =>
array(1) {
...
}
}
'translator' =>
NULL
'translatorTextDomain' =>
NULL
'translatorEnabled' =>
bool(true)
'valueObscured' =>
bool(false)
}
}
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.
iirc, the behavior is, on StaticValidator::execute, the args is an array of parameter. at this case, args will be extracted. First parameter as min, and second as max.
Warm regards,
Abdul Malik Ikhsan
Pada 8 Jul 2015, pukul 16.13, Alex Denvir notifications@github.com menulis:
In test/StaticValidatorTest.php:
@@ -154,7 +154,6 @@ public function testPassingNullWhenSettingPluginManagerResetsPluginManager()
public function testExecuteValidWithParameters() {
Actually, I'm not sure it ever was. It definitely isn't valid with my changes at least.$this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10]));
As far as I can tell, passing this array in to the constructor for Between results in the following set as options (in the master branch):
protected $options = [
// default options in master branch
'inclusive' => true,
'min' => 0,
'max' => PHP_INT_MAX,
// options created by parent:__construct
0 => 1,
1 => 10
];Because the class has default options (in master), no error is encountered in this test because 5 is between 0 and PHP_INT_MAX
—
Reply to this email directly or view it on GitHub.
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 agree, it probably should.
But I'm afraid it doesn't:
% php -a code/zend-validator (master) two
Interactive mode enabled
php > require_once "vendor/autoload.php";
php > var_dump(\Zend\Validator\StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
bool(true)
php > var_dump(\Zend\Validator\StaticValidator::execute(0, 'Between', ['min' => 1, 'max' => 10]));
bool(false)
php > var_dump(\Zend\Validator\StaticValidator::execute(100, 'Between', ['min' => 1, 'max' => 10]));
bool(false)
php > var_dump(\Zend\Validator\StaticValidator::execute(5, 'Between', [1, 10]));
bool(true)
php > var_dump(\Zend\Validator\StaticValidator::execute(0, 'Between', [1, 10]));
bool(true)
php > var_dump(\Zend\Validator\StaticValidator::execute(100, 'Between', [1, 10]));
bool(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.
The second test here is actually better.
Originally it was testing for 5 being between 1 and 10, which would have always been true with the defaults. It never tested for the negative case though.
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 have opened a separate issue to address this outside of this pull request: #16
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.
@alexdenvir it seems the old one is an issue that need to be updated as args is an array, so it should be:
var_dump(\Zend\Validator\StaticValidator::execute(100, 'Between', ['min' => 1, 'max' => 10]));
bool(false)
php > var_dump(\Zend\Validator\StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
bool(true)
So, it should can be updated with:
$this->assertTrue(StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
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.
That's exactly what the next line in the test is 😉
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.
cool ;). just aware of that, thanks ;)
Sorry maybe I'm lost something, why an array? new \Zend\Validator\Between([0, 10]); |
@gianarb yeah, the old one on StaticValidator call seems an issue, and can be updated with: $this->assertTrue(StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10])); |
@@ -164,6 +164,10 @@ public function isValid($value) | |||
{ | |||
$this->setValue($value); | |||
|
|||
if ($this->getMin() === null || $this->getMax() === null) { | |||
throw new Exception\InvalidArgumentException("Missing option. 'min' and 'max' have to be given"); |
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 disagree with this approach. What's the actual use-case for this change?
I'm basically just missing this bit: why do you want to instantiate a validator and keep it in an invalid state?
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.
As explained in the reply I just made to @GeeH, it's not to let you instantiate a validator and keep it in an invalid state, but instead to allow you to avoid using the "array of options with arbitrary string keys" if you want to.
Related issue I opened #16 - the current tests show an example of the Between validator being instantiated with invalid options, but not throwing an exception (because there are 2 values in the array), and the validator is then giving false positives because t falls back to the default values (0 and PHP_INT_MAX). This PR works around that, because if you pass in invalid options to the constructor (leaving the validator in an invalid state), the isValid function will throw the exception
I don't think this brings anything of value to the table. I actually would prefer you write your own implementations of these validators if you want defaults, I can't see that, for example, only passing in a max and not min to a between validator is improving anything. |
To clarify, I think that ensuring that the min/max values are passed in via the options array is the correct way to use this component, and while it might be useful to override these values after construction, I don't see what this PR improves. |
@GeeH - copy/pasted from a comment I made on the previous PR: Ultimately the reason for me creating this PR is that I'm not a fan of being forced to pass an array of options in a constructor, as the values attached to array keys cannot be typehinted, there is no IDE autocomplete for the array keys themselves etc. Compare calling: $validator = new Between(['min' => 5, 'max' => 10]); to: $validator = new Between();
$validator->setMin(5)->setMax(10); Any good IDE will autocomplete the function calls, while also telling you exactly what the function is doing and what value it is expecting, whether the value is typehinted etc. |
$validator = new Between();
$validator->setMin(5)->setMax(10); This is NOT good code.
|
@Ocramius "This is NOT good code." - seems a bit harsh considering its a difference of opinion, but whatever. As discussed in previous comments against this PR (and the previous), the Between validator has a default min of $validator1 = new Between(['min' => 5, 'max' => 10);
$validator2 = new Between(5, 10); But you can also instantiate it in invalid ways and have no Exception thrown: $bad_validator = new Between(['minimum' => 5, 'maximum' => 10]); In this example, the validator is not doing what is expected, you haven't passed a valid array of options, and isValid is going to produce false positives: $bad_validator->isValid(3);
// returns true If the checks are done and Exceptions are thrown in |
@alexdenvir - I'm sorry, I just don't agree. The validator needs a minimum and maximum value set before it can validate. That's a hard configuration dependency, and therefore these should be required at construction time. |
Just a note: Allowing an array of options to pass to the constructor allows for generalized factories for instantiation; without them, we'd need a factory per validator (or we'd need to define a As such, I tend to agree with @GeeH and @Ocramius ; this patch is moving in a different direction than we ultimately intend to be, and for that reason, I'm closing it. |