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

Reworked AbstractSchemaManager::extractDoctrineTypeFromComment(), removed ::removeDoctrineTypeFromComment()
doctrine · Apr 16, 2019 · 6daf933 · 6daf933
2 parents ba45fbd + 6b5d226
commit 6daf933
Show file tree

Hide file tree

Showing 10 changed files with 44 additions and 75 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,9 @@
 # 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.

diff --git a/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php b/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php
@@ -23,7 +23,6 @@
 use function is_array;
 use function is_callable;
 use function preg_match;
-use function str_replace;
 use function strtolower;
 
 /**
@@ -1099,35 +1098,19 @@ public function getSchemaSearchPaths()
     }
 
     /**
-     * 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];
     }
 }
diff --git a/lib/Doctrine/DBAL/Schema/DB2SchemaManager.php b/lib/Doctrine/DBAL/Schema/DB2SchemaManager.php
@@ -56,12 +56,8 @@ protected function _getPortableTableColumnDefinition($tableColumn)
             $default = trim($tableColumn['default'], "'");
         }
 
-        $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':

diff --git a/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php b/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php
@@ -108,13 +108,8 @@ protected function _getPortableTableColumnDefinition($tableColumn)
         $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':

diff --git a/lib/Doctrine/DBAL/Schema/OracleSchemaManager.php b/lib/Doctrine/DBAL/Schema/OracleSchemaManager.php
@@ -160,9 +160,8 @@ protected function _getPortableTableColumnDefinition($tableColumn)
             $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':

diff --git a/lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php b/lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php
@@ -366,9 +366,8 @@ protected function _getPortableTableColumnDefinition($tableColumn)
             $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':

diff --git a/lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php b/lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php
@@ -88,13 +88,13 @@ protected function _getPortableSequenceDefinition($sequence)
      */
     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.

diff --git a/lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php b/lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php
@@ -101,9 +101,8 @@ protected function _getPortableTableColumnDefinition($tableColumn)
             $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,

diff --git a/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php b/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php
@@ -281,16 +281,10 @@ protected function _getPortableTableColumnList($table, $database, $tableColumns)
 
             $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);

diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
@@ -32,6 +32,7 @@
 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;
@@ -1440,27 +1441,26 @@ public function testComparatorShouldNotAddCommentToJsonTypeSinceItIsTheDefaultNo
      * @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'],
         ];
     }