Permalink
Browse files

merged branch canni/fix_collection_validator (PR #3078)

Commits
-------

7f7c2a7 Add prof-of-concept test, this test will fail without changes in previous commit
253eeba [BugFix][Validator] Fix for PHP incosistent behaviour of ArrayAccess

Discussion
----------

[BugFix][Validator] Fix for PHP incosistent behaviour of ArrayAccess

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2779
Todo: -

[![Build Status](https://secure.travis-ci.org/canni/symfony.png)](http://travis-ci.org/canni/symfony)

Because PHP function `array_key_exists` is buggy, it works great with native
PHP `ArrayObject` instances, but hand written implementations of `ArrayAccess`
and `Traversable` objects will fail to work with `CollectionValidator`

Tests from second commit are valid use cases, but without this change, they will fail.
  • Loading branch information...
fabpot committed Jan 12, 2012
2 parents 7ee2f6d + 7f7c2a7 commit c185302c0dd7c47b9c8e77bed47a361e99eb7abc
@@ -52,7 +52,11 @@ public function isValid($value, Constraint $constraint)
}
foreach ($constraint->fields as $field => $constraints) {
- if (array_key_exists($field, $value)) {
+ if (
+ // bug fix issue #2779
+ (is_array($value) && array_key_exists($field, $value)) ||
+ ($value instanceof \ArrayAccess && $value->offsetExists($field))
+ ) {
// cannot simply cast to array, because then the object is converted to an
// array instead of wrapped inside
$constraints = is_array($constraints) ? $constraints : array($constraints);
@@ -13,9 +13,71 @@
use Symfony\Component\Validator\ExecutionContext;
use Symfony\Component\Validator\Constraints\Min;
+use Symfony\Component\Validator\Constraints\NotBlank;
+use Symfony\Component\Validator\Constraints\NotNull;
use Symfony\Component\Validator\Constraints\Collection;
use Symfony\Component\Validator\Constraints\CollectionValidator;
+/**
+ * This class is a hand written simplified version of PHP native `ArrayObject`
+ * class, to show that it behaves different than PHP native implementation.
+ */
+class TestArrayObject implements \ArrayAccess, \IteratorAggregate, \Countable, \Serializable
+{
+ private $array;
+
+ public function __construct(array $array = null)
+ {
+ $this->array = (array) ($array ?: array());
+ }
+
+ public function offsetExists($offset)
+ {
+ return array_key_exists($offset, $this->array);
+ }
+
+ public function offsetGet($offset)
+ {
+ return $this->array[$offset];
+ }
+
+ public function offsetSet($offset, $value)
+ {
+ if (null === $offset) {
+ $this->array[] = $value;
+ } else {
+ $this->array[$offset] = $value;
+ }
+ }
+
+ public function offsetUnset($offset)
+ {
+ if (array_key_exists($offset, $this->array)) {
+ unset($this->array[$offset]);
+ }
+ }
+
+ public function getIterator()
+ {
+ return new \ArrayIterator($this->array);
+ }
+
+ public function count()
+ {
+ return count($this->array);
+ }
+
+ public function serialize()
+ {
+ return serialize($this->array);
+ }
+
+ public function unserialize($serialized)
+ {
+ $this->array = (array) unserialize((string) $serialized);
+ }
+}
+
class CollectionValidatorTest extends \PHPUnit_Framework_TestCase
{
protected $validator;
@@ -49,15 +111,22 @@ public function testNullIsValid()
public function testFieldsAsDefaultOption()
{
- $this->validator->isValid(array('foo' => 'foobar'), new Collection(array(
+ $this->assertTrue($this->validator->isValid(array('foo' => 'foobar'), new Collection(array(
'foo' => new Min(4),
- )));
+ ))));
+ $this->assertTrue($this->validator->isValid(new \ArrayObject(array('foo' => 'foobar')), new Collection(array(
+ 'foo' => new Min(4),
+ ))));
+ $this->assertTrue($this->validator->isValid(new TestArrayObject(array('foo' => 'foobar')), new Collection(array(
+ 'foo' => new Min(4),
+ ))));
}
+ /**
+ * @expectedException Symfony\Component\Validator\Exception\UnexpectedTypeException
+ */
public function testThrowsExceptionIfNotTraversable()
{
- $this->setExpectedException('Symfony\Component\Validator\Exception\UnexpectedTypeException');
-
$this->validator->isValid('foobar', new Collection(array('fields' => array(
'foo' => new Min(4),
))));
@@ -112,13 +181,11 @@ public function testWalkMultipleConstraints($array)
))));
}
- public function testExtraFieldsDisallowed()
+ /**
+ * @dataProvider getArgumentsWithExtraFields
+ */
+ public function testExtraFieldsDisallowed($array)
{
- $array = array(
- 'foo' => 5,
- 'bar' => 6,
- );
-
$this->assertFalse($this->validator->isValid($array, new Collection(array(
'fields' => array(
'foo' => new Min(4),
@@ -132,12 +199,15 @@ public function testNullNotConsideredExtraField()
$array = array(
'foo' => null,
);
-
- $this->assertTrue($this->validator->isValid($array, new Collection(array(
+ $collection = new Collection(array(
'fields' => array(
'foo' => new Min(4),
),
- ))));
+ ));
+
+ $this->assertTrue($this->validator->isValid($array, $collection));
+ $this->assertTrue($this->validator->isValid(new \ArrayObject($array), $collection));
+ $this->assertTrue($this->validator->isValid(new TestArrayObject($array), $collection));
}
public function testExtraFieldsAllowed()
@@ -146,13 +216,16 @@ public function testExtraFieldsAllowed()
'foo' => 5,
'bar' => 6,
);
-
- $this->assertTrue($this->validator->isValid($array, new Collection(array(
+ $collection = new Collection(array(
'fields' => array(
'foo' => new Min(4),
),
'allowExtraFields' => true,
- ))));
+ ));
+
+ $this->assertTrue($this->validator->isValid($array, $collection));
+ $this->assertTrue($this->validator->isValid(new \ArrayObject($array), $collection));
+ $this->assertTrue($this->validator->isValid(new TestArrayObject($array), $collection));
}
public function testMissingFieldsDisallowed()
@@ -162,6 +235,16 @@ public function testMissingFieldsDisallowed()
'foo' => new Min(4),
),
))));
+ $this->assertFalse($this->validator->isValid(new \ArrayObject(array()), new Collection(array(
+ 'fields' => array(
+ 'foo' => new Min(4),
+ ),
+ ))));
+ $this->assertFalse($this->validator->isValid(new TestArrayObject(array()), new Collection(array(
+ 'fields' => array(
+ 'foo' => new Min(4),
+ ),
+ ))));
}
public function testMissingFieldsAllowed()
@@ -172,16 +255,58 @@ public function testMissingFieldsAllowed()
),
'allowMissingFields' => true,
))));
+ $this->assertTrue($this->validator->isValid(new \ArrayObject(array()), new Collection(array(
+ 'fields' => array(
+ 'foo' => new Min(4),
+ ),
+ 'allowMissingFields' => true,
+ ))));
+ $this->assertTrue($this->validator->isValid(new TestArrayObject(array()), new Collection(array(
+ 'fields' => array(
+ 'foo' => new Min(4),
+ ),
+ 'allowMissingFields' => true,
+ ))));
}
- public function getValidArguments()
- {
- return array(
- // can only test for one entry, because PHPUnits mocking does not allow
- // to expect multiple method calls with different arguments
- array(array('foo' => 3)),
- array(new \ArrayObject(array('foo' => 3))),
- );
+ public function testArrayAccessObject() {
+ $value = new TestArrayObject();
+ $value['foo'] = 12;
+ $value['asdf'] = 'asdfaf';
+
+ $this->assertTrue(isset($value['asdf']));
+ $this->assertTrue(isset($value['foo']));
+ $this->assertFalse(empty($value['asdf']));
+ $this->assertFalse(empty($value['foo']));
+
+ $result = $this->validator->isValid($value, new Collection(array(
+ 'fields' => array(
+ 'foo' => new NotBlank(),
+ 'asdf' => new NotBlank()
+ )
+ )));
+
+ $this->assertTrue($result);
+ }
+
+ public function testArrayObject() {
+ $value = new \ArrayObject(array());
+ $value['foo'] = 12;
+ $value['asdf'] = 'asdfaf';
+
+ $this->assertTrue(isset($value['asdf']));
+ $this->assertTrue(isset($value['foo']));
+ $this->assertFalse(empty($value['asdf']));
+ $this->assertFalse(empty($value['foo']));
+
+ $result = $this->validator->isValid($value, new Collection(array(
+ 'fields' => array(
+ 'foo' => new NotBlank(),
+ 'asdf' => new NotBlank()
+ )
+ )));
+
+ $this->assertTrue($result);
}
public function testObjectShouldBeLeftUnchanged()
@@ -199,4 +324,34 @@ public function testObjectShouldBeLeftUnchanged()
'foo' => 3
), (array) $value);
}
+
+ public function getValidArguments()
+ {
+ return array(
+ // can only test for one entry, because PHPUnits mocking does not allow
+ // to expect multiple method calls with different arguments
+ array(array('foo' => 3)),
+ array(new \ArrayObject(array('foo' => 3))),
+ array(new TestArrayObject(array('foo' => 3))),
+ );
+ }
+
+ public function getArgumentsWithExtraFields()
+ {
+ return array(
+ array(array(
+ 'foo' => 5,
+ 'bar' => 6,
+ )),
+ array(new \ArrayObject(array(
+ 'foo' => 5,
+ 'bar' => 6,
+ ))),
+ array(new TestArrayObject(array(
+ 'foo' => 5,
+ 'bar' => 6,
+ )))
+ );
+ }
}
+

0 comments on commit c185302

Please sign in to comment.