From 52b3e219698e83d2dbed71d9c735ccc51afec273 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 28 Jan 2015 20:10:22 -0500 Subject: [PATCH 1/4] Only getting the target platform when it's *actually* needed to avoid errors initialize() is called sometimes, even when the following code doesn't need the targetPlatform property. Specifically, in AbstractClassMetadataFactory::getAllMetadata(). But as of DBAL 2.5.0, calling Connection::getDatabasePlatform() will make a connection to the database, which means that sometimes it may fail (e.g. you haven't configured your database yet). As a result, calling a method like AbstractClassMetadataFactory::getAllMetadata() - which does not need the targetPlatform - will fail, because determining the targetPlatform requires a connection, which fails. This avoids that - we only get the targetPlatform *when* we need it, which are cases where we're doing things that do indeed need a connection. --- .../ORM/Mapping/ClassMetadataFactory.php | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index 9320d1b6d..1c53955d5 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -78,7 +78,6 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory protected function initialize() { $this->driver = $this->em->getConfiguration()->getMetadataDriverImpl(); - $this->targetPlatform = $this->em->getConnection()->getDatabasePlatform(); $this->evm = $this->em->getEventManager(); $this->initialized = true; } @@ -432,9 +431,9 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory { $idGenType = $class->generatorType; if ($idGenType == ClassMetadata::GENERATOR_TYPE_AUTO) { - if ($this->targetPlatform->prefersSequences()) { + if ($this->getTargetPlatform()->prefersSequences()) { $class->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_SEQUENCE); - } else if ($this->targetPlatform->prefersIdentityColumns()) { + } else if ($this->getTargetPlatform()->prefersIdentityColumns()) { $class->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY); } else { $class->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_TABLE); @@ -450,19 +449,25 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory $sequenceName = null; $fieldName = $class->identifier ? $class->getSingleIdentifierFieldName() : null; - if ($this->targetPlatform instanceof Platforms\PostgreSQLPlatform) { + // Platforms that do not have native IDENTITY support need a sequence to emulate this behaviour. + if ($this->getTargetPlatform()->usesSequenceEmulatedIdentityColumns()) { $columnName = $class->getSingleIdentifierColumnName(); $quoted = isset($class->fieldMappings[$fieldName]['quoted']) || isset($class->table['quoted']); - $sequenceName = $class->getTableName() . '_' . $columnName . '_seq'; + $sequencePrefix = $class->getSequencePrefix($this->getTargetPlatform()); + $sequenceName = $this->getTargetPlatform()->getIdentitySequenceName($sequencePrefix, $columnName); $definition = array( - 'sequenceName' => $this->targetPlatform->fixSchemaElementName($sequenceName) + 'sequenceName' => $this->getTargetPlatform()->fixSchemaElementName($sequenceName) ); if ($quoted) { $definition['quoted'] = true; } - $sequenceName = $this->em->getConfiguration()->getQuoteStrategy()->getSequenceName($definition, $class, $this->targetPlatform); + $sequenceName = $this + ->em + ->getConfiguration() + ->getQuoteStrategy() + ->getSequenceName($definition, $class, $this->getTargetPlatform()); } $generator = ($fieldName && $class->fieldMappings[$fieldName]['type'] === 'bigint') @@ -479,11 +484,11 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory if ( ! $definition) { $fieldName = $class->getSingleIdentifierFieldName(); - $columnName = $class->getSingleIdentifierColumnName(); + $sequenceName = $class->getSequenceName($this->getTargetPlatform()); $quoted = isset($class->fieldMappings[$fieldName]['quoted']) || isset($class->table['quoted']); - $sequenceName = $class->getTableName() . '_' . $columnName . '_seq'; - $definition = array( - 'sequenceName' => $this->targetPlatform->fixSchemaElementName($sequenceName), + + $definition = array( + 'sequenceName' => $this->getTargetPlatform()->fixSchemaElementName($sequenceName), 'allocationSize' => 1, 'initialValue' => 1, ); @@ -496,7 +501,7 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory } $sequenceGenerator = new \Doctrine\ORM\Id\SequenceGenerator( - $this->em->getConfiguration()->getQuoteStrategy()->getSequenceName($definition, $class, $this->targetPlatform), + $this->em->getConfiguration()->getQuoteStrategy()->getSequenceName($definition, $class, $this->getTargetPlatform()), $definition['allocationSize'] ); $class->setIdGenerator($sequenceGenerator); @@ -569,4 +574,13 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory { return isset($class->isMappedSuperclass) && $class->isMappedSuperclass === false; } + + private function getTargetPlatform() + { + if ($this->targetPlatform === null) { + $this->targetPlatform = $this->em->getConnection()->getDatabasePlatform(); + } + + return $this->targetPlatform; + } } From 9d7256aace79c6ece96772a72bd457caf69a7585 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 4 Feb 2015 13:49:51 -0500 Subject: [PATCH 2/4] Small code change thanks to the comments and adding a test --- .../ORM/Mapping/ClassMetadataFactory.php | 2 +- .../ORM/Mapping/ClassMetadataFactoryTest.php | 29 +++++++++++++++++-- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index 1c53955d5..9156b6b85 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -577,7 +577,7 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory private function getTargetPlatform() { - if ($this->targetPlatform === null) { + if (!$this->targetPlatform) { $this->targetPlatform = $this->em->getConnection()->getDatabasePlatform(); } diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php index 7f07af2ad..d865a886c 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php @@ -192,15 +192,38 @@ class ClassMetadataFactoryTest extends \Doctrine\Tests\OrmTestCase $rootMetadata = $cmf->getMetadataFor('Doctrine\Tests\Models\JoinedInheritanceType\RootClass'); } - protected function _createEntityManager($metadataDriver) + public function testGetAllMetadataWorksWithBadConnection() + { + // DDC-3551 + $conn = $this->getMockBuilder('Doctrine\DBAL\Connection') + ->disableOriginalConstructor() + ->getMock(); + $mockDriver = new MetadataDriverMock(); + $em = $this->_createEntityManager($mockDriver, $conn); + + $conn->expects($this->any()) + ->method('getDatabasePlatform') + ->will($this->throwException(new \Exception('Exception thrown in test when calling getDatabasePlatform'))); + + $cmf = new ClassMetadataFactory(); + $cmf->setEntityManager($em); + + // getting all the metadata should work, even if get DatabasePlatform blows up + $metadata = $cmf->getAllMetadata(); + // this will just be an empty array - there was no error + $this->assertEquals(array(), $metadata); + } + + protected function _createEntityManager($metadataDriver, $conn = null) { $driverMock = new DriverMock(); $config = new \Doctrine\ORM\Configuration(); $config->setProxyDir(__DIR__ . '/../../Proxies'); $config->setProxyNamespace('Doctrine\Tests\Proxies'); $eventManager = new EventManager(); - $conn = new ConnectionMock(array(), $driverMock, $config, $eventManager); - $mockDriver = new MetadataDriverMock(); + if (!$conn) { + $conn = new ConnectionMock(array(), $driverMock, $config, $eventManager); + } $config->setMetadataDriverImpl($metadataDriver); return EntityManagerMock::create($conn, $config, $eventManager); From 2e9ffe831c62fda64234ab1fa6b16da19770deef Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 4 Feb 2015 13:51:02 -0500 Subject: [PATCH 3/4] Adding docblock --- lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index 9156b6b85..2845e720c 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -575,6 +575,9 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory return isset($class->isMappedSuperclass) && $class->isMappedSuperclass === false; } + /** + * @return Platforms\AbstractPlatform + */ private function getTargetPlatform() { if (!$this->targetPlatform) { From db06355b636fe7b36e614eb3587e5063e7a64f47 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 4 Feb 2015 23:46:15 +0000 Subject: [PATCH 4/4] #1294 - fixing differences between 2.5 and 2.4 fixes (reverts DBAL dependency bump) --- lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index 2845e720c..caba0b637 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -449,12 +449,10 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory $sequenceName = null; $fieldName = $class->identifier ? $class->getSingleIdentifierFieldName() : null; - // Platforms that do not have native IDENTITY support need a sequence to emulate this behaviour. - if ($this->getTargetPlatform()->usesSequenceEmulatedIdentityColumns()) { + if ($this->getTargetPlatform() instanceof Platforms\PostgreSQLPlatform) { $columnName = $class->getSingleIdentifierColumnName(); $quoted = isset($class->fieldMappings[$fieldName]['quoted']) || isset($class->table['quoted']); - $sequencePrefix = $class->getSequencePrefix($this->getTargetPlatform()); - $sequenceName = $this->getTargetPlatform()->getIdentitySequenceName($sequencePrefix, $columnName); + $sequenceName = $class->getTableName() . '_' . $columnName . '_seq'; $definition = array( 'sequenceName' => $this->getTargetPlatform()->fixSchemaElementName($sequenceName) ); @@ -484,10 +482,10 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory if ( ! $definition) { $fieldName = $class->getSingleIdentifierFieldName(); - $sequenceName = $class->getSequenceName($this->getTargetPlatform()); + $columnName = $class->getSingleIdentifierColumnName(); $quoted = isset($class->fieldMappings[$fieldName]['quoted']) || isset($class->table['quoted']); - - $definition = array( + $sequenceName = $class->getTableName() . '_' . $columnName . '_seq'; + $definition = array( 'sequenceName' => $this->getTargetPlatform()->fixSchemaElementName($sequenceName), 'allocationSize' => 1, 'initialValue' => 1,