Merge pull request #6646 from morozov/object-names

Introduce value objects representing database object names

Author: morozov

UPGRADE.md

@@ -8,6 +8,18 @@ awareness about deprecated code.
 
 # Upgrade to 4.3
 
+## Deprecated using invalid database object names
+
+Using the following objects with an empty name is deprecated: `Column`, `View`, `Sequence`, `Identifier`.
+
+Using the following objects with a qualified name is deprecated: `Column`, `ForeignKeyConstraint`, `Index`, `Schema`,
+`UniqueConstraint`. If the object name contains a dot, the name should be quoted.
+
+Using the following objects with a name that has more than one qualifier is deprecated: `Sequence`, `Table`, `View`.
+The name should be unqualified or contain one qualifier.
+
+The `AbstractAsset` class has been marked as internal.
+
 ## Deprecated configuration-related `Table` methods
 
 The `Table::setSchemaConfig()` method and `$_schemaConfig` property have been deprecated. Pass a `TableConfiguration`

phpcs.xml.dist

@@ -76,6 +76,7 @@
     <!-- see https://github.com/squizlabs/PHP_CodeSniffer/issues/2099 -->
     <rule ref="Squiz.Commenting.FunctionComment.InvalidNoReturn">
         <exclude-pattern>src/Platforms/AbstractPlatform.php</exclude-pattern>
+        <exclude-pattern>src/Schema/AbstractAsset.php</exclude-pattern>
         <exclude-pattern>src/Schema/AbstractSchemaManager.php</exclude-pattern>
         <exclude-pattern>tests/Platforms/AbstractPlatformTestCase.php</exclude-pattern>
     </rule>
@@ -128,13 +129,14 @@
         <exclude-pattern>src/Driver/SQLSrv/Statement.php</exclude-pattern>
     </rule>
 
-    <!-- This could be considered a bug in the sniff or the shortcoming of the Platform API design
+    <!-- False positives caused by the limitation of PHP_CodeSniffer
          see https://github.com/squizlabs/PHP_CodeSniffer/issues/3035 -->
     <rule ref="Generic.CodeAnalysis.UselessOverridingMethod.Found">
-        <exclude-pattern>src/Platforms/SQLitePlatform.php</exclude-pattern>
         <exclude-pattern>src/Platforms/*/Comparator.php</exclude-pattern>
         <exclude-pattern>src/Driver/PDO/SQLSrv/Connection.php</exclude-pattern>
         <exclude-pattern>src/Driver/PDO/SQLSrv/Statement.php</exclude-pattern>
+        <exclude-pattern>src/Schema/AbstractNamedObject.php</exclude-pattern>
+        <exclude-pattern>src/Schema/Name/UnqualifiedName.php</exclude-pattern>
     </rule>
 
     <!-- This issue is likely fixed in Slevomat Coding Standard 8.0.0 -->

psalm.xml.dist

@@ -115,6 +115,13 @@
                 -->
                 <referencedMethod name="Doctrine\DBAL\Schema\Table::_getMaxIdentifierLength" />
                 <referencedMethod name="Doctrine\DBAL\Schema\Table::setSchemaConfig" />
+
+                <!--
+                    https://github.com/doctrine/dbal/pull/6646
+                    TODO: remove in 5.0.0
+                -->
+                <referencedMethod name="Doctrine\DBAL\Schema\AbstractAsset::getNameParser" />
+                <referencedMethod name="Doctrine\DBAL\Schema\AbstractAsset::setName" />
             </errorLevel>
         </DeprecatedMethod>
         <DeprecatedProperty>
@@ -206,6 +213,14 @@
                 <file name="tests/Driver/PDO/*/DriverTest.php"/>
                 <file name="tests/Functional/Driver/Mysqli/ConnectionTest.php"/>
                 <file name="tests/Platforms/AbstractPlatformTestCase.php"/>
+
+                <!--
+                    https://github.com/doctrine/dbal/pull/6646
+                    TODO: remove in 5.0.0
+
+                    This looks like a Psalm bug.
+                -->
+                <referencedFunction name="Doctrine\DBAL\Schema\AbstractAsset::setName"/>
             </errorLevel>
         </InvalidArgument>
         <InvalidArrayOffset>
@@ -312,6 +327,21 @@
         </PossiblyFalseReference>
         <PropertyNotSetInConstructor>
             <errorLevel type="suppress">
+                <!--
+                    https://github.com/doctrine/dbal/pull/6646
+                    TODO: remove in 5.0.0
+                -->
+                <referencedProperty name="Doctrine\DBAL\Schema\AbstractAsset::$name"/>
+
+                <!--
+                    https://github.com/doctrine/dbal/pull/6646
+                    TODO: remove in 5.0.0
+
+                    For some reason, referencing the property name doesn't work. Furthermore,
+                    the class doesn't reference the property.
+                -->
+                <file name="src/Schema/Table.php"/>
+
                 <!-- See https://github.com/psalm/psalm-plugin-phpunit/issues/107 -->
                 <!-- See https://github.com/sebastianbergmann/phpunit/pull/4610 -->
                 <directory name="tests"/>

src/Schema/AbstractAsset.php

@@ -5,9 +5,14 @@
 namespace Doctrine\DBAL\Schema;
 
 use Doctrine\DBAL\Platforms\AbstractPlatform;
+use Doctrine\DBAL\Schema\Exception\NotImplemented;
+use Doctrine\DBAL\Schema\Name\GenericName;
+use Doctrine\DBAL\Schema\Name\Identifier;
+use Doctrine\DBAL\Schema\Name\OptionallyQualifiedName;
 use Doctrine\DBAL\Schema\Name\Parser;
-use Doctrine\DBAL\Schema\Name\Parser\Identifier;
+use Doctrine\DBAL\Schema\Name\UnqualifiedName;
 use Doctrine\Deprecations\Deprecation;
+use Throwable;
 
 use function array_map;
 use function count;
@@ -28,11 +33,20 @@
  * This encapsulation hack is necessary to keep a consistent state of the database schema. Say we have a list of tables
  * array($tableName => Table($tableName)); if you want to rename the table, you have to make sure this does not get
  * recreated during schema migration.
+ *
+ * @internal This class should be extended only by DBAL itself.
+ *
+ * @template N of Name
  */
 abstract class AbstractAsset
 {
     protected string $_name = '';
 
+    /**
+     * Indicates whether the object name has been initialized.
+     */
+    protected bool $isNameInitialized = false;
+
     /**
      * Namespace of the asset. If none isset the default namespace is assumed.
      */
@@ -61,13 +75,39 @@ public function __construct(?string $name = null)
         $this->_setName($name);
     }
 
+    /**
+     * Returns a parser for parsing the object name.
+     *
+     * @deprecated Parse the name in the constructor instead.
+     *
+     * @return Parser<N>
+     */
+    protected function getNameParser(): Parser
+    {
+        throw NotImplemented::fromMethod(static::class, __FUNCTION__);
+    }
+
+    /**
+     * Sets the object name.
+     *
+     * @deprecated Set the name in the constructor instead.
+     *
+     * @param ?N $name
+     */
+    protected function setName(?Name $name): void
+    {
+        throw NotImplemented::fromMethod(static::class, __FUNCTION__);
+    }
+
     /**
      * Sets the name of this asset.
      *
      * @deprecated Use the constructor instead.
      */
     protected function _setName(string $name): void
     {
+        $this->isNameInitialized = false;
+
         Deprecation::triggerIfCalledFromOutside(
             'doctrine/dbal',
             'https://github.com/doctrine/dbal/pull/6610',
@@ -93,11 +133,9 @@ protected function _setName(string $name): void
         $this->validateFuture = false;
 
         if ($input !== '') {
-            $parser = new Parser();
-
             try {
-                $identifiers = $parser->parse($input);
-            } catch (Parser\Exception $e) {
+                $parsedName = $this->getNameParser()->parse($input);
+            } catch (Throwable $e) {
                 Deprecation::trigger(
                     'doctrine/dbal',
                     'https://github.com/doctrine/dbal/pull/6592',
@@ -108,14 +146,46 @@ protected function _setName(string $name): void
                 return;
             }
         } else {
-            $identifiers = [];
+            $parsedName = null;
         }
 
-        switch (count($identifiers)) {
-            case 0:
-                $this->identifiers = [];
+        try {
+            $this->setName($parsedName);
+        } catch (Throwable $e) {
+            Deprecation::trigger(
+                'doctrine/dbal',
+                'https://github.com/doctrine/dbal/pull/6646',
+                'Using invalid database object names is deprecated: %s.',
+                $e->getMessage(),
+            );
 
-                return;
+            return;
+        }
+
+        $this->isNameInitialized = true;
+
+        if ($parsedName === null) {
+            $this->identifiers = [];
+
+            return;
+        }
+
+        if ($parsedName instanceof UnqualifiedName) {
+            $identifiers = [$parsedName->getIdentifier()];
+        } elseif ($parsedName instanceof OptionallyQualifiedName) {
+            $unqualifiedName = $parsedName->getUnqualifiedName();
+            $qualifier       = $parsedName->getQualifier();
+
+            $identifiers = $qualifier !== null
+                ? [$qualifier, $unqualifiedName]
+                : [$unqualifiedName];
+        } elseif ($parsedName instanceof GenericName) {
+            $identifiers = $parsedName->getIdentifiers();
+        } else {
+            return;
+        }
+
+        switch (count($identifiers)) {
             case 1:
                 $namespace = null;
                 $name      = $identifiers[0];
@@ -127,13 +197,6 @@ protected function _setName(string $name): void
                 break;
 
             default:
-                Deprecation::trigger(
-                    'doctrine/dbal',
-                    'https://github.com/doctrine/dbal/pull/6592',
-                    'An object name may consist of at most 2 identifiers (<namespace>.<name>), %d given.',
-                    count($identifiers),
-                );
-
                 return;
         }
 

src/Schema/AbstractNamedObject.php

@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\DBAL\Schema;
+
+use Doctrine\DBAL\Schema\Exception\InvalidName;
+use Doctrine\DBAL\Schema\Exception\NameIsNotInitialized;
+
+/**
+ * An abstract {@see NamedObject}.
+ *
+ * @template N of Name
+ * @extends AbstractAsset<N>
+ * @implements NamedObject<N>
+ */
+abstract class AbstractNamedObject extends AbstractAsset implements NamedObject
+{
+    /**
+     * The name of the database object.
+     *
+     * Until the validity of the name is enforced, this property isn't guaranteed to be always initialized. The property
+     * can be accessed only if {@see $isNameInitialized} is set to true.
+     *
+     * @var N
+     */
+    protected Name $name;
+
+    public function __construct(string $name)
+    {
+        parent::__construct($name);
+    }
+
+    /**
+     * Returns the object name.
+     *
+     * @return N
+     *
+     * @throws NameIsNotInitialized
+     */
+    public function getObjectName(): Name
+    {
+        if (! $this->isNameInitialized) {
+            throw NameIsNotInitialized::new();
+        }
+
+        return $this->name;
+    }
+
+    protected function setName(?Name $name): void
+    {
+        if ($name === null) {
+            throw InvalidName::fromEmpty();
+        }
+
+        $this->name = $name;
+    }
+}

src/Schema/AbstractOptionallyNamedObject.php

@@ -0,0 +1,46 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\DBAL\Schema;
+
+use Doctrine\DBAL\Schema\Exception\NameIsNotInitialized;
+
+/**
+ * An abstract {@see OptionallyNamedObject}.
+ *
+ * @template N of Name
+ * @extends AbstractAsset<N>
+ * @implements OptionallyNamedObject<N>
+ */
+abstract class AbstractOptionallyNamedObject extends AbstractAsset implements OptionallyNamedObject
+{
+    /**
+     * The name of the database object.
+     *
+     * Until the validity of the name is enforced, this property isn't guaranteed to be always initialized. The property
+     * can be accessed only if {@see $isNameInitialized} is set to true.
+     *
+     * @var ?N
+     */
+    protected ?Name $name;
+
+    public function __construct(?string $name)
+    {
+        parent::__construct($name ?? '');
+    }
+
+    public function getObjectName(): ?Name
+    {
+        if (! $this->isNameInitialized) {
+            throw NameIsNotInitialized::new();
+        }
+
+        return $this->name;
+    }
+
+    protected function setName(?Name $name): void
+    {
+        $this->name = $name;
+    }
+}

src/Schema/Column.php

@@ -5,15 +5,20 @@
 namespace Doctrine\DBAL\Schema;
 
 use Doctrine\DBAL\Schema\Exception\UnknownColumnOption;
+use Doctrine\DBAL\Schema\Name\Parser\UnqualifiedNameParser;
+use Doctrine\DBAL\Schema\Name\Parsers;
+use Doctrine\DBAL\Schema\Name\UnqualifiedName;
 use Doctrine\DBAL\Types\Type;
 
 use function array_merge;
 use function method_exists;
 
 /**
  * Object representation of a database column.
+ *
+ * @extends AbstractNamedObject<UnqualifiedName>
  */
-class Column extends AbstractAsset
+class Column extends AbstractNamedObject
 {
     protected Type $_type;
 
@@ -56,6 +61,11 @@ public function __construct(string $name, Type $type, array $options = [])
         $this->setOptions($options);
     }
 
+    protected function getNameParser(): UnqualifiedNameParser
+    {
+        return Parsers::getUnqualifiedNameParser();
+    }
+
     /** @param array<string, mixed> $options */
     public function setOptions(array $options): self
     {