From d5ee21218fbd74e83193aa0061491f504db33a6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Fri, 20 Aug 2021 20:39:26 +0200 Subject: [PATCH] Deprecate reliance on non-optimal defaults What was optimal 10 years ago no longer is, and things might change in the future. Using AUTO is still the best solution in most cases, and it should be easy to make it mean something else when it is not. --- UPGRADE.md | 27 +++++++ docs/en/reference/basic-mapping.rst | 39 +++++++--- lib/Doctrine/ORM/Configuration.php | 17 +++++ .../ORM/Mapping/ClassMetadataFactory.php | 73 +++++++++++++++++-- phpstan-baseline.neon | 5 ++ phpstan-dbal2.neon | 5 ++ psalm-baseline.xml | 7 +- .../ORM/Mapping/ClassMetadataFactoryTest.php | 66 +++++++++++++++++ 8 files changed, 221 insertions(+), 18 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index 77c8529e7ce..1a3dd4ac610 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,32 @@ # Upgrade to 2.12 +## Deprecated: reliance on the non-optimal defaults that come with the `AUTO` identifier generation strategy + +When the `AUTO` identifier generation strategy was introduced, the best +strategy at the time was selected for each database platform. +A lot of time has passed since then, and support for better strategies has been +added. + +Because of that, it is now deprecated to rely on the historical defaults when +they differ from what we recommend now. + +Instead, you should pick a strategy for each database platform you use, and it +will be used when using `AUTO`. As of now, only PostgreSQL is affected by this. +It is recommended that PostgreSQL user configure their new applications to use +`IDENTITY`: + +```php +use Doctrine\DBAL\Platforms\PostgreSQLPlatform; +use Doctrine\ORM\Configuration; + +assert($configuration instanceof Configuration); +$configuration->setIdentityGenerationPreferences([ + PostgreSQLPlatform::CLASS => ClassMetadata::GENERATOR_TYPE_IDENTITY, +]); +``` + +If migrating an existing application is too costly, the deprecation can be +addressed by configuring `SEQUENCE` as the default strategy. ## Deprecate `Doctrine\ORM\Cache\MultiGetRegion` The interface will be merged with `Doctrine\ORM\Cache\Region` in 3.0. diff --git a/docs/en/reference/basic-mapping.rst b/docs/en/reference/basic-mapping.rst index b17a968e3d3..0f9aa8c4070 100644 --- a/docs/en/reference/basic-mapping.rst +++ b/docs/en/reference/basic-mapping.rst @@ -358,9 +358,27 @@ annotation. # fields here In most cases using the automatic generator strategy (``@GeneratedValue``) is -what you want. It defaults to the identifier generation mechanism your current -database vendor prefers: AUTO_INCREMENT with MySQL, sequences with PostgreSQL -and Oracle and so on. +what you want, but for backwards-compatibility reasons it might not. It +defaults to the identifier generation mechanism your current database +vendor preferred at the time that strategy was introduced: +``AUTO_INCREMENT`` with MySQL, sequences with PostgreSQL and Oracle and +so on. +We now recommend using ``IDENTITY`` for PostgreSQL and Oracle, and you +can achieve that while still using the ``AUTO`` strategy, by configuring +what it defaults to. + +.. code-block:: php + + setIdentityGenerationPreferences([ + OraclePlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + PostgreSQLPlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + ]); .. _identifier-generation-strategies: @@ -377,17 +395,18 @@ Here is the list of possible generation strategies: - ``AUTO`` (default): Tells Doctrine to pick the strategy that is preferred by the used database platform. The preferred strategies - are IDENTITY for MySQL, SQLite, MsSQL and SQL Anywhere and SEQUENCE - for Oracle and PostgreSQL. This strategy provides full portability. -- ``SEQUENCE``: Tells Doctrine to use a database sequence for ID - generation. This strategy does currently not provide full - portability. Sequences are supported by Oracle, PostgreSql and - SQL Anywhere. + are ``IDENTITY`` for MySQL, SQLite, MsSQL and SQL Anywhere and, for + historical reasons, ``SEQUENCE`` for Oracle and PostgreSQL. This + strategy provides full portability. - ``IDENTITY``: Tells Doctrine to use special identity columns in the database that generate a value on insertion of a row. This strategy does currently not provide full portability and is supported by the following platforms: MySQL/SQLite/SQL Anywhere - (AUTO\_INCREMENT), MSSQL (IDENTITY) and PostgreSQL (SERIAL). + (``AUTO_INCREMENT``), MSSQL (``IDENTITY``) and PostgreSQL (``SERIAL``). +- ``SEQUENCE``: Tells Doctrine to use a database sequence for ID + generation. This strategy does currently not provide full + portability. Sequences are supported by Oracle, PostgreSql and + SQL Anywhere. - ``UUID`` (deprecated): Tells Doctrine to use the built-in Universally Unique Identifier generator. This strategy provides full portability. - ``NONE``: Tells Doctrine that the identifiers are assigned (and diff --git a/lib/Doctrine/ORM/Configuration.php b/lib/Doctrine/ORM/Configuration.php index 8c8fd985684..7ad029ca58d 100644 --- a/lib/Doctrine/ORM/Configuration.php +++ b/lib/Doctrine/ORM/Configuration.php @@ -13,6 +13,7 @@ use Doctrine\Common\Cache\Psr6\CacheAdapter; use Doctrine\Common\Cache\Psr6\DoctrineProvider; use Doctrine\Common\Proxy\AbstractProxyFactory; +use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\Deprecations\Deprecation; use Doctrine\ORM\Cache\CacheConfiguration; use Doctrine\ORM\Cache\Exception\CacheException; @@ -26,6 +27,7 @@ use Doctrine\ORM\Exception\ProxyClassesAlwaysRegenerating; use Doctrine\ORM\Exception\UnknownEntityNamespace; use Doctrine\ORM\Internal\Hydration\AbstractHydrator; +use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Mapping\ClassMetadataFactory; use Doctrine\ORM\Mapping\DefaultEntityListenerResolver; use Doctrine\ORM\Mapping\DefaultNamingStrategy; @@ -58,6 +60,21 @@ class Configuration extends \Doctrine\DBAL\Configuration /** @var mixed[] */ protected $_attributes = []; + /** @psalm-var array, ClassMetadata::GENERATOR_TYPE_*> */ + private $identityGenerationPreferences = []; + + /** @psalm-param array, ClassMetadata::GENERATOR_TYPE_*> $value */ + public function setIdentityGenerationPreferences(array $value): void + { + $this->identityGenerationPreferences = $value; + } + + /** @psalm-return array, ClassMetadata::GENERATOR_TYPE_*> $value */ + public function getIdentityGenerationPreferences(): array + { + return $this->identityGenerationPreferences; + } + /** * Sets the directory where Doctrine generates any necessary proxy class files. * diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index b134632817c..c08c2a7194b 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -30,10 +30,12 @@ use function assert; use function class_exists; +use function constant; use function count; use function end; use function explode; use function in_array; +use function is_a; use function is_subclass_of; use function strlen; use function strpos; @@ -64,6 +66,32 @@ class ClassMetadataFactory extends AbstractClassMetadataFactory /** @var mixed[] */ private $embeddablesActiveNesting = []; + private const LEGACY_DEFAULTS_FOR_ID_GENERATION = [ + 'Doctrine\DBAL\Platforms\MySqlPlatform' => ClassMetadata::GENERATOR_TYPE_IDENTITY, + 'Doctrine\DBAL\Platforms\PostgreSqlPlatform' => ClassMetadata::GENERATOR_TYPE_SEQUENCE, + Platforms\DB2Platform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + Platforms\DrizzlePlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + Platforms\MySQLPlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + Platforms\OraclePlatform::class => ClassMetadata::GENERATOR_TYPE_SEQUENCE, + Platforms\PostgreSQLPlatform::class => ClassMetadata::GENERATOR_TYPE_SEQUENCE, + Platforms\SQLAnywherePlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + Platforms\SQLServerPlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + Platforms\SqlitePlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + ]; + + private const RECOMMENDED_STRATEGY = [ + 'Doctrine\DBAL\Platforms\MySqlPlatform' => 'IDENTITY', + 'Doctrine\DBAL\Platforms\PostgreSqlPlatform' => 'IDENTITY', + Platforms\DB2Platform::class => 'IDENTITY', + Platforms\DrizzlePlatform::class => 'IDENTITY', + Platforms\MySQLPlatform::class => 'IDENTITY', + Platforms\OraclePlatform::class => 'SEQUENCE', + Platforms\PostgreSQLPlatform::class => 'IDENTITY', + Platforms\SQLAnywherePlatform::class => 'IDENTITY', + Platforms\SQLServerPlatform::class => 'IDENTITY', + Platforms\SqlitePlatform::class => 'IDENTITY', + ]; + /** * @return void */ @@ -643,22 +671,53 @@ private function completeIdGeneratorMapping(ClassMetadataInfo $class): void } /** - * @psalm-return ClassMetadata::GENERATOR_TYPE_SEQUENCE|ClassMetadata::GENERATOR_TYPE_IDENTITY + * @return ClassMetadata::GENERATOR_TYPE_* */ private function determineIdGeneratorStrategy(AbstractPlatform $platform): int { - if ( - $platform instanceof Platforms\OraclePlatform - || $platform instanceof Platforms\PostgreSQLPlatform - ) { - return ClassMetadata::GENERATOR_TYPE_SEQUENCE; + assert($this->em !== null); + foreach ($this->em->getConfiguration()->getIdentityGenerationPreferences() as $platformFamily => $strategy) { + if (is_a($platform, $platformFamily)) { + return $strategy; + } + } + + foreach (self::LEGACY_DEFAULTS_FOR_ID_GENERATION as $platformFamily => $strategy) { + if (is_a($platform, $platformFamily)) { + $recommendedStrategyName = self::RECOMMENDED_STRATEGY[$platformFamily]; + if ($strategy !== constant('Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_' . $recommendedStrategyName)) { + Deprecation::trigger( + 'doctrine/orm', + 'https://github.com/doctrine/orm/issues/8893', + <<<'DEPRECATION' +Relying on non-optimal defaults for ID generation is deprecated. +Instead, configure identifier generation strategies explicitly through +configuration. +We currently recommend "%s" for "%s", so you should use +$configuration->setIdentityGenerationPreferences([ + "%s" => ClassMetadata::GENERATOR_TYPE_%s, +]); +DEPRECATION + , + $recommendedStrategyName, + $platformFamily, + $platformFamily, + $recommendedStrategyName + ); + } + + return $strategy; + } } if ($platform->supportsIdentityColumns()) { return ClassMetadata::GENERATOR_TYPE_IDENTITY; } - if ($platform->supportsSequences()) { + if ( + $platform instanceof Platforms\OraclePlatform + || $platform instanceof Platforms\PostgreSQLPlatform + ) { return ClassMetadata::GENERATOR_TYPE_SEQUENCE; } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 724c9c549ed..737a5f9d631 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -235,6 +235,11 @@ parameters: count: 1 path: lib/Doctrine/ORM/Mapping/Builder/ClassMetadataBuilder.php + - + message: "#^Class Doctrine\\\\DBAL\\\\Platforms\\\\DrizzlePlatform not found\\.$#" + count: 2 + path: lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php + - message: "#^If condition is always true\\.$#" count: 2 diff --git a/phpstan-dbal2.neon b/phpstan-dbal2.neon index baee22e56e8..584f3ef3cec 100644 --- a/phpstan-dbal2.neon +++ b/phpstan-dbal2.neon @@ -42,5 +42,10 @@ parameters: count: 1 path: lib/Doctrine/ORM/Query/AST/Functions/SubstringFunction.php + - + message: '#^Class Doctrine\\\\DBAL\\\\Platforms\\\\MySQLPlatform not found\\.$#' + count: 2 + path: lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php + # Symfony cache supports passing a key prefix to the clear method. - '/^Method Psr\\Cache\\CacheItemPoolInterface\:\:clear\(\) invoked with 1 parameter, 0 required\.$/' diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 73538651193..17695283950 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -653,10 +653,11 @@ - + $class $class $nonSuperclassParents + $platformFamily new $definition['class']() @@ -704,6 +705,10 @@ $subClass->table + + Platforms\DrizzlePlatform + Platforms\DrizzlePlatform + diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php index 65eb46278a3..1ee0dad10cc 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php @@ -8,6 +8,9 @@ use Doctrine\DBAL\Connection; use Doctrine\DBAL\Driver; use Doctrine\DBAL\Platforms\AbstractPlatform; +use Doctrine\DBAL\Platforms\OraclePlatform; +use Doctrine\DBAL\Platforms\PostgreSQLPlatform; +use Doctrine\Deprecations\PHPUnit\VerifyDeprecations; use Doctrine\ORM\Configuration; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Event\OnClassMetadataNotFoundEventArgs; @@ -27,6 +30,7 @@ use Doctrine\ORM\Mapping\MappingException; use Doctrine\Persistence\Mapping\Driver\MappingDriver; use Doctrine\Persistence\Mapping\RuntimeReflectionService; +use Doctrine\Tests\Mocks\ConnectionMock; use Doctrine\Tests\Mocks\EntityManagerMock; use Doctrine\Tests\Mocks\MetadataDriverMock; use Doctrine\Tests\Models\CMS\CmsArticle; @@ -50,6 +54,8 @@ class ClassMetadataFactoryTest extends OrmTestCase { + use VerifyDeprecations; + public function testGetMetadataForSingleClass(): void { $platform = $this->createMock(AbstractPlatform::class); @@ -123,6 +129,66 @@ public function testGetMetadataForReturnsLoadedCustomIdGenerator(): void self::assertInstanceOf(CustomIdGenerator::class, $actual->idGenerator); } + public function testRelyingOnLegacyIdGenerationDefaultsIsDeprecatedIfItResultsInASuboptimalDefault(): void + { + $cm1 = $this->createValidClassMetadata(); + $cm1->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_AUTO); + $cmf = new ClassMetadataFactoryTestSubject(); + $driver = $this->createMock(Driver::class); + $driver->method('getDatabasePlatform') + ->willReturn(new PostgreSQLPlatform()); + $entityManager = $this->createEntityManager( + new MetadataDriverMock(), + new Connection([], $driver) + ); + $cmf->setEntityManager($entityManager); + $cmf->setMetadataForClass($cm1->name, $cm1); + + $this->expectDeprecationWithIdentifier('https://github.com/doctrine/orm/issues/8893'); + $cmf->getMetadataFor($cm1->name); + } + + public function testSpecifyingIdGenerationStrategyThroughConfigurationFixesTheDeprecation(): void + { + $cm1 = $this->createValidClassMetadata(); + $cm1->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_AUTO); + $cmf = new ClassMetadataFactoryTestSubject(); + $driver = $this->createMock(Driver::class); + $driver->method('getDatabasePlatform') + ->willReturn(new PostgreSQLPlatform()); + $entityManager = $this->createEntityManager( + new MetadataDriverMock(), + new Connection([], $driver) + ); + $cmf->setEntityManager($entityManager); + $cmf->setMetadataForClass($cm1->name, $cm1); + + $entityManager->getConfiguration()->setIdentityGenerationPreferences([ + PostgreSQLPlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY, + ]); + $this->expectNoDeprecationWithIdentifier('https://github.com/doctrine/orm/issues/8893'); + $cmf->getMetadataFor($cm1->name); + } + + public function testRelyingOnLegacyIdGenerationDefaultsIsOKIfItResultsInTheCurrentlyRecommendedStrategyBeingUsed(): void + { + $cm1 = $this->createValidClassMetadata(); + $cm1->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_AUTO); + $cmf = new ClassMetadataFactoryTestSubject(); + $driver = $this->createMock(Driver::class); + $driver->method('getDatabasePlatform') + ->willReturn(new OraclePlatform()); + $entityManager = $this->createEntityManager( + new MetadataDriverMock(), + new Connection([], $driver) + ); + $cmf->setEntityManager($entityManager); + $cmf->setMetadataForClass($cm1->name, $cm1); + + $this->expectNoDeprecationWithIdentifier('https://github.com/doctrine/orm/issues/8893'); + $cmf->getMetadataFor($cm1->name); + } + public function testGetMetadataForThrowsExceptionOnUnknownCustomGeneratorClass(): void { $cm1 = $this->createValidClassMetadata();