4.x - Fix snapshot migration tests and index generation #628
Conversation
This file contains 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
|
@markstory @MasterOdin Can we unblock this? Need to figure out where the phinx merge will go. |
ndm2
force-pushed
the
4.x-fix-snapshot-migration-tests-and-index-generation
branch
from
bd1c1d3
to
ab33833
Compare
othercorey
force-pushed
the
4.x-fix-snapshot-migration-tests-and-index-generation
branch
from
ab33833
to
98eb3cb
Compare
ndm2
force-pushed
the
4.x-fix-snapshot-migration-tests-and-index-generation
branch
2 times, most recently
from
346a3b8
to
678f148
Compare
Uses the actual unique index constraints to generate indexes, that way the correct index name is being used, and it won't create indexes that do not actually exist in the schema that is being snapshotted (this might have been done to emulate MySQL, where indexes are automatically created for foreign keys).
This should help to make identifying problems easier, as it's clear whether the name denotes an index or a foreign key, and to which tables they belong.
Uses the actual unique index constraints to generate indexes, that way the correct index name is being used, and it won't create indexes that do not actually exist in the schema that is being snapshotted (this might have been done to emulate MySQL, where indexes are automatically created for foreign keys).
This should help to make identifying problems easier, as it's clear whether the name denotes an index or a foreign key, and to which tables they belong.
othercorey
force-pushed
the
4.x-fix-snapshot-migration-tests-and-index-generation
branch
from
678f148
to
741ff9b
Compare
|
Rebased with phinx 0.15.1 support. |
othercorey
deleted the
4.x-fix-snapshot-migration-tests-and-index-generation
branch
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.
This depends on / is blocked by
cakephp/cakephp#17256 andcakephp/phinx#2212.Without the test that this PR re-enables it has gone unnoticed at the time, but #617 uncovered some weird behavior, where Migrations will filter out all indexes that are created over the same columns as any foreign key, and then it uses the foreign keys in the templates to generate
addIndex()calls for them, which results in the original index names getting lost, and indexes being created that do not necessarily exist.I'm not quite sure whether that is a bug, or if it was intended in order to emulate how MySQL (the DBMS, not Migrations) would automatically created indexes in the source table when creating foreign keys (should no index already exist that covers it). The original code stems from a huge pile of changes that were part of the initial CakePHP 3 compatibility revamp, and there's no explanation or anything.
Personally I don't like it, I'd neither want to loose the index names, nor would I want a snapshot to create indexes that do not actually exist in my schema. So alongside of making the snapshot migration test work again, I would like to suggest making the index handling more strict, and only create
addIndex()calls for indexes that actually exist.