Merge pull request #3501 from morozov/extract-type-from-comment

Reworked AbstractSchemaManager::extractDoctrineTypeFromComment(), removed ::removeDoctrineTypeFromComment()

UPGRADE.md

 # Upgrade to 3.0
 
+## BC BREAK `AbstractSchemaManager::extractDoctrineTypeFromComment()` changed, `::removeDoctrineTypeFromComment()` removed
+
+`AbstractSchemaManager::extractDoctrineTypeFromComment()` made `protected`. It takes the comment by reference, removes the type annotation from it and returns the extracted Doctrine type.
+
 ## BC BREAK `::errorCode()` and `::errorInfo()` removed from `Connection` and `Statement` APIs
 
 The error information is available in `DriverException` trown in case of an error.

lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php

@@ -23,7 +23,6 @@ use function func_get_args;
 use function is_array;
 use function is_callable;
 use function preg_match;
-use function str_replace;
 use function strtolower;
 
 /**
@@ -1105,35 +1104,19 @@ abstract class AbstractSchemaManager
     }
 
     /**
-     * Given a table comment this method tries to extract a typehint for Doctrine Type, or returns
-     * the type given as default.
+     * Given a table comment this method tries to extract a type hint for Doctrine Type. If the type hint is found,
+     * it's removed from the comment.
      *
-     * @param string|null $comment
-     * @param string      $currentType
-     *
-     * @return string
-     */
-    public function extractDoctrineTypeFromComment($comment, $currentType)
-    {
-        if ($comment !== null && preg_match('(\(DC2Type:(((?!\)).)+)\))', $comment, $match)) {
-            return $match[1];
-        }
-
-        return $currentType;
-    }
-
-    /**
-     * @param string|null $comment
-     * @param string|null $type
-     *
-     * @return string|null
+     * @return string|null The extracted Doctrine type or NULL of the type hint was not found.
      */
-    public function removeDoctrineTypeFromComment($comment, $type)
+    final protected function extractDoctrineTypeFromComment(?string &$comment) : ?string
     {
-        if ($comment === null) {
+        if ($comment === null || ! preg_match('/(.*)\(DC2Type:(((?!\)).)+)\)(.*)/', $comment, $match)) {
             return null;
         }
 
-        return str_replace('(DC2Type:' . $type . ')', '', $comment);
+        $comment = $match[1] . $match[4];
+
+        return $match[2];
     }
 }

lib/Doctrine/DBAL/Schema/DB2SchemaManager.php

@@ -62,12 +62,8 @@ class DB2SchemaManager extends AbstractSchemaManager
             }
         }
 
-        $type = $this->_platform->getDoctrineTypeMapping($tableColumn['typename']);
-
-        if (isset($tableColumn['comment'])) {
-            $type                   = $this->extractDoctrineTypeFromComment($tableColumn['comment'], $type);
-            $tableColumn['comment'] = $this->removeDoctrineTypeFromComment($tableColumn['comment'], $type);
-        }
+        $type = $this->extractDoctrineTypeFromComment($tableColumn['comment'])
+            ?? $this->_platform->getDoctrineTypeMapping($tableColumn['typename']);
 
         switch (strtolower($tableColumn['typename'])) {
             case 'varchar':

lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php

@@ -127,13 +127,8 @@ class MySqlSchemaManager extends AbstractSchemaManager
         $scale     = null;
         $precision = null;
 
-        $type = $this->_platform->getDoctrineTypeMapping($dbType);
-
-        // In cases where not connected to a database DESCRIBE $table does not return 'Comment'
-        if (isset($tableColumn['comment'])) {
-            $type                   = $this->extractDoctrineTypeFromComment($tableColumn['comment'], $type);
-            $tableColumn['comment'] = $this->removeDoctrineTypeFromComment($tableColumn['comment'], $type);
-        }
+        $type = $this->extractDoctrineTypeFromComment($tableColumn['comment'])
+            ?? $this->_platform->getDoctrineTypeMapping($dbType);
 
         switch ($dbType) {
             case 'char':

lib/Doctrine/DBAL/Schema/OracleSchemaManager.php

@@ -163,9 +163,8 @@ class OracleSchemaManager extends AbstractSchemaManager
             $scale = (int) $tableColumn['data_scale'];
         }
 
-        $type                    = $this->_platform->getDoctrineTypeMapping($dbType);
-        $type                    = $this->extractDoctrineTypeFromComment($tableColumn['comments'], $type);
-        $tableColumn['comments'] = $this->removeDoctrineTypeFromComment($tableColumn['comments'], $type);
+        $type = $this->extractDoctrineTypeFromComment($tableColumn['comments'])
+            ?? $this->_platform->getDoctrineTypeMapping($dbType);
 
         switch ($dbType) {
             case 'number':

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

@@ -365,9 +365,8 @@ class PostgreSqlSchemaManager extends AbstractSchemaManager
             $tableColumn['complete_type'] = $tableColumn['domain_complete_type'];
         }
 
-        $type                   = $this->_platform->getDoctrineTypeMapping($dbType);
-        $type                   = $this->extractDoctrineTypeFromComment($tableColumn['comment'], $type);
-        $tableColumn['comment'] = $this->removeDoctrineTypeFromComment($tableColumn['comment'], $type);
+        $type = $this->extractDoctrineTypeFromComment($tableColumn['comment'])
+            ?? $this->_platform->getDoctrineTypeMapping($dbType);
 
         switch ($dbType) {
             case 'smallint':

lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php

@@ -88,13 +88,13 @@ class SQLAnywhereSchemaManager extends AbstractSchemaManager
      */
     protected function _getPortableTableColumnDefinition($tableColumn)
     {
-        $type                   = $this->_platform->getDoctrineTypeMapping($tableColumn['type']);
-        $type                   = $this->extractDoctrineTypeFromComment($tableColumn['comment'], $type);
-        $tableColumn['comment'] = $this->removeDoctrineTypeFromComment($tableColumn['comment'], $type);
-        $precision              = null;
-        $scale                  = null;
-        $fixed                  = false;
-        $default                = null;
+        $type = $this->extractDoctrineTypeFromComment($tableColumn['comment'])
+            ?? $this->_platform->getDoctrineTypeMapping($tableColumn['type']);
+
+        $precision = null;
+        $scale     = null;
+        $fixed     = false;
+        $default   = null;
 
         if ($tableColumn['default'] !== null) {
             // Strip quotes from default value.

lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php

@@ -101,9 +101,8 @@ class SQLServerSchemaManager extends AbstractSchemaManager
             $fixed = true;
         }
 
-        $type                   = $this->_platform->getDoctrineTypeMapping($dbType);
-        $type                   = $this->extractDoctrineTypeFromComment($tableColumn['comment'], $type);
-        $tableColumn['comment'] = $this->removeDoctrineTypeFromComment($tableColumn['comment'], $type);
+        $type = $this->extractDoctrineTypeFromComment($tableColumn['comment'])
+            ?? $this->_platform->getDoctrineTypeMapping($dbType);
 
         $options = [
             'length'        => $length === 0 || ! in_array($type, ['text', 'string']) ? null : $length,

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

@@ -283,16 +283,10 @@ class SqliteSchemaManager extends AbstractSchemaManager
 
             $comment = $this->parseColumnCommentFromSQL($columnName, $createSql);
 
-            if ($comment === null) {
-                continue;
-            }
+            $type = $this->extractDoctrineTypeFromComment($comment);
 
-            $type = $this->extractDoctrineTypeFromComment($comment, '');
-
-            if ($type !== '') {
+            if ($type !== null) {
                 $column->setType(Type::getType($type));
-
-                $comment = $this->removeDoctrineTypeFromComment($comment, $type);
             }
 
             $column->setComment($comment);

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

@@ -32,6 +32,7 @@ use Doctrine\DBAL\Types\StringType;
 use Doctrine\DBAL\Types\TextType;
 use Doctrine\DBAL\Types\Type;
 use Doctrine\Tests\DbalFunctionalTestCase;
+use ReflectionMethod;
 use function array_filter;
 use function array_keys;
 use function array_map;
@@ -1458,27 +1459,26 @@ abstract class SchemaManagerFunctionalTestCase extends DbalFunctionalTestCase
      * @dataProvider commentsProvider
      * @group 2596
      */
-    public function testExtractDoctrineTypeFromComment(string $comment, string $expected, string $currentType) : void
+    public function testExtractDoctrineTypeFromComment(?string $comment, ?string $expectedType) : void
     {
-        $result = $this->schemaManager->extractDoctrineTypeFromComment($comment, $currentType);
+        $re = new ReflectionMethod($this->schemaManager, 'extractDoctrineTypeFromComment');
+        $re->setAccessible(true);
 
-        self::assertSame($expected, $result);
+        self::assertSame($expectedType, $re->invokeArgs($this->schemaManager, [&$comment]));
     }
 
     /**
-     * @return string[][]
+     * @return mixed[][]
      */
-    public function commentsProvider() : array
+    public static function commentsProvider() : iterable
     {
-        $currentType = 'current type';
-
         return [
-            'invalid custom type comments'      => ['should.return.current.type', $currentType, $currentType],
-            'valid doctrine type'               => ['(DC2Type:guid)', 'guid', $currentType],
-            'valid with dots'                   => ['(DC2Type:type.should.return)', 'type.should.return', $currentType],
-            'valid with namespace'              => ['(DC2Type:Namespace\Class)', 'Namespace\Class', $currentType],
-            'valid with extra closing bracket'  => ['(DC2Type:should.stop)).before)', 'should.stop', $currentType],
-            'valid with extra opening brackets' => ['(DC2Type:should((.stop)).before)', 'should((.stop', $currentType],
+            'invalid custom type comments'      => ['should.return.null', null],
+            'valid doctrine type'               => ['(DC2Type:guid)', 'guid'],
+            'valid with dots'                   => ['(DC2Type:type.should.return)', 'type.should.return'],
+            'valid with namespace'              => ['(DC2Type:Namespace\Class)', 'Namespace\Class'],
+            'valid with extra closing bracket'  => ['(DC2Type:should.stop)).before)', 'should.stop'],
+            'valid with extra opening brackets' => ['(DC2Type:should((.stop)).before)', 'should((.stop'],
         ];
     }