Allow to pass table option signed for primary key

[DimazzzZ]

Just new pull request for #658 made by @Serhioromano. Very useful feature.

Are additional tests required?

[DimazzzZ]

Please check my fixes in tests, are they correct? The main idea is that table column that is used in FK must be unsigned as referenced table's pk is.

[rquadling]

Will these changes still allow me to create a signed primary key? I think the tests are now only looking for unsigned primary keys.

[DimazzzZ]

@rquadling yes they allow. For example:

// Default (unsigned)
$this->table('test')->addColumn('name', 'string')->create();

// Custom with signed PK
$this->table('test', ['signed' => true])->addColumn('name', 'string')->create();

I fixed existing tests because of changing default behavior, so I think we need additional tests with signed pk.

[rquadling]

@DimazzzZ The extra tests to make sure signed PK is still feasible. I'd be wary about changing the default though. Consider all those migrations out there already. Rolling them back and then forward again could lead to issues. Ones that come under the heading of "It used to work and we've not changed anything!".

[DimazzzZ]

I understand your concerns about these changes but how many and how often developers make signed PK? Or even other ids? IMHO most of them using hacks like @twoixter's from #250 (thanks him btw).

My position is "table auto increment pk must be unsigned" since AUTO_INCREMENT starts at zero by default and increments in the positive direction.

I'll add more tests as soon as posible.

[twoixter]

Hi @DimazzzZ , couldn't agree more! +1, or better +1,000 to this PR. ;-)

[shadowhand]

This represents a backwards compatibility break as implemented. The way to do it without breaking BC would be to keep the current behavior (signed by default) and respect the signed option to create unsigned PKs.

[DimazzzZ]

The way to do it without breaking BC would be to keep the current behavior (signed by default) and respect the signed option to create unsigned PKs.

I thought about it and that was a second option... On the one hand it breaks a backwards compatibility but on the other hand is signed pk is right behavior or we just want to save backwards compatibility forever? We can inform users via changelog about new pk behavior as it was done with PHP 5.3 support, by the way nobody forbids to download an older version of Phinx.

Also we may add new property to phinx.yml like signed_pk: false or something.

Let's discuss.

[rquadling]

Minor patch upgrades must not break BC. This is important as people rely on the semantic version numbers.

http://semver.org/

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.
Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

So, at best, introduce this as a patch, but allow the user to configure their setup to use unsigned PK, but the default must be signed.

[DimazzzZ]

@rquadling okay this is a good reason. I'll set it as signed for now.

May we set this behavior as default in the major 1.0 version?

[DimazzzZ]

Added fix as a minor patch with signed default behavior and new tests. Check this out please.

Who can restart Scrutinizer task or just merge the PK?

[rquadling]

We have to wait for merges. Takes a while sometimes.

[shadowhand]

Looks good to me. @robmorgan do you have any objections to this?

[shadowhand]

Can you add this option to the docs?

[DimazzzZ]

I was in hospital, sorry. The docs has been updated.

[rquadling]

Hope all is OK.

[DimazzzZ]

@rquadling all is ok, thanks!

Any updates on this PR?

[rquadling]

Still waiting for Rob.

[DimazzzZ]

Version 0.6.x released but PR still opened... Should I re-create PR for 0.6.x branch? (

[rquadling]

Hi. Please do.

[magnus-eriksson]

Is there any ETA on this patch?

[Serhioromano]

Or my goodness! For one line change more than 100 lines of testing. That is what I call testing obsession. Technologies are good, but sometimes they just overkill.

@DimazzzZ thank you for this PR. Hope it gets accepted.

[robmorgan]

I can accept this if the default is unsigned however I think the docs need to be updated to say its MySQL specific.

[DimazzzZ]

@robmorgan so I update for unsigned default behavior? As @rquadling says it may cause backward compatibility problems for < v.0.6.*.

[dmarklund]

Any updates to this PR?

[jakejohns]

any word here?

[magnus-eriksson]

Version 0.8 is now out and this is still open? Any ETA?

[robmorgan]

I tweaked the docs a little bit. I was on the fence about renaming the signed table option to unsigned_primary_key, but I've left it alone for now. I'll ship it later today.