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

Reverts #268 and properly fix what was failing #279

Merged
merged 4 commits into from
Dec 14, 2016
Merged

Conversation

HavokInspiration
Copy link
Member

#268 was a mistake.
This should properly fix it.

#268 introduced a regression when baking a diff : you ended up with all your schema being in the diff where existing tables / columns had a changeColumn registered for no reason.

@HavokInspiration
Copy link
Member Author

Oh, and I also removed 5.4 from the tests matrix.

@HavokInspiration
Copy link
Member Author

HavokInspiration commented Dec 8, 2016

On second thoughts, this could still be a problem the other way around.
The root cause of the problem is from the core : the per-column collation was added. Which means that when a table is reflected to its SchemaTable object counter-part, each column now has a new collation column.

So if you had a dump from before 3.2.12 (when per column collation was added), your dump did not contain the collation column. If you were to update to 3.2.12 and bake a diff, you'd experience the problem described in the original post of the PR.
If you had a dump from after (with the collation column) but used migrations 1.6.4, you had the problem I described above (that's how I encoutered it : CakePHP 3.3.9 and migrations 1.6.4 and my full schema was in the diff file each time).

So it should be known that if people with a dump that does not have the collation column now bake a diff, they will end up with all their schema being in the diff where columns from existing tables will have a changeColumn call attached.

@HavokInspiration HavokInspiration added this to the 1.6.5 milestone Dec 8, 2016
@HavokInspiration
Copy link
Member Author

HavokInspiration commented Dec 9, 2016

Or we could remove collation from both the schema from the dump and from the database and we would not have a problem anymore whatever the versions of Cake / plugin used.

Since phinx does not support per-column collation (last time I checked), this is actually not a problem to remove it.

Any opinion on this ?

…m the dump to prevent undesired side-effects
@HavokInspiration
Copy link
Member Author

I pushed a new commit where I play defensive and unset the collate option from both the schema read from the table and the schema from the dump. It seems safer this way and prevents undesired side-effects.
And since per-column collation is not supported right now, this should not be such a big-deals.

This PR was reported in issue #283 as well.

@codecov-io
Copy link

Current coverage is 84.15% (diff: 100%)

Merging #279 into master will increase coverage by <.01%

@@             master       #279   diff @@
==========================================
  Files            31         31          
  Lines          1671       1672     +1   
  Methods         213        213          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1406       1407     +1   
  Misses          265        265          
  Partials          0          0          

Powered by Codecov. Last update a7d36f1...5228366

@lorenzo
Copy link
Member

lorenzo commented Dec 14, 2016

Hmm too bad phone does not support collate. This fix makes sense to me

@lorenzo lorenzo merged commit 54ffe95 into master Dec 14, 2016
@HavokInspiration HavokInspiration deleted the test-mysql-build branch December 14, 2016 09:18
@HavokInspiration
Copy link
Member Author

@lorenzo The only real collate support I have seen in phinx is with MySQL on a per-table basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants