From b59fc23f869547c2eb82d7181bf456086229d379 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 10 Oct 2019 17:30:43 +0200 Subject: [PATCH 01/11] #7837 reproduced issue: DQL caching prevents `WhereInWalker` run Since `WhereInWalker` does not run, query parameters are not translated from their in-memory type to the expected SQL type when the paginator is run again with the same DQL string. This is an architectural issue, since (for the sake of simplicity) we moved parameter translation into the SQL walker, we didn't consider that SQL walkers only act when no cache is in place. The translatio needs to be moved into the paginator logic again. --- .../ORM/Functional/Ticket/GH7820Test.php | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php index b1732b4a918..afb29f08369 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php @@ -4,6 +4,7 @@ namespace Doctrine\Tests\ORM\Functional\Ticket; +use Doctrine\Common\Cache\ClearableCache; use Doctrine\Common\Collections\Criteria; use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Types\StringType; @@ -11,6 +12,7 @@ use Doctrine\ORM\Tools\Pagination\Paginator; use Doctrine\Tests\OrmFunctionalTestCase; use function array_map; +use function assert; use function is_string; use function iterator_to_array; @@ -53,6 +55,9 @@ protected function setUp() : void $this->setUpEntitySchema([GH7820Line::class]); + $this->_em->createQuery('DELETE FROM ' . GH7820Line::class . ' l') + ->execute(); + foreach (self::SONG as $index => $line) { $this->_em->persist(new GH7820Line(GH7820LineText::fromText($line), $index)); } @@ -73,6 +78,41 @@ public function testWillFindSongsInPaginator() : void }, iterator_to_array(new Paginator($query))) ); } + + /** @group GH7837 */ + public function testWillFindSongsInPaginatorEvenWithCachedQueryParsing() : void + { + $cache = $this->_em->getConfiguration() + ->getQueryCacheImpl(); + + assert($cache instanceof ClearableCache); + + $cache->deleteAll(); + + $query = $this->_em->getRepository(GH7820Line::class) + ->createQueryBuilder('l') + ->orderBy('l.lineNumber', Criteria::ASC); + + self::assertSame( + self::SONG, + array_map(static function (GH7820Line $line) : string { + return $line->toString(); + }, iterator_to_array(new Paginator($query))), + 'Paginator runs once, query cache is populated with DQL -> SQL translation' + ); + + $query = $this->_em->getRepository(GH7820Line::class) + ->createQueryBuilder('l') + ->orderBy('l.lineNumber', Criteria::ASC); + + self::assertSame( + self::SONG, + array_map(static function (GH7820Line $line) : string { + return $line->toString(); + }, iterator_to_array(new Paginator($query))), + 'Paginator runs again, SQL parameters are translated again, even with cached DQL -> SQL translation' + ); + } } /** @Entity */ From 023e94661a1956215a6ddf17cb1b8908274b0d01 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 10 Oct 2019 18:23:31 +0200 Subject: [PATCH 02/11] #7837 force expiry of query cache when `WhereInWalker` is being used In order to figure out the paginated query identifier type, we would have to parse the DQL query into an AST+SQL anyway, so we'd have to re-parse it manually: instead of doing that, we can force the `WhereInWalker` to be reached at all times by forcing the `$whereInQuery` to use no query cache. While it is a sad performance regression, it is also not a noticeable one, since we'll be performing an `O(1)` operation around an I/O one (query execution, in this case). --- lib/Doctrine/ORM/Tools/Pagination/Paginator.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Doctrine/ORM/Tools/Pagination/Paginator.php b/lib/Doctrine/ORM/Tools/Pagination/Paginator.php index 6c9ee44d03d..08c924bc598 100644 --- a/lib/Doctrine/ORM/Tools/Pagination/Paginator.php +++ b/lib/Doctrine/ORM/Tools/Pagination/Paginator.php @@ -166,6 +166,7 @@ public function getIterator() $whereInQuery->setFirstResult(null)->setMaxResults(null); $whereInQuery->setParameter(WhereInWalker::PAGINATOR_ID_ALIAS, $ids); $whereInQuery->setCacheable($this->query->isCacheable()); + $whereInQuery->expireQueryCache(); $result = $whereInQuery->getResult($this->query->getHydrationMode()); } else { From cc5f84ac22815747a5b9b41bad2c4519674a1684 Mon Sep 17 00:00:00 2001 From: Benjamin Morel Date: Tue, 15 Oct 2019 23:53:12 +0200 Subject: [PATCH 03/11] UnitOfWork::clear() misses $eagerLoadingEntities --- lib/Doctrine/ORM/UnitOfWork.php | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 82695984b65..773eae23b6b 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -2483,22 +2483,23 @@ public function getCommitOrderCalculator() public function clear($entityName = null) { if ($entityName === null) { - $this->identityMap = - $this->entityIdentifiers = - $this->originalEntityData = - $this->entityChangeSets = - $this->entityStates = - $this->scheduledForSynchronization = - $this->entityInsertions = - $this->entityUpdates = - $this->entityDeletions = + $this->identityMap = + $this->entityIdentifiers = + $this->originalEntityData = + $this->entityChangeSets = + $this->entityStates = + $this->scheduledForSynchronization = + $this->entityInsertions = + $this->entityUpdates = + $this->entityDeletions = $this->nonCascadedNewDetectedEntities = - $this->collectionDeletions = - $this->collectionUpdates = - $this->extraUpdates = - $this->readOnlyObjects = - $this->visitedCollections = - $this->orphanRemovals = []; + $this->collectionDeletions = + $this->collectionUpdates = + $this->extraUpdates = + $this->readOnlyObjects = + $this->visitedCollections = + $this->eagerLoadingEntities = + $this->orphanRemovals = []; } else { $this->clearIdentityMapForEntityName($entityName); $this->clearEntityInsertionsForEntityName($entityName); From d1db0655acf2eccec99ad9781ab33727151170b4 Mon Sep 17 00:00:00 2001 From: Mathieu Lemoine Date: Mon, 28 Oct 2019 15:30:56 +0100 Subject: [PATCH 04/11] Update documentation to recommend DQL over QueryBuilder when possible --- docs/en/reference/faq.rst | 15 +++++++++++++++ docs/en/reference/query-builder.rst | 15 ++++++++++----- docs/en/reference/working-with-objects.rst | 4 +++- docs/en/tutorials/getting-started.rst | 3 +-- 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/docs/en/reference/faq.rst b/docs/en/reference/faq.rst index 78b5b01c946..ca7784edca0 100644 --- a/docs/en/reference/faq.rst +++ b/docs/en/reference/faq.rst @@ -198,6 +198,21 @@ No, it is not supported to sort by function in DQL. If you need this functionali use a native-query or come up with another solution. As a side note: Sorting with ORDER BY RAND() is painfully slow starting with 1000 rows. +Is it better to write DQL or to generate it with the query builder? +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The purpose of the ``QueryBuilder`` is to generate DQL dynamically, +which is useful when you have optional filters, conditional joins, etc. + +But the ``QueryBuilder`` is not an alternative to DQL, it actually generates DQL +queries at runtime, which are then interpreted by Doctrine. This means that +using the ``QueryBuilder`` to build and run a query is actually always slower +than only running the corresponding DQL query. + +So if you only need to generate a query and bind parameters to it, +you should use plain DQL, as this is a simpler and much more readable solution. +You should only use the ``QueryBuilder`` when you can't achieve what you want to do with a DQL query. + A Query fails, how can I debug it? ---------------------------------- diff --git a/docs/en/reference/query-builder.rst b/docs/en/reference/query-builder.rst index 18199e7c55f..91833aee53d 100644 --- a/docs/en/reference/query-builder.rst +++ b/docs/en/reference/query-builder.rst @@ -9,6 +9,12 @@ programmatically build queries, and also provides a fluent API. This means that you can change between one methodology to the other as you want, or just pick a preferred one. +.. note:: + + The ``QueryBuilder`` is not an abstraction of DQL, but merely a tool to dynamically build it. + You should still use plain DQL when you can, as it is simpler and more readable. + More about this in the :doc:`FAQ `_. + Constructing a new QueryBuilder object ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -80,7 +86,7 @@ Working with QueryBuilder High level API methods ^^^^^^^^^^^^^^^^^^^^^^ -To simplify even more the way you build a query in Doctrine, you can take +The most straightforward way to build a dynamic query with the ``QueryBuilder`` is by taking advantage of Helper methods. For all base code, there is a set of useful methods to simplify a programmer's life. To illustrate how to work with them, here is the same example 6 re-written using @@ -97,10 +103,9 @@ to work with them, here is the same example 6 re-written using ->orderBy('u.name', 'ASC'); ``QueryBuilder`` helper methods are considered the standard way to -build DQL queries. Although it is supported, using string-based -queries should be avoided. You are greatly encouraged to use -``$qb->expr()->*`` methods. Here is a converted example 8 to -suggested standard way to build queries: +use the ``QueryBuilder``. The ``$qb->expr()->*`` methods can help you +build conditional expressions dynamically. Here is a converted example 8 to +suggested way to build queries with dynamic conditions: .. code-block:: php diff --git a/docs/en/reference/working-with-objects.rst b/docs/en/reference/working-with-objects.rst index 064c9e1f9e8..4b16fc3579d 100644 --- a/docs/en/reference/working-with-objects.rst +++ b/docs/en/reference/working-with-objects.rst @@ -800,7 +800,9 @@ DQL and its syntax as well as the Doctrine class can be found in :doc:`the dedicated chapter `. For programmatically building up queries based on conditions that are only known at runtime, Doctrine provides the special -``Doctrine\ORM\QueryBuilder`` class. More information on +``Doctrine\ORM\QueryBuilder`` class. While this a powerful tool, +it also brings more complexity to your code compared to plain DQL, +so you should only use it when you need it. More information on constructing queries with a QueryBuilder can be found :doc:`in Query Builder chapter `. diff --git a/docs/en/tutorials/getting-started.rst b/docs/en/tutorials/getting-started.rst index b01d44d4339..52190e57099 100644 --- a/docs/en/tutorials/getting-started.rst +++ b/docs/en/tutorials/getting-started.rst @@ -1210,8 +1210,7 @@ The console output of this script is then: throw your ORM into the dumpster, because it doesn't support some the more powerful SQL concepts. - - Instead of handwriting DQL you can use the ``QueryBuilder`` retrieved + If you need to build your query dynamically, you can use the ``QueryBuilder`` retrieved by calling ``$entityManager->createQueryBuilder()``. There are more details about this in the relevant part of the documentation. From 19aa3c125c5b8b5853b5dd4284eb9306a04f9206 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rog=C3=A9rio=20Alencar=20Lino=20Filho?= Date: Thu, 31 Oct 2019 18:20:58 -0300 Subject: [PATCH 05/11] missing entity alias --- ...vanced-field-value-conversion-using-custom-mapping-types.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/cookbook/advanced-field-value-conversion-using-custom-mapping-types.rst b/docs/en/cookbook/advanced-field-value-conversion-using-custom-mapping-types.rst index e5ca21632aa..f735d99f46b 100644 --- a/docs/en/cookbook/advanced-field-value-conversion-using-custom-mapping-types.rst +++ b/docs/en/cookbook/advanced-field-value-conversion-using-custom-mapping-types.rst @@ -249,7 +249,7 @@ Example usage $em->clear(); // Fetch the Location object - $query = $em->createQuery("SELECT l FROM Geo\Entity\Location WHERE l.address = '1600 Amphitheatre Parkway, Mountain View, CA'"); + $query = $em->createQuery("SELECT l FROM Geo\Entity\Location l WHERE l.address = '1600 Amphitheatre Parkway, Mountain View, CA'"); $location = $query->getSingleResult(); /* @var Geo\ValueObject\Point */ From 1bc4e1f594de5550f0d67fe073a0b6c53096d082 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20J=2E=20Garc=C3=ADa=20Lagar?= Date: Tue, 5 Nov 2019 14:10:32 +0100 Subject: [PATCH 06/11] Use quoted collation declaration when available. --- .../SchemaTool/MySqlSchemaToolTest.php | 36 +++++++++++++------ .../ORM/Functional/Ticket/DDC2182Test.php | 17 +++++++-- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/SchemaTool/MySqlSchemaToolTest.php b/tests/Doctrine/Tests/ORM/Functional/SchemaTool/MySqlSchemaToolTest.php index 2deedb37aa6..829b3aa2b85 100644 --- a/tests/Doctrine/Tests/ORM/Functional/SchemaTool/MySqlSchemaToolTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/SchemaTool/MySqlSchemaToolTest.php @@ -5,6 +5,8 @@ use Doctrine\ORM\Tools\SchemaTool; use Doctrine\Tests\OrmFunctionalTestCase; use Doctrine\Tests\Models; +use function method_exists; +use function sprintf; class MySqlSchemaToolTest extends OrmFunctionalTestCase { @@ -28,15 +30,16 @@ public function testGetCreateSchemaSql() $tool = new SchemaTool($this->_em); $sql = $tool->getCreateSchemaSql($classes); - - $this->assertEquals("CREATE TABLE cms_groups (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(50) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[0]); - $this->assertEquals("CREATE TABLE cms_users (id INT AUTO_INCREMENT NOT NULL, email_id INT DEFAULT NULL, status VARCHAR(50) DEFAULT NULL, username VARCHAR(255) NOT NULL, name VARCHAR(255) NOT NULL, UNIQUE INDEX UNIQ_3AF03EC5F85E0677 (username), UNIQUE INDEX UNIQ_3AF03EC5A832C1C9 (email_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[1]); - $this->assertEquals("CREATE TABLE cms_users_groups (user_id INT NOT NULL, group_id INT NOT NULL, INDEX IDX_7EA9409AA76ED395 (user_id), INDEX IDX_7EA9409AFE54D947 (group_id), PRIMARY KEY(user_id, group_id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[2]); - $this->assertEquals("CREATE TABLE cms_users_tags (user_id INT NOT NULL, tag_id INT NOT NULL, INDEX IDX_93F5A1ADA76ED395 (user_id), INDEX IDX_93F5A1ADBAD26311 (tag_id), PRIMARY KEY(user_id, tag_id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[3]); - $this->assertEquals("CREATE TABLE cms_tags (id INT AUTO_INCREMENT NOT NULL, tag_name VARCHAR(50) DEFAULT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[4]); - $this->assertEquals("CREATE TABLE cms_addresses (id INT AUTO_INCREMENT NOT NULL, user_id INT DEFAULT NULL, country VARCHAR(50) NOT NULL, zip VARCHAR(50) NOT NULL, city VARCHAR(50) NOT NULL, UNIQUE INDEX UNIQ_ACAC157BA76ED395 (user_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[5]); - $this->assertEquals("CREATE TABLE cms_emails (id INT AUTO_INCREMENT NOT NULL, email VARCHAR(250) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[6]); - $this->assertEquals("CREATE TABLE cms_phonenumbers (phonenumber VARCHAR(50) NOT NULL, user_id INT DEFAULT NULL, INDEX IDX_F21F790FA76ED395 (user_id), PRIMARY KEY(phonenumber)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[7]); + $collation = $this->getColumnCollationDeclarationSQL('utf8_unicode_ci'); + + $this->assertEquals('CREATE TABLE cms_groups (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(50) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[0]); + $this->assertEquals('CREATE TABLE cms_users (id INT AUTO_INCREMENT NOT NULL, email_id INT DEFAULT NULL, status VARCHAR(50) DEFAULT NULL, username VARCHAR(255) NOT NULL, name VARCHAR(255) NOT NULL, UNIQUE INDEX UNIQ_3AF03EC5F85E0677 (username), UNIQUE INDEX UNIQ_3AF03EC5A832C1C9 (email_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[1]); + $this->assertEquals('CREATE TABLE cms_users_groups (user_id INT NOT NULL, group_id INT NOT NULL, INDEX IDX_7EA9409AA76ED395 (user_id), INDEX IDX_7EA9409AFE54D947 (group_id), PRIMARY KEY(user_id, group_id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[2]); + $this->assertEquals('CREATE TABLE cms_users_tags (user_id INT NOT NULL, tag_id INT NOT NULL, INDEX IDX_93F5A1ADA76ED395 (user_id), INDEX IDX_93F5A1ADBAD26311 (tag_id), PRIMARY KEY(user_id, tag_id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[3]); + $this->assertEquals('CREATE TABLE cms_tags (id INT AUTO_INCREMENT NOT NULL, tag_name VARCHAR(50) DEFAULT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[4]); + $this->assertEquals('CREATE TABLE cms_addresses (id INT AUTO_INCREMENT NOT NULL, user_id INT DEFAULT NULL, country VARCHAR(50) NOT NULL, zip VARCHAR(50) NOT NULL, city VARCHAR(50) NOT NULL, UNIQUE INDEX UNIQ_ACAC157BA76ED395 (user_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[5]); + $this->assertEquals('CREATE TABLE cms_emails (id INT AUTO_INCREMENT NOT NULL, email VARCHAR(250) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[6]); + $this->assertEquals('CREATE TABLE cms_phonenumbers (phonenumber VARCHAR(50) NOT NULL, user_id INT DEFAULT NULL, INDEX IDX_F21F790FA76ED395 (user_id), PRIMARY KEY(phonenumber)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[7]); $this->assertEquals("ALTER TABLE cms_users ADD CONSTRAINT FK_3AF03EC5A832C1C9 FOREIGN KEY (email_id) REFERENCES cms_emails (id)", $sql[8]); $this->assertEquals("ALTER TABLE cms_users_groups ADD CONSTRAINT FK_7EA9409AA76ED395 FOREIGN KEY (user_id) REFERENCES cms_users (id)", $sql[9]); $this->assertEquals("ALTER TABLE cms_users_groups ADD CONSTRAINT FK_7EA9409AFE54D947 FOREIGN KEY (group_id) REFERENCES cms_groups (id)", $sql[10]); @@ -48,6 +51,15 @@ public function testGetCreateSchemaSql() $this->assertEquals(15, count($sql)); } + private function getColumnCollationDeclarationSQL(string $collation) : string + { + if (method_exists($this->_em->getConnection()->getDatabasePlatform(), 'getColumnCollationDeclarationSQL')) { + return $this->_em->getConnection()->getDatabasePlatform()->getColumnCollationDeclarationSQL($collation); + } + + return sprintf('COLLATE %s', $collation); + } + public function testGetCreateSchemaSql2() { $classes = [ @@ -56,9 +68,10 @@ public function testGetCreateSchemaSql2() $tool = new SchemaTool($this->_em); $sql = $tool->getCreateSchemaSql($classes); + $collation = $this->getColumnCollationDeclarationSQL('utf8_unicode_ci'); $this->assertEquals(1, count($sql)); - $this->assertEquals("CREATE TABLE decimal_model (id INT AUTO_INCREMENT NOT NULL, `decimal` NUMERIC(5, 2) NOT NULL, `high_scale` NUMERIC(14, 4) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[0]); + $this->assertEquals('CREATE TABLE decimal_model (id INT AUTO_INCREMENT NOT NULL, `decimal` NUMERIC(5, 2) NOT NULL, `high_scale` NUMERIC(14, 4) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[0]); } public function testGetCreateSchemaSql3() @@ -69,9 +82,10 @@ public function testGetCreateSchemaSql3() $tool = new SchemaTool($this->_em); $sql = $tool->getCreateSchemaSql($classes); + $collation = $this->getColumnCollationDeclarationSQL('utf8_unicode_ci'); $this->assertEquals(1, count($sql)); - $this->assertEquals("CREATE TABLE boolean_model (id INT AUTO_INCREMENT NOT NULL, booleanField TINYINT(1) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[0]); + $this->assertEquals('CREATE TABLE boolean_model (id INT AUTO_INCREMENT NOT NULL, booleanField TINYINT(1) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[0]); } /** diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2182Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2182Test.php index 590b7cd5f58..e610a67bcb2 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2182Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2182Test.php @@ -2,6 +2,9 @@ namespace Doctrine\Tests\ORM\Functional\Ticket; +use function method_exists; +use function sprintf; + class DDC2182Test extends \Doctrine\Tests\OrmFunctionalTestCase { public function testPassColumnOptionsToJoinColumns() @@ -16,11 +19,21 @@ public function testPassColumnOptionsToJoinColumns() $this->_em->getClassMetadata(DDC2182OptionChild::class), ] ); + $collation = $this->getColumnCollationDeclarationSQL('utf8_unicode_ci'); - $this->assertEquals("CREATE TABLE DDC2182OptionParent (id INT UNSIGNED NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[0]); - $this->assertEquals("CREATE TABLE DDC2182OptionChild (id VARCHAR(255) NOT NULL, parent_id INT UNSIGNED DEFAULT NULL, INDEX IDX_B314D4AD727ACA70 (parent_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB", $sql[1]); + $this->assertEquals('CREATE TABLE DDC2182OptionParent (id INT UNSIGNED NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[0]); + $this->assertEquals('CREATE TABLE DDC2182OptionChild (id VARCHAR(255) NOT NULL, parent_id INT UNSIGNED DEFAULT NULL, INDEX IDX_B314D4AD727ACA70 (parent_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 ' . $collation . ' ENGINE = InnoDB', $sql[1]); $this->assertEquals("ALTER TABLE DDC2182OptionChild ADD CONSTRAINT FK_B314D4AD727ACA70 FOREIGN KEY (parent_id) REFERENCES DDC2182OptionParent (id)", $sql[2]); } + + private function getColumnCollationDeclarationSQL(string $collation) : string + { + if (method_exists($this->_em->getConnection()->getDatabasePlatform(), 'getColumnCollationDeclarationSQL')) { + return $this->_em->getConnection()->getDatabasePlatform()->getColumnCollationDeclarationSQL($collation); + } + + return sprintf('COLLATE %s', $collation); + } } /** From 1dde2c9e8ee144d634ecf93a7e971d62633fc5d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Ostroluck=C3=BD?= Date: Thu, 14 Nov 2019 22:17:06 +0100 Subject: [PATCH 07/11] Add test case verifying eager loads are clear Otherwise, getClassMetadata would be triggered more times --- .../ORM/Functional/Ticket/GH7869Test.php | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH7869Test.php diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7869Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7869Test.php new file mode 100644 index 00000000000..db16e38967e --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7869Test.php @@ -0,0 +1,67 @@ +getMockBuilder(EntityManagerDecorator::class) + ->setConstructorArgs([$decoratedEm]) + ->setMethods(['getClassMetadata']) + ->getMock(); + + $em->expects($this->exactly(2)) + ->method('getClassMetadata') + ->willReturnCallback([$decoratedEm, 'getClassMetadata']); + + $hints = [ + UnitOfWork::HINT_DEFEREAGERLOAD => true, + 'fetchMode' => [GH7869Appointment::class => ['patient' => ClassMetadata::FETCH_EAGER]], + ]; + + $uow = new UnitOfWork($em); + $uow->createEntity(GH7869Appointment::class, ['id' => 1, 'patient_id' => 1], $hints); + $uow->clear(); + $uow->triggerEagerLoads(); + } +} + +/** + * @Entity + */ +class GH7869Appointment +{ + /** @Id @Column(type="integer") @GeneratedValue */ + public $id; + + /** @OneToOne(targetEntity="GH7869Patient", inversedBy="appointment", fetch="EAGER") */ + public $patient; +} + +/** + * @Entity + */ +class GH7869Patient +{ + /** @Id @Column(type="integer") @GeneratedValue */ + public $id; + + /** @OneToOne(targetEntity="GH7869Appointment", mappedBy="patient") */ + public $appointment; +} From 98e557b68eeecd7f266f1e4628007aed2714026c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Ostroluck=C3=BD?= Date: Thu, 14 Nov 2019 23:36:15 +0100 Subject: [PATCH 08/11] Improve assertion failure message for testWillFindSongsInPaginatorEvenWithCachedQueryParsing --- tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php index afb29f08369..11110e5a73e 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php @@ -98,7 +98,7 @@ public function testWillFindSongsInPaginatorEvenWithCachedQueryParsing() : void array_map(static function (GH7820Line $line) : string { return $line->toString(); }, iterator_to_array(new Paginator($query))), - 'Paginator runs once, query cache is populated with DQL -> SQL translation' + 'Expected to return expected data before query cache is populated with DQL -> SQL translation. Were SQL parameters translated?' ); $query = $this->_em->getRepository(GH7820Line::class) @@ -110,7 +110,7 @@ public function testWillFindSongsInPaginatorEvenWithCachedQueryParsing() : void array_map(static function (GH7820Line $line) : string { return $line->toString(); }, iterator_to_array(new Paginator($query))), - 'Paginator runs again, SQL parameters are translated again, even with cached DQL -> SQL translation' + 'Expected to return expected data even when DQL -> SQL translation is present in cache. Were SQL parameters translated again?' ); } } From 982d1519db80bca3342014546c173df9900f712b Mon Sep 17 00:00:00 2001 From: Robert den Harink Date: Wed, 17 Apr 2019 12:51:24 +0200 Subject: [PATCH 09/11] only replace '_id' at end of columnName --- .../ORM/Mapping/Driver/DatabaseDriver.php | 3 +- .../ORM/Functional/Ticket/GH7684Test.php | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH7684Test.php diff --git a/lib/Doctrine/ORM/Mapping/Driver/DatabaseDriver.php b/lib/Doctrine/ORM/Mapping/Driver/DatabaseDriver.php index ddaff9d4344..83798e16e21 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/DatabaseDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/DatabaseDriver.php @@ -29,6 +29,7 @@ use Doctrine\DBAL\Types\Type; use Doctrine\ORM\Mapping\ClassMetadataInfo; use Doctrine\ORM\Mapping\MappingException; +use function preg_replace; /** * The DatabaseDriver reverse engineers the mapping metadata from a database. @@ -548,7 +549,7 @@ private function getFieldNameForColumn($tableName, $columnName, $fk = false) // Replace _id if it is a foreignkey column if ($fk) { - $columnName = str_replace('_id', '', $columnName); + $columnName = preg_replace('/_id$/', '', $columnName); } return Inflector::camelize($columnName); diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7684Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7684Test.php new file mode 100644 index 00000000000..2d833036619 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7684Test.php @@ -0,0 +1,38 @@ +_em->getConnection()->getDatabasePlatform()->supportsForeignKeyConstraints()) { + $this->markTestSkipped('Platform does not support foreign keys.'); + } + + $table1 = new Table('GH7684_identity_test_table'); + $table1->addColumn('id', 'integer'); + $table1->setPrimaryKey(['id']); + + $table2 = new Table('GH7684_identity_test_assoc_table'); + $table2->addColumn('id', 'integer'); + $table2->addColumn('gh7684_identity_test_id', 'integer'); + $table2->setPrimaryKey(['id']); + $table2->addForeignKeyConstraint('GH7684_identity_test', ['gh7684_identity_test_id'], ['id']); + + $metadatas = $this->convertToClassMetadata([$table1, $table2]); + $metadata = $metadatas['Gh7684IdentityTestAssocTable']; + + $this->assertArrayHasKey('gh7684IdentityTest', $metadata->associationMappings); + } +} From ec930147139a9557e975220256cc7c53ffbc54c5 Mon Sep 17 00:00:00 2001 From: Ferran Vidal Date: Wed, 9 Oct 2019 11:43:24 +0200 Subject: [PATCH 10/11] Delete statements will not be created using `clear`. --- .../Tests/ORM/Functional/Ticket/GH7761Test.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php index 53215cf0546..c6149465e0c 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php @@ -31,6 +31,12 @@ protected function setUp() : void $this->_em->clear(); } + protected function tearDown() + { + $this->_em->clear(); + parent::tearDown(); + } + public function testCollectionClearDoesNotClearIfNotPersisted() : void { /** @var GH7761Entity $entity */ @@ -43,8 +49,20 @@ public function testCollectionClearDoesNotClearIfNotPersisted() : void $entity = $this->_em->find(GH7761Entity::class, 1); self::assertCount(1, $entity->children); + } + + public function testCollectionClearDoesClearIfPersisted() : void + { + /** @var GH7761Entity $entity */ + $entity = $this->_em->find(GH7761Entity::class, 1); + $entity->children->clear(); + $this->_em->persist($entity); + $this->_em->flush(); $this->_em->clear(); + + $entity = $this->_em->find(GH7761Entity::class, 1); + self::assertCount(0, $entity->children); } } From 7d7798430603bec154ff99329eb62633e1486620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Ostroluck=C3=BD?= Date: Fri, 15 Nov 2019 21:56:27 +0100 Subject: [PATCH 11/11] Restore ability to clear deferred explicit tracked collections This was regression from #7862 which tried to respect tracking config when clearing collections, but this logic can happen in UOW only, PersistentCollection::clear is triggered too early to know what is (going to be) persisted. Fixes #7862 --- lib/Doctrine/ORM/PersistentCollection.php | 4 +--- lib/Doctrine/ORM/UnitOfWork.php | 14 +++++++++++++- .../Tests/ORM/Functional/Ticket/GH7761Test.php | 9 +++------ .../Tests/ORM/PersistentCollectionTest.php | 12 ------------ 4 files changed, 17 insertions(+), 22 deletions(-) diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php index c49e5e5f203..7d52d5392af 100644 --- a/lib/Doctrine/ORM/PersistentCollection.php +++ b/lib/Doctrine/ORM/PersistentCollection.php @@ -566,9 +566,7 @@ public function clear() if ($this->association['isOwningSide'] && $this->owner) { $this->changed(); - if (! $this->em->getClassMetadata(get_class($this->owner))->isChangeTrackingDeferredExplicit()) { - $uow->scheduleCollectionDeletion($this); - } + $uow->scheduleCollectionDeletion($this); $this->takeSnapshot(); } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 773eae23b6b..b27e7405a84 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -46,6 +46,7 @@ use InvalidArgumentException; use Throwable; use UnexpectedValueException; +use function get_class; /** * The UnitOfWork is responsible for tracking changes to objects during an @@ -380,7 +381,18 @@ public function commit($entity = null) try { // Collection deletions (deletions of complete collections) foreach ($this->collectionDeletions as $collectionToDelete) { - $this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete); + if (! $collectionToDelete instanceof PersistentCollection) { + $this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete); + + continue; + } + + // Deferred explicit tracked collections can be removed only when owning relation was persisted + $owner = $collectionToDelete->getOwner(); + + if ($this->em->getClassMetadata(get_class($owner))->isChangeTrackingDeferredImplicit() || $this->isScheduledForDirtyCheck($owner)) { + $this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete); + } } if ($this->entityInsertions) { diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php index c6149465e0c..fcf3e3be7d2 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7761Test.php @@ -31,12 +31,6 @@ protected function setUp() : void $this->_em->clear(); } - protected function tearDown() - { - $this->_em->clear(); - parent::tearDown(); - } - public function testCollectionClearDoesNotClearIfNotPersisted() : void { /** @var GH7761Entity $entity */ @@ -51,6 +45,9 @@ public function testCollectionClearDoesNotClearIfNotPersisted() : void self::assertCount(1, $entity->children); } + /** + * @group GH-7862 + */ public function testCollectionClearDoesClearIfPersisted() : void { /** @var GH7761Entity $entity */ diff --git a/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php b/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php index 179d6e9d517..bfee130f0b5 100644 --- a/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php @@ -274,16 +274,4 @@ public function testModifyUOWForDeferredImplicitOwnerOnClear() : void $this->collection->clear(); } - - public function testDoNotModifyUOWForDeferredExplicitOwnerOnClear() : void - { - $unitOfWork = $this->createMock(UnitOfWork::class); - $unitOfWork->expects(self::never())->method('scheduleCollectionDeletion'); - $this->_emMock->setUnitOfWork($unitOfWork); - - $classMetaData = $this->_emMock->getClassMetadata(ECommerceCart::class); - $classMetaData->setChangeTrackingPolicy(ClassMetadataInfo::CHANGETRACKING_DEFERRED_EXPLICIT); - - $this->collection->clear(); - } }