Preserve quoting when detecting renamed indexes - #7507
Open
raphyabak wants to merge 1 commit into
Open
Conversation
Comparator::detectRenamedIndexes() used strtolower($index->getName())
as the array key for a renamed index's old name. getName() strips
quoting metadata and returns the bare identifier, so any information
about whether the original index name was quoted was lost, and the
name was unconditionally folded to lower case regardless of the
target platform's identifier folding rules.
Downstream, AbstractPlatform::getAlterTableSQL() re-wraps this bare
string in a new Identifier() to decide whether to quote the old name
in the generated SQL. Since the quoting information was already gone,
the old name was never quoted, even when it originally required
quoting to preserve case (e.g. Oracle, which folds unquoted
identifiers to upper case). This produced statements like:
ALTER INDEX idx_test_col RENAME TO "idx_test_type"
instead of:
ALTER INDEX "idx_test_col" RENAME TO "idx_test_type"
which Oracle rejects with an index-not-found error, since the
generated statement no longer refers to the original, case-sensitive
index name.
Use getQuotedName($this->platform) instead, which already contains
the exact logic needed to conditionally quote the identifier (based
on whether it was originally quoted, or is a reserved keyword) while
leaving already-safe unquoted names untouched.
Fixes doctrine#6565
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #6565
Comparator::detectRenamedIndexes()usedstrtolower($index->getName())as the array key for a renamed index's old name.getName()strips quoting metadata and returns the bare identifier, so any information about whether the original index name was quoted was lost, and the name was unconditionally folded to lower case regardless of the target platform's identifier folding rules.Downstream,
AbstractPlatform::getAlterTableSQL()re-wraps this bare string in anew Identifier()to decide whether to quote the old name in the generated SQL. Since the quoting information was already gone, the old name was never quoted, even when it originally required quoting to preserve case (e.g. Oracle, which folds unquoted identifiers to upper case). This produced statements like:instead of:
which Oracle rejects with an index-not-found error, since the generated statement no longer refers to the original, case-sensitive index name.
Fix
Use
getQuotedName($this->platform)instead, which already contains the exact logic needed to conditionally quote the identifier (based on whether it was originally quoted, or is a reserved keyword) while leaving already-safe unquoted names untouched — this is the same mechanismAbstractPlatform::getAlterTableSQL()and other index/column name handling already rely on elsewhere.Test plan
AbstractComparatorTestCase::testDetectRenameIndexPreservesQuoting(), exercised against all three concreteComparatorsubclasses (SQLite, MySQL, SQLServer) so the assertion works across each platform's own quoting character (",`,[...]).OraclePlatform+Comparator::compareTables(): before the fix,ALTER INDEX idx_test_col RENAME TO "idx_test_type"; after the fix,ALTER INDEX "idx_test_col" RENAME TO "idx_test_type".vendor/bin/phpunit tests --exclude-group=performance): 3932 tests, all passing (3929 pre-existing + 3 new), same pre-existing skip/incomplete counts (604 skipped / 13 incomplete, all DB-connection-dependent functional tests unrelated to this change).vendor/bin/phpcsandvendor/bin/phpstanon the changed files: clean.