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 modified indexes in TableDiff #6831

Merged
merged 2 commits into from
Mar 9, 2025

Conversation

morozov
Copy link
Member

@morozov morozov commented Mar 9, 2025

The same reasoning applies as in #6827.

The @throws Exception annotations being removed from AbstractMySQLPlatform seem to have been mistakenly introduced as a result of conflict resolution during a merge up (2.11.x → 3.0.x).

See the details about the changes in PostgreSQLPlatform inline.

@morozov morozov force-pushed the deprecate-modified-indexes branch from 3ad9ce0 to f33f986 Compare March 9, 2025 16:17

return $this->getDropConstraintSQL($constraintName, $table);
if ($name === '"primary"' || $name === $primaryKeyName) {
Copy link
Member Author

@morozov morozov Mar 9, 2025

Choose a reason for hiding this comment

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

This deprecation is not as smooth as the previous one. The change in this condition is needed to prevent SchemaManagerFunctionalTestCase#testSwitchPrimaryKeyOrder() from failing. Technically, this is a bug.

The following is happening under the hood:

  1. As part of this deprecation, instead of adding the new index to the "modified" list, the comparator adds the old and the new indexes to the corresponding collections.
  2. In the test, the new index is created as part of the new table definition, so its name is "primary"; the old index is introspected from the database, and its name is "<table>_pkey".
  3. When the schema manager attempts to drop the old index, the hack that compares the index name with "primary" no longer kicks in (previously, the schema manager would drop the new index, and now it drops the old one).

The whole point of this deprecation is to lay the groundwork for the introduction of the PrimaryKeyConstraint as the first-class citizen, which will allow to get rid of this hack and implement proper schema management on all platforms. So I believe, the risk of breaking some edge case (and then fixing it in a subsequent patch release) is justified.

An alternative might be to have the comparator add the new index both to the old and new index collections (to mimic the old behavior), but that would end up being a Postgres-specific hack in the platform-agnostic comparator, which would have to be fixed (BC-broken) later.

@morozov morozov marked this pull request as ready for review March 9, 2025 16:40
@morozov morozov requested a review from greg0ire March 9, 2025 16:40
@morozov morozov merged commit 2599612 into doctrine:4.3.x Mar 9, 2025
68 checks passed
@morozov morozov deleted the deprecate-modified-indexes branch March 9, 2025 21:43
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