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

Introduce table editor #6660

Merged
merged 4 commits into from
Dec 22, 2024
Merged

Introduce table editor #6660

merged 4 commits into from
Dec 22, 2024

Conversation

morozov
Copy link
Member

@morozov morozov commented Dec 22, 2024

Q A
Type feature

Motivation

The introduction of the table editor should facilitate the following improvements:

  1. Eventually make tables immutable.
  2. Reduce the amount of validation that needs to be performed in the case of multi-step modification of a table (see Deprecate dropping columns referenced by constraints #6559 (comment)).
  3. Get rid of this hack:

    dbal/src/Schema/Schema.php

    Lines 305 to 309 in e7e2615

    $identifier = new Identifier($newName);
    $table->_name = $identifier->_name;
    $table->_namespace = $identifier->_namespace;
    $table->_quoted = $identifier->_quoted;
    I just realized that replacing it with building a new table would be a BC break because the editor would create a new table instance instead of modifying an existing one.
  4. Unify the process of table creation. For instance, currently, table columns can be specified in the constructor or added via addColumn() but the primary key can be set only via setPrimaryKey().

Being able to get rid of the hack is why I'm introducing the editor now. I want to start deprecating end removing the name-related properties of AbstractIdentifier in favor of the recently introduced value objects, but this hack won't with the new properties because they are private (not protected).

Implementation details

The editor (specifically, the class and method naming) is modeled after the table editor in Debezium.

I implemented the necessary minimum of the methods that would allow to rework the hack but then realized that it's a BC break. However, this API allows to replace the direct usage of the Table constructor in the library code with the builder, so I think we can introduce it as is.

I didn't touch the tests because

  1. This is a lot of mechanical work (I can do it later).
  2. The test code that uses the builder won't be as streamlined as the library code because the builder doesn't yet support all initial table properties (e.g. the primary key).

Additional changes

The first two commits make minor changes in the code that wasn't released yet.

@@ -12,8 +12,7 @@ interface Name
/**
* Returns the string representation of the name.
*
* The consumers of this method should not rely on a specific return value. It should be used only for diagnostic
* purposes.
* If passed to the corresponding parser, the name should be parsed back to an equivalent object.
Copy link
Member Author

Choose a reason for hiding this comment

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

This contract enables this expression:

->setName($this->getObjectName()->toString())

public function edit(): TableEditor
{
return self::editor()
->setName($this->getObjectName()->toString())
Copy link
Member Author

@morozov morozov Dec 22, 2024

Choose a reason for hiding this comment

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

Even though getObjectName() can throw NameIsNotInitialized, this isn't a BC break because Table::edit() is a new method not called from anywhere. It will be used to rename the table in 5.0.

@morozov morozov marked this pull request as ready for review December 22, 2024 04:11
@morozov morozov requested review from greg0ire and derrabus December 22, 2024 04:13
@morozov morozov merged commit 03049bc into doctrine:4.3.x Dec 22, 2024
67 checks passed
@morozov morozov deleted the table-editor branch December 22, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants