From a1c13b58d3e7c88c09906c93afcc8994c6f3e0ed Mon Sep 17 00:00:00 2001 From: "Roman S. Borschel" Date: Thu, 8 Jul 2010 17:30:39 +0200 Subject: [PATCH] Fixed behavior of PersistentCollection#clear(). Fixed single-valued association path expression support in DQL UpdateItems. --- lib/Doctrine/ORM/PersistentCollection.php | 13 ++++-- lib/Doctrine/ORM/Query.php | 1 + lib/Doctrine/ORM/Query/AST/UpdateItem.php | 7 ++-- .../Query/Exec/MultiTableUpdateExecutor.php | 5 ++- lib/Doctrine/ORM/Query/Parser.php | 23 +--------- lib/Doctrine/ORM/Query/SqlWalker.php | 22 +++------- .../ORM/Functional/BasicFunctionalTest.php | 42 ++++++++++++++++--- .../ORM/Query/UpdateSqlGenerationTest.php | 8 ++++ .../Doctrine/Tests/OrmFunctionalTestCase.php | 4 +- 9 files changed, 71 insertions(+), 54 deletions(-) diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php index 0d36260f1..b1cd9d40a 100644 --- a/lib/Doctrine/ORM/PersistentCollection.php +++ b/lib/Doctrine/ORM/PersistentCollection.php @@ -560,15 +560,20 @@ final class PersistentCollection implements Collection */ public function clear() { - $this->initialize(); // TODO: not needed!? - $result = $this->coll->clear(); + if ($this->initialized && $this->isEmpty()) { + return; + } + if ($this->association->isOneToMany() && $this->association->orphanRemoval) { + foreach ($this->coll as $element) { + $this->em->getUnitOfWork()->scheduleOrphanRemoval($element); + } + } + $this->coll->clear(); if ($this->association->isOwningSide) { $this->changed(); $this->em->getUnitOfWork()->scheduleCollectionDeletion($this); $this->takeSnapshot(); } - - return $result; } /** diff --git a/lib/Doctrine/ORM/Query.php b/lib/Doctrine/ORM/Query.php index 18087ddd0..4c755d0f4 100644 --- a/lib/Doctrine/ORM/Query.php +++ b/lib/Doctrine/ORM/Query.php @@ -242,6 +242,7 @@ final class Query extends AbstractQuery } if (is_object($value) && $this->_em->getMetadataFactory()->hasMetadataFor(get_class($value))) { + //TODO: Check that $value is MANAGED? $values = $this->_em->getUnitOfWork()->getEntityIdentifier($value); $sqlPositions = $paramMappings[$key]; $sqlParams += array_combine((array)$sqlPositions, $values); diff --git a/lib/Doctrine/ORM/Query/AST/UpdateItem.php b/lib/Doctrine/ORM/Query/AST/UpdateItem.php index 929149897..5f8b67802 100644 --- a/lib/Doctrine/ORM/Query/AST/UpdateItem.php +++ b/lib/Doctrine/ORM/Query/AST/UpdateItem.php @@ -36,13 +36,12 @@ namespace Doctrine\ORM\Query\AST; */ class UpdateItem extends Node { - public $identificationVariable; - public $field; + public $pathExpression; public $newValue; - public function __construct($field, $newValue) + public function __construct($pathExpression, $newValue) { - $this->field = $field; + $this->pathExpression = $pathExpression; $this->newValue = $newValue; } diff --git a/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php b/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php index 43adaddea..d6f355684 100644 --- a/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php +++ b/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php @@ -84,8 +84,9 @@ class MultiTableUpdateExecutor extends AbstractSqlExecutor $updateSql = 'UPDATE ' . $class->getQuotedTableName($platform) . ' SET '; foreach ($updateItems as $updateItem) { - $field = $updateItem->field; - if (isset($class->fieldMappings[$field]) && ! isset($class->fieldMappings[$field]['inherited'])) { + $field = $updateItem->pathExpression->parts[0]; + if (isset($class->fieldMappings[$field]) && ! isset($class->fieldMappings[$field]['inherited']) || + isset($class->associationMappings[$field]) && ! $class->associationMappings[$field]->inherited) { $newValue = $updateItem->newValue; if ( ! $affected) { diff --git a/lib/Doctrine/ORM/Query/Parser.php b/lib/Doctrine/ORM/Query/Parser.php index 009941cfb..d17a92070 100644 --- a/lib/Doctrine/ORM/Query/Parser.php +++ b/lib/Doctrine/ORM/Query/Parser.php @@ -1291,28 +1291,9 @@ class Parser */ public function UpdateItem() { - $token = $this->_lexer->lookahead; - - $identVariable = $this->IdentificationVariable(); - $this->match(Lexer::T_DOT); - $this->match(Lexer::T_IDENTIFIER); - $field = $this->_lexer->token['value']; - - // Check if field exists - $class = $this->_queryComponents[$identVariable]['metadata']; - - if ( ! isset($class->associationMappings[$field]) && ! isset($class->fieldMappings[$field])) { - $this->semanticalError( - 'Class ' . $class->name . ' has no field named ' . $field, $token - ); - } - + $pathExpr = $this->PathExpression(AST\PathExpression::TYPE_STATE_FIELD | AST\PathExpression::TYPE_SINGLE_VALUED_ASSOCIATION); $this->match(Lexer::T_EQUALS); - - $newValue = $this->NewValue(); - - $updateItem = new AST\UpdateItem($field, $newValue); - $updateItem->identificationVariable = $identVariable; + $updateItem = new AST\UpdateItem($pathExpr, $this->NewValue()); return $updateItem; } diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 89634ecb4..6875629f7 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -1202,26 +1202,16 @@ class SqlWalker implements TreeWalker $useTableAliasesBefore = $this->_useSqlTableAliases; $this->_useSqlTableAliases = false; - $sql = ''; - $dqlAlias = $updateItem->identificationVariable; - $qComp = $this->_queryComponents[$dqlAlias]; - - if ($this->_useSqlTableAliases) { - $sql .= $this->getSqlTableAlias($qComp['metadata']->getTableName()) . '.'; - } - - $sql .= $qComp['metadata']->getQuotedColumnName($updateItem->field, $this->_platform) . ' = '; + $sql = $this->walkPathExpression($updateItem->pathExpression) . ' = '; $newValue = $updateItem->newValue; - if ($newValue instanceof AST\Node) { + if ($newValue === null) { + $sql .= 'NULL'; + } else if ($newValue instanceof AST\Node) { $sql .= $newValue->dispatch($this); - } else if (is_string($newValue)) { - if (strcasecmp($newValue, 'NULL') === 0) { - $sql .= 'NULL'; - } else { - $sql .= $this->_conn->quote($newValue); - } + } else { + $sql .= $this->_conn->quote($newValue); } $this->_useSqlTableAliases = $useTableAliasesBefore; diff --git a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php index 3292b5a5a..24928f377 100644 --- a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php @@ -161,10 +161,13 @@ class BasicFunctionalTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->_em->flush(); // Check that there are just 2 phonenumbers left - $count = $this->_em->getConnection()->executeQuery( - "SELECT COUNT(*) FROM cms_phonenumbers", array() - )->fetchColumn(); + $count = $this->_em->getConnection()->fetchColumn("SELECT COUNT(*) FROM cms_phonenumbers"); $this->assertEquals(2, $count); // only 2 remaining + + // check that clear() removes the others via orphan removal + $user->getPhonenumbers()->clear(); + $this->_em->flush(); + $this->assertEquals(0, $this->_em->getConnection()->fetchColumn("select count(*) from cms_phonenumbers")); } public function testBasicQuery() @@ -664,9 +667,6 @@ class BasicFunctionalTest extends \Doctrine\Tests\OrmFunctionalTestCase } catch (\InvalidArgumentException $expected) {} } - /** - * @group orphan - */ public function testOneToOneOrphanRemoval() { //$this->_em->getConnection()->getConfiguration()->setSQLLogger(new \Doctrine\DBAL\Logging\EchoSQLLogger); @@ -713,6 +713,36 @@ class BasicFunctionalTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertEquals(1, $this->_em->getConnection()->fetchColumn("select count(*) from cms_addresses")); $this->assertEquals(0, $this->_em->getConnection()->fetchColumn("select count(*) from cms_addresses where id=".$addressId."")); } + + public function testClearingCollectionDoesNotInitialize() + { + $user = new CmsUser(); + $user->username = "beberlei"; + $user->name = "Benjamin E."; + $user->status = 'active'; + + $grp = new CmsGroup(); + $grp->setName("The Dudes"); + + $grp->addUser($user); + $user->addGroup($grp); + + $this->_em->persist($user); + $this->_em->persist($grp); + $this->_em->flush(); + $this->_em->clear(); + + $this->assertEquals(1, $this->_em->getConnection()->fetchColumn("select count(*) from cms_users_groups")); + + $user2 = $this->_em->find(get_class($user), $user->id); + $this->assertFalse($user2->groups->isInitialized()); + $user2->groups->clear(); + $this->assertFalse($user2->groups->isInitialized()); + $this->_em->flush(); + $this->assertFalse($user2->groups->isInitialized()); + + $this->assertEquals(0, $this->_em->getConnection()->fetchColumn("select count(*) from cms_users_groups")); + } //DRAFT OF EXPECTED/DESIRED BEHAVIOR /*public function testPersistentCollectionContainsDoesNeverInitialize() diff --git a/tests/Doctrine/Tests/ORM/Query/UpdateSqlGenerationTest.php b/tests/Doctrine/Tests/ORM/Query/UpdateSqlGenerationTest.php index 506dacb8b..725e2aeb6 100644 --- a/tests/Doctrine/Tests/ORM/Query/UpdateSqlGenerationTest.php +++ b/tests/Doctrine/Tests/ORM/Query/UpdateSqlGenerationTest.php @@ -167,4 +167,12 @@ class UpdateSqlGenerationTest extends \Doctrine\Tests\OrmTestCase "UPDATE cms_phonenumbers SET phonenumber = 1234 WHERE user_id = ?" ); } + + public function testSingleValuedAssociationFieldInSetClause() + { + $this->assertSqlGeneration( + "update Doctrine\Tests\Models\CMS\CmsComment c set c.article = null where c.article=?1", + "UPDATE cms_comments SET article_id = NULL WHERE article_id = ?" + ); + } } diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index 1ee4a85b3..a1bf2b2bf 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -45,7 +45,8 @@ abstract class OrmFunctionalTestCase extends OrmTestCase 'Doctrine\Tests\Models\CMS\CmsPhonenumber', 'Doctrine\Tests\Models\CMS\CmsAddress', 'Doctrine\Tests\Models\CMS\CmsGroup', - 'Doctrine\Tests\Models\CMS\CmsArticle' + 'Doctrine\Tests\Models\CMS\CmsArticle', + 'Doctrine\Tests\Models\CMS\CmsComment', ), 'forum' => array(), 'company' => array( @@ -106,6 +107,7 @@ abstract class OrmFunctionalTestCase extends OrmTestCase $conn->executeUpdate('DELETE FROM cms_groups'); $conn->executeUpdate('DELETE FROM cms_addresses'); $conn->executeUpdate('DELETE FROM cms_phonenumbers'); + $conn->executeUpdate('DELETE FROM cms_comments'); $conn->executeUpdate('DELETE FROM cms_articles'); $conn->executeUpdate('DELETE FROM cms_users'); }