Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Update validators to allow instantiation without options #15

Closed
wants to merge 2 commits into from
Closed

Update validators to allow instantiation without options #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a515863
Update validators to allow instantiation without options
alexdenvir Jul 7, 2015
b87e2cc
=== null instead of is_null
alexdenvir Jul 8, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/Between.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ class Between extends AbstractValidator
*/
protected $options = [
'inclusive' => true, // Whether to do inclusive comparisons, allowing equivalence to min and/or max
'min' => 0,
'max' => PHP_INT_MAX,
'min' => null,
'max' => null,
];

/**
Expand All @@ -66,7 +66,13 @@ public function __construct($options = null)
}
if (!is_array($options)) {
$options = func_get_args();
$temp['min'] = array_shift($options);

$temp = [];

if (!empty($options)) {
$temp['min'] = array_shift($options);
}

if (!empty($options)) {
$temp['max'] = array_shift($options);
}
Expand All @@ -78,12 +84,6 @@ public function __construct($options = null)
$options = $temp;
}

if (count($options) !== 2
&& (!array_key_exists('min', $options) || !array_key_exists('max', $options))
) {
throw new Exception\InvalidArgumentException("Missing option. 'min' and 'max' have to be given");
}

parent::__construct($options);
}

Expand Down Expand Up @@ -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");
Copy link
Member

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?

Copy link
Contributor Author

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

}

if ($this->getInclusive()) {
if ($this->getMin() > $value || $value > $this->getMax()) {
$this->error(self::NOT_BETWEEN);
Expand Down
24 changes: 13 additions & 11 deletions src/GreaterThan.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ public function __construct($options = null)
}
if (!is_array($options)) {
$options = func_get_args();
$temp['min'] = array_shift($options);

$temp = [];

if (!empty($options)) {
$temp['min'] = array_shift($options);
}

if (!empty($options)) {
$temp['inclusive'] = array_shift($options);
Expand All @@ -73,17 +78,10 @@ public function __construct($options = null)
$options = $temp;
}

if (!array_key_exists('min', $options)) {
throw new Exception\InvalidArgumentException("Missing option 'min'");
}

if (!array_key_exists('inclusive', $options)) {
$options['inclusive'] = false;
}

$this->setMin($options['min'])
->setInclusive($options['inclusive']);

parent::__construct($options);
}

Expand Down Expand Up @@ -141,13 +139,17 @@ public function isValid($value)
{
$this->setValue($value);

if ($this->inclusive) {
if ($this->min > $value) {
if ($this->getMin() === null) {
throw new Exception\InvalidArgumentException("Missing option 'min'");
}

if ($this->getInclusive()) {
if ($this->getMin() > $value) {
$this->error(self::NOT_GREATER_INCLUSIVE);
return false;
}
} else {
if ($this->min >= $value) {
if ($this->getMin() >= $value) {
$this->error(self::NOT_GREATER);
return false;
}
Expand Down
13 changes: 8 additions & 5 deletions src/IsInstanceOf.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,12 @@ public function __construct($options = null)
$options = func_get_args();

$tmpOptions = [];
$tmpOptions['className'] = array_shift($options);

$options = $tmpOptions;
}
if (!empty($options)) {
$tmpOptions['className'] = array_shift($options);
}

if (!array_key_exists('className', $options)) {
throw new Exception\InvalidArgumentException('Missing option "className"');
$options = $tmpOptions;
}

parent::__construct($options);
Expand Down Expand Up @@ -98,6 +97,10 @@ public function setClassName($className)
*/
public function isValid($value)
{
if ($this->getClassName() === null) {
throw new Exception\InvalidArgumentException('Missing option "className"');
}

if ($value instanceof $this->className) {
return true;
}
Expand Down
24 changes: 13 additions & 11 deletions src/LessThan.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ public function __construct($options = null)
}
if (!is_array($options)) {
$options = func_get_args();
$temp['max'] = array_shift($options);

$temp = [];

if (!empty($options)) {
$temp['max'] = array_shift($options);
}

if (!empty($options)) {
$temp['inclusive'] = array_shift($options);
Expand All @@ -75,17 +80,10 @@ public function __construct($options = null)
$options = $temp;
}

if (!array_key_exists('max', $options)) {
throw new Exception\InvalidArgumentException("Missing option 'max'");
}

if (!array_key_exists('inclusive', $options)) {
$options['inclusive'] = false;
}

$this->setMax($options['max'])
->setInclusive($options['inclusive']);

parent::__construct($options);
}

Expand Down Expand Up @@ -142,15 +140,19 @@ public function setInclusive($inclusive)
*/
public function isValid($value)
{
if ($this->getMax() === null) {
throw new Exception\InvalidArgumentException("Missing option 'max'");
}

$this->setValue($value);

if ($this->inclusive) {
if ($value > $this->max) {
if ($this->getInclusive()) {
if ($value > $this->getMax()) {
$this->error(self::NOT_LESS_INCLUSIVE);
return false;
}
} else {
if ($value >= $this->max) {
if ($value >= $this->getMax()) {
$this->error(self::NOT_LESS);
return false;
}
Expand Down
19 changes: 9 additions & 10 deletions src/Regex.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class Regex extends AbstractValidator
* @param string|Traversable $pattern
* @throws Exception\InvalidArgumentException On missing 'pattern' parameter
*/
public function __construct($pattern)
public function __construct($pattern = null)
{
if (is_string($pattern)) {
$this->setPattern($pattern);
Expand All @@ -60,16 +60,11 @@ public function __construct($pattern)
$pattern = ArrayUtils::iteratorToArray($pattern);
}

if (!is_array($pattern)) {
throw new Exception\InvalidArgumentException('Invalid options provided to constructor');
if (is_array($pattern) && array_key_exists('pattern', $pattern)) {
$this->setPattern($pattern['pattern']);
unset($pattern['pattern']);
}

if (!array_key_exists('pattern', $pattern)) {
throw new Exception\InvalidArgumentException("Missing option 'pattern'");
}

$this->setPattern($pattern['pattern']);
unset($pattern['pattern']);
parent::__construct($pattern);
}

Expand Down Expand Up @@ -116,6 +111,10 @@ public function setPattern($pattern)
*/
public function isValid($value)
{
if ($this->getPattern() === null) {
throw new Exception\InvalidArgumentException("Missing option 'pattern'");
}

if (!is_string($value) && !is_int($value) && !is_float($value)) {
$this->error(self::INVALID);
return false;
Expand All @@ -124,7 +123,7 @@ public function isValid($value)
$this->setValue($value);

ErrorHandler::start();
$status = preg_match($this->pattern, $value);
$status = preg_match($this->getPattern(), $value);
ErrorHandler::stop();
if (false === $status) {
$this->error(self::ERROROUS);
Expand Down
22 changes: 21 additions & 1 deletion test/BetweenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ public function testMissingMinOrMax(array $args)
"Missing option. 'min' and 'max' have to be given"
);

new Between($args);
$value = 3;

$validator = new Between($args);

$validator->isValid($value);
}

public function constructBetweenValidatorInvalidDataProvider()
Expand All @@ -130,4 +134,20 @@ public function constructBetweenValidatorInvalidDataProvider()
],
];
}

public function testCanBeInstantiatedWithoutOptions()
{
$validator = new Between();
}

public function testIsValidThrowsExceptionWithNoOptions()
{
$this->setExpectedException(
'Zend\Validator\Exception\InvalidArgumentException',
"Missing option. 'min' and 'max' have to be given"
);

$validator = new Between();
$validator->isValid(5);
}
}
16 changes: 16 additions & 0 deletions test/GreaterThanTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,20 @@ public function testCorrectNotInclusiveMessageReturn()
$this->assertEquals($message['notGreaterThanInclusive'], "The input is not greater or equal than '10'");
}
}

public function testCanBeInstantiatedWithoutOptions()
{
$validator = new GreaterThan();
}

public function testIsValidThrowsExceptionWithNoOptions()
{
$this->setExpectedException(
'Zend\Validator\Exception\InvalidArgumentException',
"Missing option 'min'"
);

$validator = new GreaterThan();
$validator->isValid(5);
}
}
17 changes: 17 additions & 0 deletions test/IsInstanceOfTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,22 @@ public function testPassOptionsWithoutClassNameKey()

$options = ['NotClassNameKey' => 'DateTime'];
$validator = new Validator\IsInstanceOf($options);
$validator->isValid(null);
}

public function testCanBeInstantiatedWithoutOptions()
{
$validator = new Validator\IsInstanceOf();
}

public function testIsValidThrowsExceptionWithNoOptions()
{
$this->setExpectedException(
'Zend\Validator\Exception\InvalidArgumentException',
'Missing option "className"'
);

$validator = new Validator\IsInstanceOf();
$validator->isValid(null);
}
}
16 changes: 16 additions & 0 deletions test/LessThanTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,20 @@ public function testEqualsMessageVariables()
$this->assertAttributeEquals($validator->getOption('messageVariables'),
'messageVariables', $validator);
}

public function testCanBeInstantiatedWithoutOptions()
{
$validator = new LessThan();
}

public function testIsValidThrowsExceptionWithNoOptions()
{
$this->setExpectedException(
'Zend\Validator\Exception\InvalidArgumentException',
"Missing option 'max'"
);

$validator = new LessThan();
$validator->isValid(5);
}
}
22 changes: 22 additions & 0 deletions test/RegexTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ public function testBadPattern()
$validator = new Regex('/');
}

public function testBadPatternWithArrayOption()
{
$this->setExpectedException('Zend\Validator\Exception\InvalidArgumentException', 'Internal error parsing');
$validator = new Regex(['pattern' => '/']);
}

/**
* @ZF-4352
*/
Expand Down Expand Up @@ -125,4 +131,20 @@ public function testEqualsMessageVariables()
$this->assertAttributeEquals($validator->getOption('messageVariables'),
'messageVariables', $validator);
}

public function testCanBeInstantiatedWithoutOptions()
{
$validator = new Regex();
}

public function testIsValidThrowsExceptionWithNoOptions()
{
$this->setExpectedException(
'Zend\Validator\Exception\InvalidArgumentException',
"Missing option 'pattern'"
);

$validator = new Regex();
$validator->isValid(5);
}
}
1 change: 0 additions & 1 deletion test/StaticValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ public function testPassingNullWhenSettingPluginManagerResetsPluginManager()

public function testExecuteValidWithParameters()
{
$this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10]));
$this->assertTrue(StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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)
  }
}

Copy link
Contributor

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 [email protected] menulis:

In test/StaticValidatorTest.php:

@@ -154,7 +154,6 @@ public function testPassingNullWhenSettingPluginManagerResetsPluginManager()

 public function testExecuteValidWithParameters()
 {

  •    $this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10]));
    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
—
Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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]));

Copy link
Contributor Author

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 😉

Copy link
Contributor

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 ;)

}
}
1 change: 0 additions & 1 deletion test/ValidatorChainTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ public function testStaticFactoryClassNotFound()

public function testIsValidWithParameters()
{
$this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10]));
$this->assertTrue(StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
}

Expand Down