Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate the Constraint interface #4839

Merged
merged 1 commit into from
Oct 2, 2021

Conversation

morozov
Copy link
Member

@morozov morozov commented Oct 2, 2021

Q A
Type deprecation, improvement
BC Break no

Summary

The interface attempts to unify the API for different constraint classes but does it poorly.

  1. Indeed, both indices and unique constraints are defined with a list of columns but the foreign key constraint is defined with two sets of columns: referring (a.k.a. local) and referred (a.k.a. foreign). The getColumns() method on a ForeignKeyConstraint class doesn't make any sense (implies the only set of columns) but has to be implemented:
    * @see getLocalColumns
    */
    public function getColumns()
    {
    return $this->getLocalColumns();
    }
  2. There are other constraints in SQL like CHECK, DEFAULT, NOT NULL, etc. that do not use a list of columns in their definition.
  3. The AbstractPlatform::getCreateConstraintSQL() method accepts a Constraint and by its signature is supposed to be capable of creating any constraint. But it only implements handling some specific sub-classes:
    if ($constraint instanceof Index) {
    if ($constraint->isPrimary()) {
    $query .= ' PRIMARY KEY';
    } elseif ($constraint->isUnique()) {
    $query .= ' UNIQUE';
    } else {
    throw new InvalidArgumentException(
    'Can only create primary or unique constraints, no common indexes with getCreateConstraintSQL().'
    );
    }
    } elseif ($constraint instanceof UniqueConstraint) {
    $query .= ' UNIQUE';
    } elseif ($constraint instanceof ForeignKeyConstraint) {
    $query .= ' FOREIGN KEY';
    $referencesClause = ' REFERENCES ' . $constraint->getQuotedForeignTableName($this) .
    ' (' . implode(', ', $constraint->getQuotedForeignColumns($this)) . ')';
    }
    If a new class implementing the Constraint method is added, this method will have to be reworked which violates the Open-closed principle.

Additionally

The current implementation of AbstractPlatform::getDropConstraintSQL() doesn't work on MySQL older than 8.0.19 and MariaDB (see SchemaManagerFunctionalTestCase::testDropAndCreateUniqueConstraint()).

Proposed changes

  1. Deprecate the Constraint interface and the corresponding methods.
  2. Mark AbstractPlatform::getDropConstraintSQL() as @internal and convert to protected in the next major release. This method generates valid SQL and could be used by the methods that drop specific constraints.
  3. Introduce drop/create methods for unique constraints in AbstractPlatform and AbstractSchemaManager.
  4. Do not introduce AbstractSchemaManager::dropAndCreateUniqueConstraint(). It's hard to think of the usefulness of such "drop-and-create" methods outside of integration testing. These methods are primarily untested, do not allow proper static analysis, and suppress all underlying exceptions thrown by the "drop" part. We can deprecate them separately.
  5. The proper implementation of OraclePlatform::getCreatePrimaryKeySQL() allows using createIndex() to create a primary key in the test instead of createConstraint(). This is still not the best API but at least it's consistent with the rest of the platforms.

@morozov morozov merged commit bda52c5 into doctrine:3.2.x Oct 2, 2021
@morozov morozov deleted the deprecate-constraint branch October 2, 2021 15:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants