BC break mitigation strategy

[dereuromark]

#1899
Since Phinx 0.13, foreign key columns have to be specified with "signed" => false

Also refs cakephp/migrations#599

That causes now BC breaks in all kind of applications due to plugins not containing this

PDOException: SQLSTATE[HY000]: General error: 1005 Can't create table release_local.state_machine_item_states (errno: 150 "Foreign key constraint is incorrectly formed") in /shared/httpd/release/vendor/robmorgan/phinx/src/Phinx/Db/Adapter/PdoAdapter.php:192

due to https://github.com/spryker/cakephp-statemachine/blob/81b6ca3c9fddf5dbacc8d863f4096f77a9d6c4c5/config/Migrations/20161029230233_StateMachineInit.php

Why was this a requirement again? Why couldnt we continue to treat also unsigned ones as valid foreign key columns and just use signed on by default then?
I would only expect the generation of new migration files to be "more specific", while keeping support for every file so far.

spryker/cakephp-statemachine@953489c seems to be needed, instead, maybe we could offer some migration to provide this where needed instead of just failing hard?
Not sure what could be done here.

[markstory]

I also ran into my migrations being 'broken' by this change. Tracking down where each foreign key was added to the schema and updating those migrations was pretty tedious.

[othercorey]

Seems that question was asked in the PR but was merged anyway.

#1899

[dereuromark]

Yeah, there was no answer given at the time. I guess we need to revisit this, or provide a mitigation strategy as outlined above.

[othercorey]

Do you want to revert this change for now? It doesn't affect me, but does seem like a change we should make now if we're going to.

[dereuromark]

It would be good to discuss mitigation strategies first, as this mainly affects us in terms of cake migration users right now.
But if there is no other Option we can revert for sure

[MasterOdin]

I don't think this should be reverted. In my opinion, Phinx (anad other DB migration libraries) should ideally encourage the best practice usage of a database, and having an unsigned primary key int is that, and is the recommendation in the official docs about auto-increment columns.

There is some amount of split amongst other migration libraries here on what to do, e.g. rails uses signed while knex.js uses unsigned, so there's prior art to push for either direction and it works.

Why couldnt we continue to treat also unsigned ones as valid foreign key columns and just use signed on by default then?

Both signed and unsigned foreign key columns are still valid, it just has to match what's allowed for the primary key. So we cannot have signed foreign key columns for unsigned primary keys since the allowed value ranges don't align, and the DB is (imo) smartly forbidding you from getting into a state where you can create stuff in your parent table that cannot be referenced in a child table.

I would only expect the generation of new migration files to be "more specific", while keeping support for every file so far.

Rails does this via versioning their base migration classes, where your migration can be ActiveRecord::Migration[5.0] or ActiveRecord::Migration[6.2] or whatever. This would probably require a fair bit of work to support though as we'd need a mechanism to pass the version around, as well as run different things for each version. Not sure if you had a better idea of how to support this in mind?

or provide a mitigation strategy as outlined above.

The migration strategy as outlined in the 0.13 release was to either:

1. add 'signed' => true to all tables that are created using the default id (to restore previous behavior)
2. add 'signed' => false to all columns created as foreign key references where you didn't do (1)

I get that it's a painful thing to do, but it should be a one-off thing, and is probably just as painful as dealing with #1872. I feel like perhaps the biggest thing is having phinx get updated in a non-major version of cakephp/migrations given that since it's a shallow wrapper, it was majorly affected by a number of the BC breaks, not just on this one.

[MasterOdin]

Do you want to revert this change for now?

I think that reverting cakephp/migrations#517 would make sense given the BC break this causes going from cakephp/migrations@3.6 (and lower) to cakephp/migrations@3.7, and would be better documented for a cakephp/migrations@4.0 release.

[dereuromark]

I think that reverting cakephp/migrations#517 would make sense given the BC break

That would not solve the missing migration issue, though - and trigger a chain reacting of other things (also in terms of dependencies towards this).
As people have already long migrations for their apps, so either way there is quite the impact on them.
For now we mitigated the unknown factor with a note in the release notes.

Not a perfect solution either way, I understand.

[ADmad]

https://old.reddit.com/r/shittychangelog/comments/zl5gaz/here_at_reddit_we_believe_everything_is_better_in

If even reddit overflowed 32bit signed int only now I am not sure the grief this change caused was worth it.

[MasterOdin]

Should we also revert #1872 as that mostly certainly caused way more of a headache than this change, and I'll note wasn't called out in https://github.com/cakephp/migrations/releases/tag/3.7.0.

[dereuromark]

Having it NULL by default shouldnt be creating that much of an issue? As they would be fine taking "not null" data?
Or what do you mean?
From the 0.13 perpective those changes seem OKish after all.

Could we maybe provide a "feature flag" for those and just deactivate this from Migrations plugin side unless actively enabled by user?

[MasterOdin]

$this->table('foo')
  ->addColumn('bar', 'int')
  ->update();

Used to generate (typing this freehand):

ALTER TABLE foo ADD COLUMN bar INT NOT NULL

It now generates:

ALTER TABLE foo ADD COLUMN bar INT NULL

While this probably won't "break" anyone's existing migrations in the way the signed change will, it does drastically affect the structure of data consistency.

0.13.0 was a very disruptive breaking change over a number of its PRs, and I think that downstream consumers that were shallowish wrappers bumping it in a minor release is probably not great in terms of semver.

[dereuromark]

Again: The major version is not as important here as feature flags IMO to manage those defaults.
A semantic (0.x) major you can never consume fixes from (which we only deliver into those) is also not helpful.

If users and downstream tools are in power of those feature flags, they wouldnt have to deal with those any time soon, while any consumer that wants could go to the latest and coolest (e.g. for new projects).
And both sides could use the fixes provided into 0.13 without being stuck on unmaintained 0.12

We would then:

• disable feature flags for Migrations (current), restoring BC
• enable (or discuss this at least) feature flags for new Migrations major for Cake5 next year, communicating how to restore BC for existing projects if needed.

[dereuromark]

That said: You could also do the other way around:
Revert both and add feature flags to restore the current master behavior for both topics.
Then only 0.14 could maybe switch feature flag behavior and we have more time to figure things out.
Consumers can read in release notes on how to enable better defaults by enabling the feature flags on their own if e.g. new project or safe to do so.

[ADmad]

Should we also revert #1872 as that mostly certainly caused way more of a headache than this change

Haven't had any issues reported for this change yet. We can reconsider if we do.

Changing the pk/fk to unsigned was fine for phinx 0.13 (which is effectively a major since we are 0.x) but the problem is that it caused issues in a minor release for the migrations plugin where backwards compatibility is expected.

[dereuromark]

Can we agree on the feature flag to be quickly added and then disabled for current migrations release?

[ndm2]

Couldn't this be worked around by reintroducing the previous default in \Migrations\Table::create()?

[dereuromark]

We could try that as quick solution for now, sure.

[MasterOdin]

Haven't had any issues reported for this change yet. We can reconsider if we do.

As I said, the null change is probably not something that'll immediately bite people with an error message (unlike the PK signed change) unless they're inspecting the generated SQL and notice that it's now NULL instead of NOT NULL.

This change, like the signed PK change, should be called out in downstream wrappers, and not doing so is, in my opinion, setting your users up for unexpected issues down the road.

[dereuromark]

PR open
Afterwards we can adjust Migrations plugin and docs for the upgrade path