Merge pull request #3586 from morozov/issues/3263

Get rid of hard-coded default values of maximum field lengths
doctrine · Jun 27, 2019 · bff2f33 · bff2f33
2 parents 785050a + 3906e21
commit bff2f33
Show file tree

Hide file tree

Showing 45 changed files with 980 additions and 673 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,11 @@
 # Upgrade to 3.0
 
+## BC BREAK: Changes in handling string and binary columns
+
+- When generating schema DDL, DBAL no longer provides the default length for string and binary columns. The application may need to provide the column length if required by the target platform.
+- The `\DBAL\Platforms\AbstractPlatform::getVarcharTypeDeclarationSQL()` method has been renamed to `::getStringTypeDeclarationSQL()`.
+- The following `AbstractPlatform` methods have been removed as no longer relevant: `::getCharMaxLength()`, `::getVarcharMaxLength()`, `::getVarcharDefaultLength()`, `::getBinaryMaxLength()`, `::getBinaryDefaultLength()`. 
+
 ## BC BREAK: Changes in `Doctrine\DBAL\Event\SchemaCreateTableEventArgs`
 
 Table columns are no longer indexed by column name. Use the `name` attribute of the column instead.

diff --git a/lib/Doctrine/DBAL/Exception/ColumnLengthRequired.php b/lib/Doctrine/DBAL/Exception/ColumnLengthRequired.php
@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\DBAL\Exception;
+
+use Doctrine\DBAL\DBALException;
+use Doctrine\DBAL\Platforms\AbstractPlatform;
+use function sprintf;
+
+final class ColumnLengthRequired extends DBALException
+{
+    /**
+     * @param AbstractPlatform $platform The target platform
+     * @param string           $type     The SQL column type
+     */
+    public static function new(AbstractPlatform $platform, string $type) : self
+    {
+        return new self(
+            sprintf(
+                'The "%s" platform requires the length of a %s column to be specified',
+                $platform->getName(),
+                $type
+            )
+        );
+    }
+}
diff --git a/lib/Doctrine/DBAL/Id/TableGeneratorSchemaVisitor.php b/lib/Doctrine/DBAL/Id/TableGeneratorSchemaVisitor.php
@@ -28,7 +28,8 @@ public function __construct(string $generatorTableName = 'sequences')
     public function acceptSchema(Schema $schema) : void
     {
         $table = $schema->createTable($this->generatorTableName);
-        $table->addColumn('sequence_name', 'string');
+
+        $table->addColumn('sequence_name', 'string', ['length' => 255]);
         $table->addColumn('sequence_value', 'integer', ['default' => 1]);
         $table->addColumn('sequence_increment_by', 'integer', ['default' => 1]);
     }

diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -15,6 +15,7 @@
 use Doctrine\DBAL\Event\SchemaCreateTableEventArgs;
 use Doctrine\DBAL\Event\SchemaDropTableEventArgs;
 use Doctrine\DBAL\Events;
+use Doctrine\DBAL\Exception\ColumnLengthRequired;
 use Doctrine\DBAL\Platforms\Exception\NoColumnsSpecifiedForTable;
 use Doctrine\DBAL\Platforms\Exception\NotSupported;
 use Doctrine\DBAL\Platforms\Keywords\KeywordList;
@@ -168,43 +169,39 @@ private function initializeAllDoctrineTypeMappings() : void
     }
 
     /**
-     * Returns the SQL snippet used to declare a VARCHAR column type.
+     * Returns the SQL snippet used to declare a string column type.
      *
-     * @param mixed[] $field
+     * @param array<string, mixed> $column The column definition.
+     *
+     * @throws ColumnLengthRequired
      */
-    public function getVarcharTypeDeclarationSQL(array $field) : string
+    public function getStringTypeDeclarationSQL(array $column) : string
     {
-        if (! isset($field['length'])) {
-            $field['length'] = $this->getVarcharDefaultLength();
-        }
-
-        $fixed = $field['fixed'] ?? false;
-
-        $maxLength = $fixed
-            ? $this->getCharMaxLength()
-            : $this->getVarcharMaxLength();
+        $length = $column['length'] ?? null;
 
-        if ($field['length'] > $maxLength) {
-            return $this->getClobTypeDeclarationSQL($field);
+        if (empty($column['fixed'])) {
+            return $this->getVarcharTypeDeclarationSQLSnippet($length);
         }
 
-        return $this->getVarcharTypeDeclarationSQLSnippet($field['length'], $fixed);
+        return $this->getCharTypeDeclarationSQLSnippet($length);
     }
 
     /**
-     * Returns the SQL snippet used to declare a BINARY/VARBINARY column type.
+     * Returns the SQL snippet used to declare a binary string column type.
      *
-     * @param mixed[] $field The column definition.
+     * @param array<string, mixed> $column The column definition.
+     *
+     * @throws ColumnLengthRequired
      */
-    public function getBinaryTypeDeclarationSQL(array $field) : string
+    public function getBinaryTypeDeclarationSQL(array $column) : string
     {
-        if (! isset($field['length'])) {
-            $field['length'] = $this->getBinaryDefaultLength();
-        }
+        $length = $column['length'] ?? null;
 
-        $fixed = $field['fixed'] ?? false;
+        if (empty($column['fixed'])) {
+            return $this->getVarbinaryTypeDeclarationSQLSnippet($length);
+        }
 
-        return $this->getBinaryTypeDeclarationSQLSnippet($field['length'], $fixed);
+        return $this->getBinaryTypeDeclarationSQLSnippet($length);
     }
 
     /**
@@ -213,14 +210,16 @@ public function getBinaryTypeDeclarationSQL(array $field) : string
      * By default this maps directly to a CHAR(36) and only maps to more
      * special datatypes when the underlying databases support this datatype.
      *
-     * @param mixed[] $field
+     * @param array<string, mixed> $column The column definition.
+     *
+     * @throws DBALException
      */
-    public function getGuidTypeDeclarationSQL(array $field) : string
+    public function getGuidTypeDeclarationSQL(array $column) : string
     {
-        $field['length'] = 36;
-        $field['fixed']  = true;
+        $column['length'] = 36;
+        $column['fixed']  = true;
 
-        return $this->getVarcharTypeDeclarationSQL($field);
+        return $this->getStringTypeDeclarationSQL($column);
     }
 
     /**
@@ -237,27 +236,71 @@ public function getJsonTypeDeclarationSQL(array $field) : string
     }
 
     /**
-     * @param int  $length The length of the column.
-     * @param bool $fixed  Whether the column length is fixed.
+     * @param int|null $length The length of the column in characters
+     *                         or NULL if the length should be omitted.
      *
-     * @throws DBALException If not supported on this platform.
+     * @throws ColumnLengthRequired
+     */
+    protected function getCharTypeDeclarationSQLSnippet(?int $length) : string
+    {
+        $sql = 'CHAR';
+
+        if ($length !== null) {
+            $sql .= sprintf('(%d)', $length);
+        }
+
+        return $sql;
+    }
+
+    /**
+     * @param int|null $length The length of the column in characters
+     *                         or NULL if the length should be omitted.
+     *
+     * @throws ColumnLengthRequired
      */
-    protected function getVarcharTypeDeclarationSQLSnippet(int $length, bool $fixed) : string
+    protected function getVarcharTypeDeclarationSQLSnippet(?int $length) : string
     {
-        throw NotSupported::new('VARCHARs not supported by Platform.');
+        if ($length === null) {
+            throw ColumnLengthRequired::new($this, 'VARCHAR');
+        }
+
+        return sprintf('VARCHAR(%d)', $length);
     }
 
     /**
-     * Returns the SQL snippet used to declare a BINARY/VARBINARY column type.
+     * Returns the SQL snippet used to declare a fixed length binary column type.
      *
-     * @param int  $length The length of the column.
-     * @param bool $fixed  Whether the column length is fixed.
+     * @param int|null $length The length of the column in bytes
+     *                         or NULL if the length should be omitted.
      *
-     * @throws DBALException If not supported on this platform.
+     * @throws ColumnLengthRequired
      */
-    protected function getBinaryTypeDeclarationSQLSnippet(int $length, bool $fixed) : string
+    protected function getBinaryTypeDeclarationSQLSnippet(?int $length) : string
     {
-        throw NotSupported::new('BINARY/VARBINARY column types are not supported by this platform.');
+        $sql = 'BINARY';
+
+        if ($length !== null) {
+            $sql .= sprintf('(%d)', $length);
+        }
+
+        return $sql;
+    }
+
+    /**
+     * Returns the SQL snippet used to declare a variable length binary column type.
+     *
+     * @param int|null $length The length of the column in bytes
+     *                         or NULL if the length should be omitted.
+     *
+     * @throws ColumnLengthRequired
+     */
+    protected function getVarbinaryTypeDeclarationSQLSnippet(?int $length) : string
+    {
+        if ($length === null) {
+            throw ColumnLengthRequired::new($this, 'VARBINARY');
+        }
+
+        return sprintf('VARBINARY(%d)', $length);
     }
 
     /**
@@ -438,46 +481,6 @@ public function getSqlCommentEndString() : string
         return "\n";
     }
 
-    /**
-     * Gets the maximum length of a char field.
-     */
-    public function getCharMaxLength() : int
-    {
-        return $this->getVarcharMaxLength();
-    }
-
-    /**
-     * Gets the maximum length of a varchar field.
-     */
-    public function getVarcharMaxLength() : int
-    {
-        return 4000;
-    }
-
-    /**
-     * Gets the default length of a varchar field.
-     */
-    public function getVarcharDefaultLength() : int
-    {
-        return 255;
-    }
-
-    /**
-     * Gets the maximum length of a binary field.
-     */
-    public function getBinaryMaxLength() : int
-    {
-        return 4000;
-    }
-
-    /**
-     * Gets the default length of a binary field.
-     */
-    public function getBinaryDefaultLength() : int
-    {
-        return 255;
-    }
-
     /**
      * Gets all SQL wildcard characters of the platform.
      *
@@ -1337,10 +1340,6 @@ public function getCreateTableSQL(Table $table, int $createFlags = self::CREATE_
             $columnData['version'] = $column->hasPlatformOption('version') ? $column->getPlatformOption('version') : false;
             $columnData['comment'] = $this->getColumnComment($column);
 
-            if ($columnData['type'] instanceof Types\StringType && $columnData['length'] === null) {
-                $columnData['length'] = 255;
-            }
-
             if (in_array($column->getName(), $options['primary'])) {
                 $columnData['primary'] = true;
             }

diff --git a/lib/Doctrine/DBAL/Platforms/DB2Platform.php b/lib/Doctrine/DBAL/Platforms/DB2Platform.php
@@ -22,43 +22,6 @@
 
 class DB2Platform extends AbstractPlatform
 {
-    /**
-     * {@inheritdoc}
-     */
-    public function getCharMaxLength() : int
-    {
-        return 254;
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getBinaryMaxLength() : int
-    {
-        return 32704;
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getBinaryDefaultLength() : int
-    {
-        return 1;
-    }
-
-    /**
-     * {@inheritDoc}
-     */
-    public function getVarcharTypeDeclarationSQL(array $field) : string
-    {
-        // for IBM DB2, the CHAR max length is less than VARCHAR default length
-        if (! isset($field['length']) && ! empty($field['fixed'])) {
-            $field['length'] = $this->getCharMaxLength();
-        }
-
-        return parent::getVarcharTypeDeclarationSQL($field);
-    }
-
     /**
      * {@inheritDoc}
      */
@@ -109,18 +72,17 @@ public function isCommentedDoctrineType(Type $doctrineType) : bool
     /**
      * {@inheritDoc}
      */
-    protected function getVarcharTypeDeclarationSQLSnippet(int $length, bool $fixed) : string
+    protected function getBinaryTypeDeclarationSQLSnippet(?int $length) : string
     {
-        return $fixed ? ($length ? 'CHAR(' . $length . ')' : 'CHAR(254)')
-                : ($length ? 'VARCHAR(' . $length . ')' : 'VARCHAR(255)');
+        return $this->getCharTypeDeclarationSQLSnippet($length) . ' FOR BIT DATA';
     }
 
     /**
-     * {@inheritdoc}
+     * {@inheritDoc}
      */
-    protected function getBinaryTypeDeclarationSQLSnippet(int $length, bool $fixed) : string
+    protected function getVarbinaryTypeDeclarationSQLSnippet(?int $length) : string
     {
-        return $this->getVarcharTypeDeclarationSQLSnippet($length, $fixed) . ' FOR BIT DATA';
+        return $this->getVarcharTypeDeclarationSQLSnippet($length) . ' FOR BIT DATA';
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php b/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php
@@ -201,23 +201,6 @@ public function getDropViewSQL(string $name) : string
         return 'DROP VIEW ' . $name;
     }
 
-    /**
-     * {@inheritDoc}
-     */
-    protected function getVarcharTypeDeclarationSQLSnippet(int $length, bool $fixed) : string
-    {
-        return $fixed ? ($length ? 'CHAR(' . $length . ')' : 'CHAR(255)')
-                : ($length ? 'VARCHAR(' . $length . ')' : 'VARCHAR(255)');
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    protected function getBinaryTypeDeclarationSQLSnippet(int $length, bool $fixed) : string
-    {
-        return $fixed ? 'BINARY(' . ($length ?: 255) . ')' : 'VARBINARY(' . ($length ?: 255) . ')';
-    }
-
     /**
      * Gets the SQL snippet used to declare a CLOB column type.
      *     TINYTEXT   : 2 ^  8 - 1 = 255
@@ -1032,22 +1015,6 @@ protected function initializeDoctrineTypeMappings() : void
         ];
     }
 
-    /**
-     * {@inheritDoc}
-     */
-    public function getVarcharMaxLength() : int
-    {
-        return 65535;
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getBinaryMaxLength() : int
-    {
-        return 65535;
-    }
-
     /**
      * {@inheritDoc}
      */