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".

[dereuromark]

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

[markstory]

We should deprecate this on the Cake ORM maybe?

The ORM API is part of the additions in 5.3. If we deprecate that it requires changes to cakephp/database and the orm. I thought we could align on what we already have in the core.

[dereuromark]

What do you propose we do?
Since it we shouldnt have both positive and negative form at the same time here in the new major.

Or in other words: Wouldn't it be cleaner to only have one side, and not the other on top?

[markstory]

Or in other words: Wouldn't it be cleaner to only have one side, and not the other on top?

Yes of course, one way would be preferable. However migrations and cake started in opposite directions, and in order for us to provide backwards compatibility we have to support both for a while.

[dereuromark]

Thus my proposal to deprecate but keep around.
otherwise feel free to modify as needed so we can merge and move forward

[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?

[markstory]

Why do these tests need to run in a separate process? They look safe to run in the current process.

[dereuromark]

The #[RunInSeparateProcess] is used here because these tests modify Configure settings, and without process isolation, those settings would persist and affect other tests.

However, there's a simpler solution: use setUp()/tearDown() to save and restore the Configure values, which avoids the overhead of spawning separate processes.

[markstory]

But the default teardown in TestCase already handles resetting the state of configure.

https://github.com/cakephp/cakephp/blob/00ca91c3e6d3e5054a3780392dcb8b0202e34945/src/TestSuite/TestCase.php#L240-L281