Default to signed for all int columns, opt-in for unsigned

[dereuromark]

Addresses Issue: cakephp/phinx#2267

Auto-generated primary keys default to unsigned, but manually added integer columns default to signed. This creates FK mismatches and requires explicit 'signed' => false declarations.

Reverts the primary key part of https://github.com/cakephp/phinx/pull/2159/files which is actually harmful as default.
This needs to be set together with all foreign keys or it will create invalid definitions.

So we can either

• default all ints to unsigned
• revert the defaulting and require unsigned to be explicitly set (this PR)
• try to limit this to foreign keys only (which then would fit again to the primary keys)

This PR aims to restore sane defaults, and people can opt-in for "optimization".

[markstory]

Changing this default will break existing applications. I'm also not convinced it is a sensible default.

[dereuromark]

Before we go into details we should first assess, as outlined in the PR descr above, which strategy is best.
Currently its a bit messy, and we should move in one direction.

If unsigned by default is not sensible, maybe the default in general should be signed, including the primary keys.

[markstory]

If unsigned by default is not sensible, maybe the default in general should be signed, including the primary keys.

My concern is around causing breaking changes in userland migrations. We did that once recently, and it was an unpleasant upgrade. This seems like a similar change.

Before we go into details we should first assess, as outlined in the PR descr above, which strategy is best.

I can see the argument for making primary and foreign integer keys unsigned, as sequences start at 1 and never go negative. I think we need to find a way to make this an opt in default change though.

[dereuromark]

Yes, opt in similar to the other is cool.
But how can we detect foreign keys here for sure and not have all ints become unsigned?

[dereuromark]

So what about

'Migrations' => [
	'unsigned_primary_keys' => true,
	'unsigned_ints' => true,

Both are false by default, to make sure the primary and foreign key maps. Those are sane defaults, and people can chose to go stricter if needed.

[markstory]

Both are false by default, to make sure the primary and foreign key maps. Those are sane defaults, and people can chose to go stricter if needed.

I think this is a good solution. Folks can opt-in to the defaults as desired. In the future we can attempt to deprecate the less popular solution.

[dereuromark]

I updated the PR accordingly.
We will have to make sure we clarify this in the migration guide.
But this is trivial to do, and for users to adjust.

[dereuromark]

We should deprecate this on the Cake ORM maybe?
We dont need isUnsigned() and isSigned() at the same time, do we?

[dereuromark]

I adjusted it as per my comment.

A few things are confusing:
We are using "signed" inside Cake ORM, but "unsigned" in other places?
Are we using isSigned()/getSigned() or isUnsigned()/getUnsigned()? We should probably deprecate one.

If we move forward with signed by default, and unsigned as opt-in, it could make sense to use *Unsigned() here as it would be default false, and then opt-in true.

So a few discussions we still have to make until we can move this PR out of draft.

[dereuromark]

Ping @markstory @LordSimal @ADmad

[LordSimal]

Well it may be inconsistent, but by definition an integer can still contain negative values. Thats why a non PK column will be signed to also allow negative values.

A primary key on the other hand is by design auto incremented and will always start at 1 and therefore will never reach negative values. Making that unsigned is obvious.

We already have ->addForeignKey() as a method to properly add a foreign key column to a table and as far as I know it also gets generated properly by our migration_snapshot bake tool.

So instead of trying to force sync up the integer column type and primary key column type I'd rather try to detect, if the given integer column is actually a foreign key column and therefore recommend using a different migration method ( to add it

[dereuromark]

But how do you properly detect this? foreign keys can not always be _id.
Any magic here will fail, thus the current opt-in to go stricter.
And you you always manually specify it on the tables and fields - but it is not a very user friendly default to require this from people.

Please all, also address the open questions here ("confusions"), that are also important as to the "clean API".

[dereuromark]

Guide added

[dereuromark]

Are we OK to move forward?