From 00a2c8e09c2f0a755beb352166aad847463a7637 Mon Sep 17 00:00:00 2001 From: flip111 Date: Wed, 23 Apr 2014 16:50:15 +0200 Subject: [PATCH 01/18] improved error handling for invalid association values Possibly to do: 1. Make custom Exception for line 713 2. Make custom Exception for line 817 3. Does the object check on line 816 slow down the code too much? Alternatively a try-catch could be put around line 1415 or higher up. --- lib/Doctrine/ORM/UnitOfWork.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 5c2eb7bc0..0fb3cca05 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -728,7 +728,11 @@ class UnitOfWork implements PropertyChangedListener continue; } - $this->computeAssociationChanges($assoc, $val); + try { + $this->computeAssociationChanges($assoc, $val); + } catch (\Exception $ex) { + throw new Exception('Expected an Object for relation '.get_class($entity).'::'.$assoc['fieldName'].' got '.gettype($ex->value).' '.var_export($ex->value, true).' instead.'); + } if ( ! isset($this->entityChangeSets[$oid]) && $assoc['isOwningSide'] && @@ -830,6 +834,12 @@ class UnitOfWork implements PropertyChangedListener $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); foreach ($unwrappedValue as $key => $entry) { + if (! is_object($entry)) { + $ex = new \Exception(gettype($entry) . ' ' . var_export($entry, true).' is not an Object.'); + $ex->value = $entry; + throw $ex; + } + $state = $this->getEntityState($entry, self::STATE_NEW); if ( ! ($entry instanceof $assoc['targetEntity'])) { From b120dafb7083f0e6eaed62af726475e321af6d16 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 10:11:57 +0100 Subject: [PATCH 02/18] Added new exception constructors --- lib/Doctrine/ORM/ORMInvalidArgumentException.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index adb0cd529..ffd85ef0a 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -198,4 +198,20 @@ class ORMInvalidArgumentException extends \InvalidArgumentException { return method_exists($obj, '__toString') ? (string)$obj : get_class($obj).'@'.spl_object_hash($obj); } + + /** + * @return ORMInvalidArgumentException + */ + public static function invalidAssociation($relation, $fieldname, $value) { + return new self('Expected an Object for relation '.get_class($relation).'::'.$fieldname.' got '.gettype($value).' instead.'); + } + + /** + * @return ORMInvalidArgumentException + */ + public static function invalidAssociation($entry) { + $ex = new self(gettype($entry) . ' is not an Object.'); + $ex->value = $entry; + return $ex; + } } From 88e071d22d8ce1b88fd5f7d67fd6a72980422430 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 10:12:14 +0100 Subject: [PATCH 03/18] moved exception constructors out of UoW --- lib/Doctrine/ORM/UnitOfWork.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 0fb3cca05..3ac943cba 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -731,7 +731,7 @@ class UnitOfWork implements PropertyChangedListener try { $this->computeAssociationChanges($assoc, $val); } catch (\Exception $ex) { - throw new Exception('Expected an Object for relation '.get_class($entity).'::'.$assoc['fieldName'].' got '.gettype($ex->value).' '.var_export($ex->value, true).' instead.'); + throw ORMInvalidArgumentException::invalidAssociation($entity, $assoc['fieldName'], $ex->value); } if ( ! isset($this->entityChangeSets[$oid]) && @@ -834,10 +834,8 @@ class UnitOfWork implements PropertyChangedListener $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); foreach ($unwrappedValue as $key => $entry) { - if (! is_object($entry)) { - $ex = new \Exception(gettype($entry) . ' ' . var_export($entry, true).' is not an Object.'); - $ex->value = $entry; - throw $ex; + if (! ($entry instanceof $assoc['targetEntity']))) { + throw ORMInvalidArgumentException::invalidAssociation($entry); } $state = $this->getEntityState($entry, self::STATE_NEW); From 04e494060750a4792e1797f41ba8a42f00d46c51 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 19:59:07 +0100 Subject: [PATCH 04/18] Update ORMInvalidArgumentException.php `@return self` trend break with `@return ORMInvalidArgumentException` --- .../ORM/ORMInvalidArgumentException.php | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index ffd85ef0a..526efdc5b 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -187,6 +187,28 @@ class ORMInvalidArgumentException extends \InvalidArgumentException return new self("Binding entities to query parameters only allowed for entities that have an identifier."); } + /** + * @param object $relation + * @param string $fieldname + * @param mixed $value + * @return self + */ + public static function invalidAssociation($relation, $fieldname, $value) + { + return new self(sprintf('Expected an Object for relation %s::%s got %s instead.', get_class($relation), $fieldname, gettype($value))); + } + + /** + * @param mixed $entry + * @return self + */ + public static function invalidAssociation($entry) + { + $ex = new self(gettype($entry) . (is_scalar($entry) ? ' "'.$entry.'"': '') . ' is not an Object.'); + $ex->value = $entry; + return $ex; + } + /** * Helper method to show an object as string. * @@ -198,20 +220,4 @@ class ORMInvalidArgumentException extends \InvalidArgumentException { return method_exists($obj, '__toString') ? (string)$obj : get_class($obj).'@'.spl_object_hash($obj); } - - /** - * @return ORMInvalidArgumentException - */ - public static function invalidAssociation($relation, $fieldname, $value) { - return new self('Expected an Object for relation '.get_class($relation).'::'.$fieldname.' got '.gettype($value).' instead.'); - } - - /** - * @return ORMInvalidArgumentException - */ - public static function invalidAssociation($entry) { - $ex = new self(gettype($entry) . ' is not an Object.'); - $ex->value = $entry; - return $ex; - } } From 643ae786917efee17c58c6920699beb186883537 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 20:02:31 +0100 Subject: [PATCH 05/18] Update ORMInvalidArgumentException.php Add unused parameters for `invalidAssociation` --- lib/Doctrine/ORM/ORMInvalidArgumentException.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index 526efdc5b..e94e3f632 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -199,10 +199,12 @@ class ORMInvalidArgumentException extends \InvalidArgumentException } /** + * @param \Doctrine\ORM\Mapping\ClassMetadata $targetClass + * @param array $assoc * @param mixed $entry * @return self */ - public static function invalidAssociation($entry) + public static function invalidAssociation($targetClass, $assoc, $entry) { $ex = new self(gettype($entry) . (is_scalar($entry) ? ' "'.$entry.'"': '') . ' is not an Object.'); $ex->value = $entry; From 1ae153d3151492f9f1e620a2c1a55409f4b7d870 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 20:02:39 +0100 Subject: [PATCH 06/18] Update UnitOfWork.php --- lib/Doctrine/ORM/UnitOfWork.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 3ac943cba..6bc964d0e 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -834,8 +834,8 @@ class UnitOfWork implements PropertyChangedListener $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); foreach ($unwrappedValue as $key => $entry) { - if (! ($entry instanceof $assoc['targetEntity']))) { - throw ORMInvalidArgumentException::invalidAssociation($entry); + if (! ($entry instanceof $targetClass->name))) { + throw ORMInvalidArgumentException::invalidAssociation($targetClass, $assoc, $entry); } $state = $this->getEntityState($entry, self::STATE_NEW); From 059c33e69d95f436e79043153a854eec1f331923 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 20:15:53 +0100 Subject: [PATCH 07/18] Update UnitOfWork.php removed one `)` too many --- lib/Doctrine/ORM/UnitOfWork.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 6bc964d0e..49176da5d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -834,7 +834,7 @@ class UnitOfWork implements PropertyChangedListener $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); foreach ($unwrappedValue as $key => $entry) { - if (! ($entry instanceof $targetClass->name))) { + if (! ($entry instanceof $targetClass->name)) { throw ORMInvalidArgumentException::invalidAssociation($targetClass, $assoc, $entry); } From b5dd999f84fc5b88da4220f7ea00f2596523dc81 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 20:24:10 +0100 Subject: [PATCH 08/18] Update ORMInvalidArgumentException.php rename duplicate method --- lib/Doctrine/ORM/ORMInvalidArgumentException.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index e94e3f632..8b10d0023 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -193,7 +193,7 @@ class ORMInvalidArgumentException extends \InvalidArgumentException * @param mixed $value * @return self */ - public static function invalidAssociation($relation, $fieldname, $value) + public static function computeAssociationChangesError($relation, $fieldname, $value) { return new self(sprintf('Expected an Object for relation %s::%s got %s instead.', get_class($relation), $fieldname, gettype($value))); } From d1a265509008e19b9042020dd0c7879c8533a64d Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 20:24:29 +0100 Subject: [PATCH 09/18] Update UnitOfWork.php rename duplicate method --- lib/Doctrine/ORM/UnitOfWork.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 49176da5d..c022a1ae4 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -731,7 +731,7 @@ class UnitOfWork implements PropertyChangedListener try { $this->computeAssociationChanges($assoc, $val); } catch (\Exception $ex) { - throw ORMInvalidArgumentException::invalidAssociation($entity, $assoc['fieldName'], $ex->value); + throw ORMInvalidArgumentException::computeAssociationChangesError($entity, $assoc['fieldName'], $ex->value); } if ( ! isset($this->entityChangeSets[$oid]) && From 83de071c00c825ff94f61768ce800ed1c5c698e0 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 13 Jan 2015 21:10:16 +0100 Subject: [PATCH 10/18] Update ORMInvalidArgumentException.php remove added `value` to exception --- lib/Doctrine/ORM/ORMInvalidArgumentException.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index 8b10d0023..d0089d752 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -206,9 +206,7 @@ class ORMInvalidArgumentException extends \InvalidArgumentException */ public static function invalidAssociation($targetClass, $assoc, $entry) { - $ex = new self(gettype($entry) . (is_scalar($entry) ? ' "'.$entry.'"': '') . ' is not an Object.'); - $ex->value = $entry; - return $ex; + return new self(gettype($entry) . (is_scalar($entry) ? ' "'.$entry.'"': '') . ' is not an Object.'); } /** From 5e76f120007bc7267005d1fdd14d5cc812975f4d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 00:37:21 +0100 Subject: [PATCH 11/18] #1228 DDC-3490 - tests for invalid association values handled in the UnitOfWork --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 109 ++++++++++++++++---- 1 file changed, 87 insertions(+), 22 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index a64f2b428..affc99159 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -2,29 +2,49 @@ namespace Doctrine\Tests\ORM; +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\NotifyPropertyChanged; +use Doctrine\Common\PropertyChangedListener; +use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Mocks\ConnectionMock; +use Doctrine\Tests\Mocks\DriverMock; use Doctrine\Tests\Mocks\EntityManagerMock; -use Doctrine\Tests\Mocks\UnitOfWorkMock; use Doctrine\Tests\Mocks\EntityPersisterMock; -use Doctrine\Tests\Models\Forum\ForumUser; +use Doctrine\Tests\Mocks\UnitOfWorkMock; use Doctrine\Tests\Models\Forum\ForumAvatar; +use Doctrine\Tests\Models\Forum\ForumUser; +use stdClass; /** * UnitOfWork tests. */ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase { - // SUT + /** + * SUT + * + * @var UnitOfWorkMock + */ private $_unitOfWork; - // Provides a sequence mock to the UnitOfWork + + /** + * Provides a sequence mock to the UnitOfWork + * + * @var ConnectionMock + */ private $_connectionMock; - // The EntityManager mock that provides the mock persisters + + /** + * The EntityManager mock that provides the mock persisters + * + * @var EntityManagerMock + */ private $_emMock; protected function setUp() { parent::setUp(); - $this->_connectionMock = new ConnectionMock(array(), new \Doctrine\Tests\Mocks\DriverMock()); + $this->_connectionMock = new ConnectionMock(array(), new DriverMock()); $this->_emMock = EntityManagerMock::create($this->_connectionMock); // SUT $this->_unitOfWork = new UnitOfWorkMock($this->_emMock); @@ -49,9 +69,9 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase public function testSavingSingleEntityWithIdentityColumnForcesInsert() { // Setup fake persister and id generator for identity generation - $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\Forum\ForumUser")); + $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumUser')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\Forum\ForumUser', $userPersister); - $userPersister->setMockIdGeneratorType(\Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_IDENTITY); + $userPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY); // Test $user = new ForumUser(); @@ -89,13 +109,13 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase { // Setup fake persister and id generator for identity generation //ForumUser - $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\Forum\ForumUser")); + $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumUser')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\Forum\ForumUser', $userPersister); - $userPersister->setMockIdGeneratorType(\Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_IDENTITY); + $userPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY); // ForumAvatar - $avatarPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\Forum\ForumAvatar")); + $avatarPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumAvatar')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\Forum\ForumAvatar', $avatarPersister); - $avatarPersister->setMockIdGeneratorType(\Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_IDENTITY); + $avatarPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY); // Test $user = new ForumUser(); @@ -120,9 +140,9 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase public function testChangeTrackingNotify() { - $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\ORM\NotifyChangedEntity")); + $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\ORM\NotifyChangedEntity')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\ORM\NotifyChangedEntity', $persister); - $itemPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\ORM\NotifyChangedRelatedItem")); + $itemPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\ORM\NotifyChangedRelatedItem')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\ORM\NotifyChangedRelatedItem', $itemPersister); $entity = new NotifyChangedEntity; @@ -164,7 +184,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase public function testGetEntityStateOnVersionedEntityWithAssignedIdentifier() { - $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity")); + $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity', $persister); $e = new VersionedAssignedIdentifierEntity(); @@ -175,7 +195,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase public function testGetEntityStateWithAssignedIdentity() { - $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\CMS\CmsPhonenumber")); + $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\Models\CMS\CmsPhonenumber')); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\CMS\CmsPhonenumber', $persister); $ph = new \Doctrine\Tests\Models\CMS\CmsPhonenumber(); @@ -197,13 +217,13 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase } /** - * DDC-2086 [GH-484] Prevented "Undefined index" notice when updating. + * DDC-2086 [GH-484] Prevented 'Undefined index' notice when updating. */ public function testNoUndefinedIndexNoticeOnScheduleForUpdateWithoutChanges() { // Setup fake persister and id generator - $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\Forum\ForumUser")); - $userPersister->setMockIdGeneratorType(\Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_IDENTITY); + $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumUser')); + $userPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY); $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\Forum\ForumUser', $userPersister); // Create a test user @@ -227,12 +247,57 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase $this->setExpectedException('InvalidArgumentException'); $this->_unitOfWork->lock(null, null, null); } + + /** + * @group DDC-3490 + * + * @dataProvider invalidAssociationValuesDataProvider + * + * @param mixed $invalidValue + */ + public function testRejectsInvalidAssociationValue($invalidValue) + { + $metadata = $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumUser'); + + $this->_unitOfWork->setEntityPersister( + 'Doctrine\Tests\Models\Forum\ForumUser', + new EntityPersisterMock($this->_emMock, $metadata) + ); + + // Create a test user + $user = new ForumUser(); + $user->username = 'John'; + $user->avatar = $invalidValue; + + $this->_unitOfWork->persist($user); + + $this->setExpectedException('Doctrine\ORM\ORMInvalidArgumentException'); + + $this->_unitOfWork->computeChangeSet($metadata, $user); + } + + /** + * Data Provider + * + * @return mixed[][] + */ + public function invalidAssociationValuesDataProvider() + { + return [ + ['foo'], + [['foo']], + [''], + [[]], + [new stdClass()], + [new ArrayCollection()], + ]; + } } /** * @Entity */ -class NotifyChangedEntity implements \Doctrine\Common\NotifyPropertyChanged +class NotifyChangedEntity implements NotifyPropertyChanged { private $_listeners = array(); /** @@ -252,7 +317,7 @@ class NotifyChangedEntity implements \Doctrine\Common\NotifyPropertyChanged private $items; public function __construct() { - $this->items = new \Doctrine\Common\Collections\ArrayCollection; + $this->items = new ArrayCollection; } public function getId() { @@ -281,7 +346,7 @@ class NotifyChangedEntity implements \Doctrine\Common\NotifyPropertyChanged } } - public function addPropertyChangedListener(\Doctrine\Common\PropertyChangedListener $listener) + public function addPropertyChangedListener(PropertyChangedListener $listener) { $this->_listeners[] = $listener; } From 71a6a88de82d23232cdf5377a44804a5f55d29b8 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 00:53:34 +0100 Subject: [PATCH 12/18] #1228 DDC-3490 -better/more complete exception message for invalid populated associations --- .../ORM/ORMInvalidArgumentException.php | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index d0089d752..633ff5c60 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -18,6 +18,7 @@ */ namespace Doctrine\ORM; +use Doctrine\ORM\Mapping\ClassMetadata; /** * Contains exception messages for all invalid lifecycle state exceptions inside UnitOfWork @@ -195,18 +196,36 @@ class ORMInvalidArgumentException extends \InvalidArgumentException */ public static function computeAssociationChangesError($relation, $fieldname, $value) { - return new self(sprintf('Expected an Object for relation %s::%s got %s instead.', get_class($relation), $fieldname, gettype($value))); + return new self(sprintf( + 'Expected an Object for relation %s::%s got %s instead.', + get_class($relation), + $fieldname, + is_object($value) ? get_class($value) : gettype($value) + )); } /** - * @param \Doctrine\ORM\Mapping\ClassMetadata $targetClass - * @param array $assoc - * @param mixed $entry + * @param ClassMetadata $targetClass + * @param array $assoc + * @param mixed $actualValue + * * @return self */ - public static function invalidAssociation($targetClass, $assoc, $entry) + public static function invalidAssociation(ClassMetadata $targetClass, $assoc, $actualValue) { - return new self(gettype($entry) . (is_scalar($entry) ? ' "'.$entry.'"': '') . ' is not an Object.'); + $expectedType = 'Doctrine\Common\Collections\Collection|array'; + + if (($assoc['type'] & ClassMetadata::TO_ONE) > 0) { + $expectedType = $targetClass->getName(); + } + + return new self(sprintf( + 'Expected value of type "%s" for association field "%s#$%s", got "%s" instead.', + $expectedType, + $assoc['sourceEntity'], + $assoc['fieldName'], + is_object($actualValue) ? get_class($actualValue) : gettype($actualValue) + )); } /** From d0c0f43c79a9f3ac42cd74ac845b6392c03d9d71 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 00:53:57 +0100 Subject: [PATCH 13/18] #1228 DDC-3490 - preventing invalid association values from being persisted --- lib/Doctrine/ORM/UnitOfWork.php | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index c022a1ae4..9764a6eca 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -731,7 +731,7 @@ class UnitOfWork implements PropertyChangedListener try { $this->computeAssociationChanges($assoc, $val); } catch (\Exception $ex) { - throw ORMInvalidArgumentException::computeAssociationChangesError($entity, $assoc['fieldName'], $ex->value); + throw ORMInvalidArgumentException::computeAssociationChangesError($entity, $assoc['fieldName'], $val); } if ( ! isset($this->entityChangeSets[$oid]) && @@ -2188,12 +2188,31 @@ class UnitOfWork implements PropertyChangedListener case ($relatedEntities instanceof Collection): case (is_array($relatedEntities)): + if (($assoc['type'] & ClassMetadata::TO_MANY) <= 0) { + throw ORMInvalidArgumentException::invalidAssociation( + $this->em->getClassMetadata($assoc['targetEntity']), + $assoc, + $relatedEntities + ); + } + foreach ($relatedEntities as $relatedEntity) { $this->doPersist($relatedEntity, $visited); } + break; case ($relatedEntities !== null): + $targetClass = $this->em->getClassMetadata($assoc['targetEntity'])->name; + + if (! $relatedEntities instanceof $targetClass) { + throw ORMInvalidArgumentException::invalidAssociation( + $this->em->getClassMetadata($assoc['targetEntity']), + $assoc, + $relatedEntities + ); + } + $this->doPersist($relatedEntities, $visited); break; From 9c1275bb1fcd19c234758c6e4594d20aa6b44c00 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 00:54:18 +0100 Subject: [PATCH 14/18] #1228 DDC-3490 - persistence of invalid values should also cause exceptions --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index affc99159..a0a1b8167 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -269,10 +269,9 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase $user->username = 'John'; $user->avatar = $invalidValue; - $this->_unitOfWork->persist($user); - $this->setExpectedException('Doctrine\ORM\ORMInvalidArgumentException'); + $this->_unitOfWork->persist($user); $this->_unitOfWork->computeChangeSet($metadata, $user); } From 66479334d4f904996442779064987979c5ca7511 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 00:55:40 +0100 Subject: [PATCH 15/18] #1228 DDC-3490 - computing changes of invalid objects should also fail --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 35 ++++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index a0a1b8167..df7256e44 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -255,7 +255,33 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase * * @param mixed $invalidValue */ - public function testRejectsInvalidAssociationValue($invalidValue) + public function testRejectsPersistenceOfObjectsWithInvalidAssociationValue($invalidValue) + { + $this->_unitOfWork->setEntityPersister( + 'Doctrine\Tests\Models\Forum\ForumUser', + new EntityPersisterMock( + $this->_emMock, + $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumUser') + ) + ); + + $user = new ForumUser(); + $user->username = 'John'; + $user->avatar = $invalidValue; + + $this->setExpectedException('Doctrine\ORM\ORMInvalidArgumentException'); + + $this->_unitOfWork->persist($user); + } + + /** + * @group DDC-3490 + * + * @dataProvider invalidAssociationValuesDataProvider + * + * @param mixed $invalidValue + */ + public function testRejectsChangeSetComputationForObjectsWithInvalidAssociationValue($invalidValue) { $metadata = $this->_emMock->getClassMetadata('Doctrine\Tests\Models\Forum\ForumUser'); @@ -264,14 +290,15 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase new EntityPersisterMock($this->_emMock, $metadata) ); - // Create a test user - $user = new ForumUser(); + $user = new ForumUser(); + + $this->_unitOfWork->persist($user); + $user->username = 'John'; $user->avatar = $invalidValue; $this->setExpectedException('Doctrine\ORM\ORMInvalidArgumentException'); - $this->_unitOfWork->persist($user); $this->_unitOfWork->computeChangeSet($metadata, $user); } From 1cd03625a57dba1f7a2f11070d145f6926b6f33f Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 00:59:58 +0100 Subject: [PATCH 16/18] #1228 DDC-3490 - fixed exception catching in `BasicFunctionalTest` logic --- .../Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php index be290acb3..29f8fab85 100644 --- a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php @@ -1288,9 +1288,14 @@ class BasicFunctionalTest extends \Doctrine\Tests\OrmFunctionalTestCase $user->status = 'developer'; $user->address = $user; + $this->setExpectedException( + 'Doctrine\ORM\ORMInvalidArgumentException', + 'Expected value of type "Doctrine\Tests\Models\CMS\CmsAddress" for association field ' + . '"Doctrine\Tests\Models\CMS\CmsUser#$address", got "Doctrine\Tests\Models\CMS\CmsUser" instead.' + ); + $this->_em->persist($user); - $this->setExpectedException("Doctrine\ORM\ORMException", "Found entity of type Doctrine\Tests\Models\CMS\CmsUser on association Doctrine\Tests\Models\CMS\CmsUser#address, but expecting Doctrine\Tests\Models\CMS\CmsAddress"); $this->_em->flush(); } } From b1d7a057fd1825cd1aee0434567c3310a6aaeb23 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 01:05:36 +0100 Subject: [PATCH 17/18] #1228 DDC-3490 - avoid catching unknown exceptions, remove unused method call --- lib/Doctrine/ORM/UnitOfWork.php | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 9764a6eca..32e4b8603 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -728,11 +728,7 @@ class UnitOfWork implements PropertyChangedListener continue; } - try { - $this->computeAssociationChanges($assoc, $val); - } catch (\Exception $ex) { - throw ORMInvalidArgumentException::computeAssociationChangesError($entity, $assoc['fieldName'], $val); - } + $this->computeAssociationChanges($assoc, $val); if ( ! isset($this->entityChangeSets[$oid]) && $assoc['isOwningSide'] && @@ -2203,9 +2199,7 @@ class UnitOfWork implements PropertyChangedListener break; case ($relatedEntities !== null): - $targetClass = $this->em->getClassMetadata($assoc['targetEntity'])->name; - - if (! $relatedEntities instanceof $targetClass) { + if (! $relatedEntities instanceof $assoc['targetEntity']) { throw ORMInvalidArgumentException::invalidAssociation( $this->em->getClassMetadata($assoc['targetEntity']), $assoc, From c7f5ee8e9e1d6da627b7c622ef2c76fd8fe70ee4 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Jan 2015 01:05:53 +0100 Subject: [PATCH 18/18] #1228 DDC-3490 - Remove unused method --- lib/Doctrine/ORM/ORMInvalidArgumentException.php | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index 633ff5c60..02c0f99aa 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -188,22 +188,6 @@ class ORMInvalidArgumentException extends \InvalidArgumentException return new self("Binding entities to query parameters only allowed for entities that have an identifier."); } - /** - * @param object $relation - * @param string $fieldname - * @param mixed $value - * @return self - */ - public static function computeAssociationChangesError($relation, $fieldname, $value) - { - return new self(sprintf( - 'Expected an Object for relation %s::%s got %s instead.', - get_class($relation), - $fieldname, - is_object($value) ? get_class($value) : gettype($value) - )); - } - /** * @param ClassMetadata $targetClass * @param array $assoc