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

Remove support for passing assets as names #4797

Merged
merged 3 commits into from
Sep 19, 2021

Conversation

morozov
Copy link
Member

@morozov morozov commented Sep 17, 2021

Q A
Type improvement
BC Break yes

Summary

The current Platform and Schema APIs allow passing object names as strings or as objects themselves (sub-classes of Asset). For instance, in order to drop a table, the caller can pass the table name or the Table object.

While it may look handly from the API consumer perspective, it violates the Interface Segregation Principle. In the example above, the getDropTableSQL() method has to support accepting an object although it only needs the name of the object.

Another problem is that this API is prone to being implemented inconsistently depending on the type of the actually passed argument. For instance:

  1. MySQL will drop the PK using ALTER TABLE tbl DROP PRIMARY KEY only if the index is passed as an object since the information about whether the given index is the PK or not isn't encoded in the index name:
    if ($index instanceof Index && $index->isPrimary()) {
    // MySQL primary keys are always named "PRIMARY",
    // so we cannot use them in statements because of them being keyword.
    return $this->getDropPrimaryKeySQL($table);
    }
  2. The Table class will validate the columns of the foreign key against the referenced table only if the referenced table name is passed as an asset:

    dbal/src/Schema/Table.php

    Lines 354 to 360 in f83121f

    if ($foreignTable instanceof Table) {
    foreach ($foreignColumnNames as $columnName) {
    if (! $foreignTable->hasColumn($columnName)) {
    throw ColumnDoesNotExist::new($columnName, $foreignTable->getName());
    }
    }
    }

Another practical problem is that it cripples the Events API:

/**
* @return string|Table
*/
public function getTable()

The consumer of the SchemaDropTableEventArgs class has to support receiving the table name expressed as either of the string and Table.

As part of prototyping #4772, I'd like to convert all string $name arguments into Name $name. This is where the need to drop the support for Asset|string arguments arose.

TODO:

@morozov morozov force-pushed the remove-assets-as-names branch from af07d53 to 772ff18 Compare September 17, 2021 01:11
@morozov morozov added this to the 4.0.0 milestone Sep 17, 2021
@morozov morozov marked this pull request as ready for review September 17, 2021 01:31
@morozov morozov requested a review from greg0ire September 17, 2021 01:31
@morozov morozov force-pushed the remove-assets-as-names branch from 772ff18 to 6f3b9c4 Compare September 17, 2021 04:04
@morozov morozov merged commit c1bc279 into doctrine:4.0.x Sep 19, 2021
@morozov morozov deleted the remove-assets-as-names branch September 19, 2021 14:49
Copy link
Contributor

@mvorisek mvorisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2023
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