Merge pull request #8961 from greg0ire/drop-table

Deprecate / remove TABLE id generator strategy
doctrine · Aug 29, 2021 · a427d7d · a427d7d
2 parents d5f65ba + 0b55275
commit a427d7d
Show file tree

Hide file tree

Showing 20 changed files with 74 additions and 62 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,14 @@
 # Upgrade to 2.10
 
+## BC Break: Removed `TABLE` id generator strategy
+
+The implementation was unfinished for 14 years.
+It is now deprecated to rely on:
+- `Doctrine\ORM\Id\TableGenerator`;
+- `Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_TABLE`;
+- `Doctrine\ORM\Mapping\ClassMetadata::$tableGeneratorDefinition`;
+- or `Doctrine\ORM\Mapping\ClassMetadata::isIdGeneratorTable()`.
+
 ## BC Break: Removed possibility to extend the doctrine mapping xml schema with anything
 
 If you want to extend it now you have to provide your own validation schema.

diff --git a/docs/en/reference/annotations-reference.rst b/docs/en/reference/annotations-reference.rst
@@ -463,7 +463,7 @@ Optional attributes:
 
 
 -  **strategy**: Set the name of the identifier generation strategy.
-   Valid values are ``AUTO``, ``SEQUENCE``, ``TABLE``, ``IDENTITY``, ``UUID`` (deprecated), ``CUSTOM`` and ``NONE``, explained
+   Valid values are ``AUTO``, ``SEQUENCE``, ``IDENTITY``, ``UUID`` (deprecated), ``CUSTOM`` and ``NONE``, explained
    in the :ref:`Identifier Generation Strategies <identifier-generation-strategies>` section.
    If not specified, default value is AUTO.
 

diff --git a/docs/en/reference/attributes-reference.rst b/docs/en/reference/attributes-reference.rst
@@ -371,8 +371,8 @@ used as default.
 Optional attributes:
 
 -  **strategy**: Set the name of the identifier generation strategy.
-   Valid values are ``AUTO``, ``SEQUENCE``, ``TABLE``, ``IDENTITY``,
-   ``UUID`` (deprecated), ``CUSTOM`` and ``NONE``.
+   Valid values are ``AUTO``, ``SEQUENCE``, ``IDENTITY``, ``UUID``
+   (deprecated), ``CUSTOM`` and ``NONE``.
    If not specified, the default value is ``AUTO``.
 
 Example:

diff --git a/docs/en/reference/basic-mapping.rst b/docs/en/reference/basic-mapping.rst
@@ -381,9 +381,6 @@ Here is the list of possible generation strategies:
    (AUTO\_INCREMENT), MSSQL (IDENTITY) and PostgreSQL (SERIAL).
 -  ``UUID`` (deprecated): Tells Doctrine to use the built-in Universally
    Unique Identifier generator. This strategy provides full portability.
--  ``TABLE``: Tells Doctrine to use a separate table for ID
-   generation. This strategy provides full portability.
-   ***This strategy is not yet implemented!***
 -  ``NONE``: Tells Doctrine that the identifiers are assigned (and
    thus generated) by your code. The assignment must take place before
    a new entity is passed to ``EntityManager#persist``. NONE is the

diff --git a/doctrine-mapping.xsd b/doctrine-mapping.xsd
@@ -240,7 +240,6 @@
   <xs:simpleType name="generator-strategy">
     <xs:restriction base="xs:token">
       <xs:enumeration value="NONE"/>
-      <xs:enumeration value="TABLE"/>
       <xs:enumeration value="SEQUENCE"/>
       <xs:enumeration value="IDENTITY"/>
       <xs:enumeration value="AUTO"/>

diff --git a/lib/Doctrine/ORM/Id/TableGenerator.php b/lib/Doctrine/ORM/Id/TableGenerator.php
@@ -8,6 +8,8 @@
 
 /**
  * Id generator that uses a single-row database table and a hi/lo algorithm.
+ *
+ * @deprecated no replacement planned
  */
 class TableGenerator extends AbstractIdGenerator
 {

diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
@@ -18,6 +18,7 @@
 use Doctrine\ORM\Id\IdentityGenerator;
 use Doctrine\ORM\Id\SequenceGenerator;
 use Doctrine\ORM\Id\UuidGenerator;
+use Doctrine\ORM\Mapping\Exception\CannotGenerateIds;
 use Doctrine\ORM\Mapping\Exception\InvalidCustomGenerator;
 use Doctrine\ORM\Mapping\Exception\TableGeneratorNotImplementedYet;
 use Doctrine\ORM\Mapping\Exception\UnknownGeneratorType;
@@ -626,11 +627,6 @@ private function completeIdGeneratorMapping(ClassMetadataInfo $class): void
                 $class->setIdGenerator(new UuidGenerator());
                 break;
 
-            case ClassMetadata::GENERATOR_TYPE_TABLE:
-                throw TableGeneratorNotImplementedYet::create();
-
-                break;
-
             case ClassMetadata::GENERATOR_TYPE_CUSTOM:
                 $definition = $class->customGeneratorDefinition;
                 if ($definition === null) {
@@ -667,7 +663,7 @@ private function determineIdGeneratorStrategy(AbstractPlatform $platform): int
             return ClassMetadata::GENERATOR_TYPE_SEQUENCE;
         }
 
-        return ClassMetadata::GENERATOR_TYPE_TABLE;
+        throw CannotGenerateIds::withPlatform($platform);
     }
 
     private function truncateSequenceName(string $schemaElementName): string
@@ -693,8 +689,6 @@ private function inheritIdGeneratorMapping(ClassMetadataInfo $class, ClassMetada
     {
         if ($parent->isIdGeneratorSequence()) {
             $class->setSequenceGeneratorDefinition($parent->sequenceGeneratorDefinition);
-        } elseif ($parent->isIdGeneratorTable()) {
-            $class->tableGeneratorDefinition = $parent->tableGeneratorDefinition;
         }
 
         if ($parent->generatorType) {

diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -113,7 +113,10 @@ class ClassMetadataInfo implements ClassMetadata
 
     /**
      * TABLE means a separate table is used for id generation.
-     * Offers full portability.
+     * Offers full portability (in that it results in an exception being thrown
+     * no matter the platform).
+     *
+     * @deprecated no replacement planned
      */
     public const GENERATOR_TYPE_TABLE = 3;
 
@@ -628,8 +631,9 @@ class ClassMetadataInfo implements ClassMetadata
      * READ-ONLY: The definition of the table generator of this class. Only used for the
      * TABLE generation strategy.
      *
+     * @deprecated
+     *
      * @var array<string, mixed>
-     * @todo Merge with tableGeneratorDefinition into generic generatorDefinition
      */
     public $tableGeneratorDefinition;
 
@@ -2230,11 +2234,13 @@ public function isIdGeneratorSequence()
     /**
      * Checks whether the class uses a table for id generation.
      *
-     * @return bool TRUE if the class uses the TABLE generator, FALSE otherwise.
+     * @deprecated
+     *
+     * @return false
      */
     public function isIdGeneratorTable()
     {
-        return $this->generatorType === self::GENERATOR_TYPE_TABLE;
+        return false;
     }
 
     /**

diff --git a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
@@ -386,8 +386,6 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
                             'initialValue' => $seqGeneratorAnnot->initialValue,
                         ]
                     );
-                } elseif ($this->reader->getPropertyAnnotation($property, TableGenerator::class)) {
-                    throw MappingException::tableIdGeneratorNotImplemented($className);
                 } else {
                     $customGeneratorAnnot = $this->reader->getPropertyAnnotation($property, Mapping\CustomIdGenerator::class);
                     if ($customGeneratorAnnot) {

diff --git a/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php b/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php
@@ -383,8 +383,6 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
                         'class' => (string) $customGenerator['class'],
                     ]
                 );
-            } elseif (isset($idElement->{'table-generator'})) {
-                throw MappingException::tableIdGeneratorNotImplemented($className);
             }
         }
 

diff --git a/lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php b/lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
@@ -350,7 +350,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
                         . strtoupper($idElement['generator']['strategy'])));
                 }
 
-                // Check for SequenceGenerator/TableGenerator definition
+                // Check for SequenceGenerator definition
                 if (isset($idElement['sequenceGenerator'])) {
                     $metadata->setSequenceGeneratorDefinition($idElement['sequenceGenerator']);
                 } elseif (isset($idElement['customIdGenerator'])) {
@@ -360,8 +360,6 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
                             'class' => (string) $customGenerator['class'],
                         ]
                     );
-                } elseif (isset($idElement['tableGenerator'])) {
-                    throw MappingException::tableIdGeneratorNotImplemented($className);
                 }
             }
         }

diff --git a/lib/Doctrine/ORM/Mapping/Exception/CannotGenerateIds.php b/lib/Doctrine/ORM/Mapping/Exception/CannotGenerateIds.php
@@ -0,0 +1,23 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Mapping\Exception;
+
+use Doctrine\DBAL\Platforms\AbstractPlatform;
+use Doctrine\ORM\Exception\ORMException;
+use LogicException;
+
+use function get_class;
+use function sprintf;
+
+final class CannotGenerateIds extends ORMException
+{
+    public static function withPlatform(AbstractPlatform $platform): self
+    {
+        return new self(sprintf(
+            'Platform %s does not support generating identifiers',
+            get_class($platform)
+        ));
+    }
+}
diff --git a/lib/Doctrine/ORM/Mapping/Exception/TableGeneratorNotImplementedYet.php b/lib/Doctrine/ORM/Mapping/Exception/TableGeneratorNotImplementedYet.php
diff --git a/lib/Doctrine/ORM/Mapping/MappingException.php b/lib/Doctrine/ORM/Mapping/MappingException.php
@@ -383,16 +383,6 @@ public static function propertyTypeIsRequired($className, $propertyName)
         ));
     }
 
-    /**
-     * @param string $className
-     *
-     * @return MappingException
-     */
-    public static function tableIdGeneratorNotImplemented($className)
-    {
-        return new self(sprintf('TableIdGenerator is not yet implemented for use with class %s', $className));
-    }
-
     /**
      * @param string $entity    The entity's name.
      * @param string $fieldName The name of the field that was already declared.

diff --git a/lib/Doctrine/ORM/Tools/Console/Command/MappingDescribeCommand.php b/lib/Doctrine/ORM/Tools/Console/Command/MappingDescribeCommand.php
@@ -109,7 +109,6 @@ private function displayEntity(
                     $this->formatField('Composite identifier?', $metadata->isIdentifierComposite),
                     $this->formatField('Foreign identifier?', $metadata->containsForeignIdentifier),
                     $this->formatField('Sequence generator definition', $metadata->sequenceGeneratorDefinition),
-                    $this->formatField('Table generator definition', $metadata->tableGeneratorDefinition),
                     $this->formatField('Change tracking policy', $metadata->changeTrackingPolicy),
                     $this->formatField('Versioned?', $metadata->isVersioned),
                     $this->formatField('Version field', $metadata->versionField),

diff --git a/lib/Doctrine/ORM/Tools/EntityGenerator.php b/lib/Doctrine/ORM/Tools/EntityGenerator.php
@@ -219,7 +219,6 @@ class EntityGenerator
     protected static $generatorStrategyMap = [
         ClassMetadataInfo::GENERATOR_TYPE_AUTO      => 'AUTO',
         ClassMetadataInfo::GENERATOR_TYPE_SEQUENCE  => 'SEQUENCE',
-        ClassMetadataInfo::GENERATOR_TYPE_TABLE     => 'TABLE',
         ClassMetadataInfo::GENERATOR_TYPE_IDENTITY  => 'IDENTITY',
         ClassMetadataInfo::GENERATOR_TYPE_NONE      => 'NONE',
         ClassMetadataInfo::GENERATOR_TYPE_UUID      => 'UUID',

diff --git a/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php b/lib/Doctrine/ORM/Tools/Export/Driver/AbstractExporter.php
@@ -250,9 +250,6 @@ protected function _getIdGeneratorTypeString($type)
             case ClassMetadataInfo::GENERATOR_TYPE_SEQUENCE:
                 return 'SEQUENCE';
 
-            case ClassMetadataInfo::GENERATOR_TYPE_TABLE:
-                return 'TABLE';
-
             case ClassMetadataInfo::GENERATOR_TYPE_IDENTITY:
                 return 'IDENTITY';
 

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -520,11 +520,6 @@ parameters:
 			count: 1
 			path: lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
 
-		-
-			message: "#^Unreachable statement \\- code above always terminates\\.$#"
-			count: 1
-			path: lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
-
 		-
 			message: "#^Array \\(array\\('name' \\=\\> string, 'schema' \\=\\> string, 'indexes' \\=\\> array, 'uniqueConstraints' \\=\\> array\\)\\) does not accept key 'options'\\.$#"
 			count: 1

diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataFactoryTest.php
@@ -19,13 +19,15 @@
 use Doctrine\ORM\Mapping\DiscriminatorColumn;
 use Doctrine\ORM\Mapping\DiscriminatorMap;
 use Doctrine\ORM\Mapping\Entity;
+use Doctrine\ORM\Mapping\Exception\CannotGenerateIds;
 use Doctrine\ORM\Mapping\GeneratedValue;
 use Doctrine\ORM\Mapping\Id;
 use Doctrine\ORM\Mapping\InheritanceType;
 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\DatabasePlatformMock;
 use Doctrine\Tests\Mocks\DriverMock;
 use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Mocks\MetadataDriverMock;
@@ -85,6 +87,24 @@ public function testGetMetadataForSingleClass(): void
         self::assertTrue($cmMap1->hasField('name'));
     }
 
+    public function testItThrowsWhenUsingAutoWithIncompatiblePlatform(): void
+    {
+        $cm1 = $this->createValidClassMetadata();
+        $cm1->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_AUTO);
+        $entityManager = $this->createEntityManager(new MetadataDriverMock());
+        $connection    = $entityManager->getConnection();
+        assert($connection instanceof ConnectionMock);
+        $platform = $connection->getDatabasePlatform();
+        assert($platform instanceof DatabasePlatformMock);
+        $platform->setSupportsIdentityColumns(false);
+        $cmf = new ClassMetadataFactoryTestSubject();
+        $cmf->setEntityManager($entityManager);
+        $cmf->setMetadataForClass($cm1->name, $cm1);
+        $this->expectException(CannotGenerateIds::class);
+
+        $actual = $cmf->getMetadataFor($cm1->name);
+    }
+
     public function testGetMetadataForReturnsLoadedCustomIdGenerator(): void
     {
         $cm1 = $this->createValidClassMetadata();

diff --git a/tests/Doctrine/Tests/ORM/Tools/EntityGeneratorTest.php b/tests/Doctrine/Tests/ORM/Tools/EntityGeneratorTest.php
@@ -754,6 +754,10 @@ public function testGetIdGeneratorTypeString(): void
                 continue;
             }
 
+            if ($name === 'GENERATOR_TYPE_TABLE') {
+                continue;
+            }
+
             $expected = preg_replace($pattern, '', $name);
             $actual   = $method->invoke($this->generator, $value);