Fix foreign key name change detection #6413
Conversation
achterin
force-pushed
the
bugfix/foreign_key_name_change_detection
branch
2 times, most recently
from
6603566 to
e51eb77
Compare
Please add links showing that. |
Sure, no problem. This changeset introduced the index-rename support and changed the corresponding test. As you can see, before this patch the test expected an empty table-diff - same as for the fk-name test below. a996bea#diff-1e412c2c0134199408e114ba49a15f2019f3ccbac5472c3815f59c815f810e45 |
|
Since this is a bugfix, and it affects 3.x, you should retarget this PR on 3.8.x (and rebase on that). |
|
Okay, I thought it would be the other way around, so fix it first in the default branch and then backport it. Will do. |
achterin
force-pushed
the
bugfix/foreign_key_name_change_detection
branch
from
e51eb77 to
25a2d03
Compare
|
@greg0ire sadly i was unable to adapt my exact changes to the 3.8.x branch, thats why i did reduce the test down to just checking if one foreign key was added and one removed. i also used the deprecated diffTable function to be in line with the other tests. let me know what you think. thanks! |
|
I think you should open another PR after we merge this up to adapt the test. |
As index renaming support was introduced a while back do the same for foreign key name changes. Signed-off-by: Christoph Krapp <christoph.krapp@power.cloud>
Both tests did expect a name change to result in an empty table diff in the past. When index renaming support was introduced the name of the test was not changed to reflect the new expectation. Signed-off-by: Christoph Krapp <christoph.krapp@power.cloud>
achterin
force-pushed
the
bugfix/foreign_key_name_change_detection
branch
from
25a2d03 to
422e459
Compare
|
Thanks @achterin ! |
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
- this is the version where indexes detection has changed - see doctrine/dbal#6413
Revert "Merge pull request #6413 from achterin/bugfix/foreign_key_name_change_detection"
* 3.8.x: PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection" CI: Update MariaDB versions (doctrine#6426) CI MariaDB: add 11.4, remove 11.0 (doctrine#6432) Fix typo in the portability documentation (doctrine#6430)
* 3.8.x: Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection" CI: Update MariaDB versions (doctrine#6426) CI MariaDB: add 11.4, remove 11.0 (doctrine#6432) Fix typo in the portability documentation (doctrine#6430)
* 3.8.x: Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection"
* 4.0.x: PHPUnit 10.5.21 (doctrine#6447) Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection"
* 4.1.x: (25 commits) Simplify signature of fetchTableOptionsByTable Add MariaDb1010Platform for fetchTableOptionsByTable PHPUnit 10.5.21 (doctrine#6447) Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Default to distinct union queries (doctrine#6439) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection" Add `QueryBuilder` support for `UNION` clause (doctrine#6369) CI: Update MariaDB versions (doctrine#6426) CI MariaDB: add 11.4, remove 11.0 (doctrine#6432) Display warnings when running PHPUnit in CI (doctrine#6431) Fix typo in the portability documentation (doctrine#6430) Fix MariaDB fetching of default table character-set (doctrine#6361) (doctrine#6425) Fix the portability documentation (doctrine#6429) Update tests/Platforms/AbstractPlatformTestCase.php Update tests/Platforms/AbstractPlatformTestCase.php add test Fix: Skip type comparison if disableTypeComments is true Remove redundant variable (doctrine#6326) Fix test names to reflect their actual purpose ...
|
@greg0ire @achterin sorry to bother you, but this fix is critical for our production environment and it seems to have not made it to 4.1 at the moment. Where did this end up in? @dmaicher you reverted the fix on June 17th (https://github.com/doctrine/dbal/commits/4.1.x/src/Schema/Comparator.php), but afterwards the merge of 3.8 didn't re-add it!? I'm confused. I'm facing different production systems that cannot migrate because they appear to have accumulated different FK names over some time, so an FK drop in a migration fails horribly. |
|
If the bug resurfaced on 4.1, please submit a new PR against the 4.1 branch. |
I'm sorry, it was reverted on purpose, a bit hard to track down. |
|
Okay. Do you want to open a new issue then, explaining the problem that you have? |
Yes, I just did: #6518 |
Summary
This changes how the schema comparator treats foreign key name changes.
The index renaming had the same behavior in the past, expecting a index name change to result in an empty table diff. This was changed some time ago but foreign key changes were still not picked up by the comparator. This pr will change that.
While at it, I've also changed the names of both tests to reflect the changes.