Deprecate / remove TABLE id generator strategy

This strategy has been marked as TODO for more than 14 years. It should be OK to remove some things related to it since they lead to an exception being thrown.
doctrine · Aug 28, 2021 · 189544c · 189544c
1 parent a06bbaf
commit 189544c
Show file tree

Hide file tree

Showing 20 changed files with 69 additions and 62 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,9 @@
 # Upgrade to 2.10
 
+## BC Break: Removed `TABLE` id generator strategy
+
+The implementation was unfinished for 14 years.
+
 ## 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) {
@@ -663,7 +659,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
@@ -689,8 +685,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
@@ -218,7 +218,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);