Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Update NotEmpty validator to use bitmasking #5874

Closed
wants to merge 8 commits into from

4 participants

@dstockto

This updates the NotEmpty validator to use bitmasks instead of adding and subtracting the values. While it may not seem like a big deal, there are bugs that can crop up if a user creates code that adds in the same value more than once. For instance, without this patch, setting the type to array('boolean', 'boolean') means that the validator will behave as though you'd set it to array('integer').

Additionally, there's no reason to subtract the value of the bit mask from the $type value since we can use bitwise math to determine which flags are set. Using |= instead of adding means that even if we set the same flag lots of times, the value still will be the same. I've added a new unit test which will fail with the current NotEmpty implementation but will work with the updated implementation.

The documentation needs to be updated as well since the NotEmpty docs talk about adding the values together which is not the correct way to use these flags.

The values for the flags have also been updated to hex as it makes it a little more clear what's actually happening. Each validation value is a number represented by a single bit (with the exception of the combined values like PHP and ALL). I did not updated the 493, but it should probably be commented or otherwise indicated the values that are being checked by default (boolean, float, string, empty array, null and space).

The only other test changed was to add 3 more checks to ensure the default setting treat 0.0 as invalid, 1.0 as valid and an empty stdClass object as valid.

David Stockton Update NotEmpty validator to use bitmasking instead of addition and s…
…ubtraction to fix an issue when string types are duplicated
0c0c19b
tests/ZendTest/Validator/NotEmptyTest.php
((5 lines not shown))
+ /**
+ * Ensures that the validator follows expected behavior so if a string is specified more than once, it doesn't
+ * cause different validations to run
+ *
+ * @return void
+ */
+ public function testStringNotationWithDuplicate()
+ {
+ $filter = new NotEmpty(
+ array(
+ // Should only count as 'boolean'
+ 'type' => array('boolean', 'boolean')
+ )
+ );
+
+ $this->assertFalse($filter->isValid(false));
@Maks3w Collaborator
Maks3w added a note

Please refactor this as a data provider

@Maks3w Collaborator
Maks3w added a note

Forgive this. Whole tests will need to be refactored and remove the duplication

I'd be happy to redo the entire test suite as data providers. I was following existing code as an example.

@Maks3w Collaborator
Maks3w added a note

Yep, but not in this PR. Too noise.

I'll have it updated shortly. I'll do the test -> data provider refactor in a different PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w
Collaborator

Please check line 363 of test. May should be changed to bits

@dstockto

I've posted an update to move the new test to a data validator and the test you indicated (line 363) moved to using bitwise math.

@Maks3w

This don't test isValid method.

All the other tests do test that. The issue was that the type was being set incorrectly if a string type was set more than once.

@dstockto

@Maks3w Do you want it updated to test isValid for all the various types if the validator is set with a double string of something? I can do that but I think it's overkill. We know the validators work based on setting the values appropriately because each of them is tested. This is testing that setting the type using duplicated string values ends up with the right type. As long as that's the case, we're in a known state (the type is right) and that's tested.

@dstockto

@Maks3w If you disagree and want additional tests for isValid when the validator is configured with a duplicated string type, I can do that, but I'd like to know how you envision those working or looking.

@weierophinney

@dstockto Easiest way to do those tests is to use a data provider; that way you write the test case once, passing it many sets of data. Ping me on IRC if you want assistance.

@weierophinney weierophinney added this to the 2.2.6 milestone
@dstockto

@weierophinney I'm just trying to get an understanding of how much we want to do with that.

I've got PR #5879 posted which converts all the existing multi-assert tests to data providers.

(Pasted from the bottom of this, if you want to know my reason on it, it's below this section.)
TL;DR - I think the added tests are for and should be for checking that the type is set correctly. The other tests already validate isValid for examples of different data for each type. The bitmask problem is due to incorrect bitmask implementation so the tests test the setting of the type and leave the testing of the isValid method to the tests that are already doing that.

I guess the question on this one remains:

We've got tests for isValid for ~12 data types for each NotEmpty type. The issue I was originally fixing is adding the same type via a string more than once which results in the wrong internal type in the validator. The tests shows that the behavior is corrected. It seems to make sense that if the tests that assure validation works properly if the type is set properly work then we rely on those for isValid. The other test should determine if setting type works properly which is a different piece of functionality.

I guess where I'm getting stuck on the data provider tests that you and @Maks3w have asked for is that it seems to be "thorough" I'd need some way to do a dataProvider provider test -- in other words testing the duplication of the string setting for each type and then for each type testing that it's valid in the same way the existing tests check.

For instance, if an individual data set was

array('boolean', array( ... array of example types and expected validity value ...))

Then the way that test would run would be to loop over each of the values in the inner array to make sure that it's valid in the way we expect. Another option is essentially duplicate every data provider test from #5879 but have the test try to set up the validator with a duplicate string type, and then use the same data providers from those tests.

One final way I've thought of would be to have a single test, but with a massive 200+ line data provider that looks something like this:

public function bigOlDataProvider() 
{ 
    return array(
        array('boolean', true, true),
        array('boolean', false, false),
        array('boolean', 1, true),
        array('boolean', 0, true),
..<snip 8 more types for boolean>..

        array('integer', 0, false),
        array('integer', 1, true),
        array('integer', false, true),
       array('integer', true, true),
.. <snip 8 more types for integer> ..

<snip 12 lines (or more) for each of the different validator types>

   );
}

In any case, I feel like the tests there are just redundant. There are already redundant tests in the testcase now (multiple times we're looking for the 493 value for instance.

TL;DR - I think the added tests are for and should be for checking that the type is set correctly. The other tests already validate isValid for examples of different data for each type. The bitmask problem is due to incorrect bitmask implementation so the tests test the setting of the type and leave the testing of the isValid method to the tests that are already doing that.

@Ocramius
Collaborator

@dstockto could you rebase this? There are conflicts because of your recently merged PR about refactoring tests for this validator

tests/ZendTest/Validator/NotEmptyTest.php
@@ -360,7 +363,7 @@ public function testArrayConfigNotation()
public function testMultiConstantNotation()
{
$filter = new NotEmpty(
- NotEmpty::ZERO + NotEmpty::STRING + NotEmpty::BOOLEAN
@Ocramius Collaborator
Ocramius added a note

This should probably not be changed, but a different test method with the bitwise OR could be used

@dstockto
dstockto added a note

@Ocramius the reason I changed it was because it is the wrong way to set the flags. There's really nothing we can do to prevent someone from doing something like

new NotEmpty(NotEmpty::BOOLEAN + NotEmpty::BOOLEAN);

I can certainly put it back and add another test for the bitwise OR but I figured it was a bad example in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius self-assigned this
@weierophinney

@dstockto I don't think we need to test isValid any more than it already is -- as you correctly note, this is more about ensuring that the bitmask does not get corrupted. Test that, and you're good. (I think you may already have done that.)

David Stockton added some commits
David Stockton Update NotEmpty validator to use bitmasking instead of addition and s…
…ubtraction to fix an issue when string types are duplicated
5d0d435
David Stockton Change to dataprovider for duplicate type setting test for NotEmpty v…
…alidator
bef9c37
David Stockton Rebase onto master
Merge branch 'notEmptyBitMask' of github.com:dstockto/zf2 into notEmptyBitMask

Conflicts:
	tests/ZendTest/Validator/NotEmptyTest.php
2150114
@dstockto

@Ocramius I think it's rebased now. It did some odd things so if it is not how you would like it, please let me know. I may need some assistance with the rebase.

@dstockto

@weierophinney The test for setting the bitmask is on line 734 and is a data provider for all type constants provided. It ensures that setting the string twice for each will result in the expected bitmast value.

@dstockto

I think I broke something with the rebase, looking at it now.

@dstockto

Should be good now. Not sure how two whole methods ended up being duplicated.

@Ocramius Ocramius was unassigned by weierophinney
@weierophinney weierophinney self-assigned this
@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/5874' into develop
Forward port #5874
39f4a79
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-validator
@weierophinney weierophinney Merge pull request zendframework/zf2#5874 from dstockto/notEmptyBitMask
Update NotEmpty validator to use bitmasking
ee3aa95
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-validator
@weierophinney weierophinney Merge branch 'hotfix/5874' 6546999
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-validator
@weierophinney weierophinney Merge branch 'hotfix/5874' into develop 69a4f37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 26, 2014
  1. Update NotEmpty validator to use bitmasking instead of addition and s…

    David Stockton authored
    …ubtraction to fix an issue when string types are duplicated
  2. Change to dataprovider for duplicate type setting test for NotEmpty v…

    David Stockton authored
    …alidator
Commits on Mar 4, 2014
  1. @weierophinney

    Merge branch 'hotfix/5773'

    weierophinney authored
    Close #5773
    Fixes #5772
  2. Update NotEmpty validator to use bitmasking instead of addition and s…

    David Stockton authored
    …ubtraction to fix an issue when string types are duplicated
  3. Change to dataprovider for duplicate type setting test for NotEmpty v…

    David Stockton authored
    …alidator
  4. Rebase onto master

    David Stockton authored
    Merge branch 'notEmptyBitMask' of github.com:dstockto/zf2 into notEmptyBitMask
    
    Conflicts:
    	tests/ZendTest/Validator/NotEmptyTest.php
  5. Removed duplicated test

    David Stockton authored
This page is out of date. Refresh to see the latest.
View
63 library/Zend/Validator/NotEmpty.php
@@ -14,19 +14,19 @@
class NotEmpty extends AbstractValidator
{
- const BOOLEAN = 1;
- const INTEGER = 2;
- const FLOAT = 4;
- const STRING = 8;
- const ZERO = 16;
- const EMPTY_ARRAY = 32;
- const NULL = 64;
- const PHP = 127;
- const SPACE = 128;
- const OBJECT = 256;
- const OBJECT_STRING = 512;
- const OBJECT_COUNT = 1024;
- const ALL = 2047;
+ const BOOLEAN = 0x001;
+ const INTEGER = 0x002;
+ const FLOAT = 0x004;
+ const STRING = 0x008;
+ const ZERO = 0x010;
+ const EMPTY_ARRAY = 0x020;
+ const NULL = 0x040;
+ const PHP = 0x07F;
+ const SPACE = 0x080;
+ const OBJECT = 0x100;
+ const OBJECT_STRING = 0x200;
+ const OBJECT_COUNT = 0x400;
+ const ALL = 0x7FF;
const INVALID = 'notEmptyInvalid';
const IS_EMPTY = 'isEmpty';
@@ -128,9 +128,9 @@ public function setType($type = null)
$detected = 0;
foreach ($type as $value) {
if (is_int($value)) {
- $detected += $value;
+ $detected |= $value;
} elseif (in_array($value, $this->constants)) {
- $detected += array_search($value, $this->constants);
+ $detected |= array_search($value, $this->constants);
}
}
@@ -167,8 +167,7 @@ public function isValid($value)
$object = false;
// OBJECT_COUNT (countable object)
- if ($type >= self::OBJECT_COUNT) {
- $type -= self::OBJECT_COUNT;
+ if ($type & self::OBJECT_COUNT) {
$object = true;
if (is_object($value) && ($value instanceof \Countable) && (count($value) == 0)) {
@@ -178,8 +177,7 @@ public function isValid($value)
}
// OBJECT_STRING (object's toString)
- if ($type >= self::OBJECT_STRING) {
- $type -= self::OBJECT_STRING;
+ if ($type & self::OBJECT_STRING) {
$object = true;
if ((is_object($value) && (!method_exists($value, '__toString'))) ||
@@ -190,8 +188,7 @@ public function isValid($value)
}
// OBJECT (object)
- if ($type >= self::OBJECT) {
- $type -= self::OBJECT;
+ if ($type & self::OBJECT) {
// fall trough, objects are always not empty
} elseif ($object === false) {
// object not allowed but object given -> return false
@@ -202,8 +199,7 @@ public function isValid($value)
}
// SPACE (' ')
- if ($type >= self::SPACE) {
- $type -= self::SPACE;
+ if ($type & self::SPACE) {
if (is_string($value) && (preg_match('/^\s+$/s', $value))) {
$this->error(self::IS_EMPTY);
return false;
@@ -211,8 +207,7 @@ public function isValid($value)
}
// NULL (null)
- if ($type >= self::NULL) {
- $type -= self::NULL;
+ if ($type & self::NULL) {
if ($value === null) {
$this->error(self::IS_EMPTY);
return false;
@@ -220,8 +215,7 @@ public function isValid($value)
}
// EMPTY_ARRAY (array())
- if ($type >= self::EMPTY_ARRAY) {
- $type -= self::EMPTY_ARRAY;
+ if ($type & self::EMPTY_ARRAY) {
if (is_array($value) && ($value == array())) {
$this->error(self::IS_EMPTY);
return false;
@@ -229,8 +223,7 @@ public function isValid($value)
}
// ZERO ('0')
- if ($type >= self::ZERO) {
- $type -= self::ZERO;
+ if ($type & self::ZERO) {
if (is_string($value) && ($value == '0')) {
$this->error(self::IS_EMPTY);
return false;
@@ -238,8 +231,7 @@ public function isValid($value)
}
// STRING ('')
- if ($type >= self::STRING) {
- $type -= self::STRING;
+ if ($type & self::STRING) {
if (is_string($value) && ($value == '')) {
$this->error(self::IS_EMPTY);
return false;
@@ -247,8 +239,7 @@ public function isValid($value)
}
// FLOAT (0.0)
- if ($type >= self::FLOAT) {
- $type -= self::FLOAT;
+ if ($type & self::FLOAT) {
if (is_float($value) && ($value == 0.0)) {
$this->error(self::IS_EMPTY);
return false;
@@ -256,8 +247,7 @@ public function isValid($value)
}
// INTEGER (0)
- if ($type >= self::INTEGER) {
- $type -= self::INTEGER;
+ if ($type & self::INTEGER) {
if (is_int($value) && ($value == 0)) {
$this->error(self::IS_EMPTY);
return false;
@@ -265,8 +255,7 @@ public function isValid($value)
}
// BOOLEAN (false)
- if ($type >= self::BOOLEAN) {
- $type -= self::BOOLEAN;
+ if ($type & self::BOOLEAN) {
if (is_bool($value) && ($value == false)) {
$this->error(self::IS_EMPTY);
return false;
View
65 tests/ZendTest/Validator/NotEmptyTest.php
@@ -67,6 +67,7 @@ public function basicProvider()
array(array(5), true),
array(0.0, false),
array(1.0, true),
+ array(new stdClass(), true),
);
}
@@ -587,6 +588,25 @@ public function testMultiConstantNotation($value, $valid)
}
/**
+ * Ensures that the validator follows expected behavior
+ *
+ * @param mixed $value Value to test
+ * @param boolean $valid Expected validity of value
+ *
+ * @return void
+ *
+ * @dataProvider multiConstantNotationProvider
+ */
+ public function testMultiConstantBooleanOrNotation($value, $valid)
+ {
+ $this->validator = new NotEmpty(
+ NotEmpty::ZERO | NotEmpty::STRING | NotEmpty::BOOLEAN
+ );
+
+ $this->checkValidationValue($value, $valid);
+ }
+
+ /**
* Provides values and their expected validity for boolean empty
*
* @return array
@@ -654,6 +674,51 @@ public function stringNotationProvider()
);
}
+
+ /**
+ * Ensures that the validator follows expected behavior so if a string is specified more than once, it doesn't
+ * cause different validations to run
+ *
+ * @param string $string Array of string type values
+ * @param integer $expected Expected type setting value
+ *
+ * @return void
+ *
+ * @dataProvider duplicateStringSettingProvider
+ */
+ public function testStringNotationWithDuplicate($string, $expected)
+ {
+ $type = array($string, $string);
+ $this->validator->setType($type);
+
+ $this->assertEquals($expected, $this->validator->getType());
+ }
+
+ /**
+ * Data provider for testStringNotationWithDuplicate method. Provides a string which will be duplicated. The test
+ * ensures that setting a string value more than once only turns on the appropriate bit once
+ *
+ * @return array
+ */
+ public function duplicateStringSettingProvider()
+ {
+ return array(
+ array('boolean', NotEmpty::BOOLEAN),
+ array('integer', NotEmpty::INTEGER),
+ array('float', NotEmpty::FLOAT),
+ array('string', NotEmpty::STRING),
+ array('zero', NotEmpty::ZERO),
+ array('array', NotEmpty::EMPTY_ARRAY),
+ array('null', NotEmpty::NULL),
+ array('php', NotEmpty::PHP),
+ array('space', NotEmpty::SPACE),
+ array('object', NotEmpty::OBJECT),
+ array('objectstring', NotEmpty::OBJECT_STRING),
+ array('objectcount', NotEmpty::OBJECT_COUNT),
+ array('all', NotEmpty::ALL),
+ );
+ }
+
/**
* Ensures that the validator follows expected behavior
*
Something went wrong with that request. Please try again.