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

[Proposal] Switch the default increments() to a bigint #1384

Closed
victorlap opened this Issue Nov 8, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@victorlap
Copy link

victorlap commented Nov 8, 2018

With regards to https://twitter.com/dhh/status/1060565296048562177

See also rails/rails#26266

Since there already is bigIncrements for all suported database drivers, implementation of this should not see too many hurdles.

@victorlap victorlap changed the title [BC BREAK] Switch the default increments() to a bigint [Proposal] Switch the default increments() to a bigint Nov 8, 2018

@tomschlick

This comment has been minimized.

Copy link

tomschlick commented Nov 8, 2018

This would need to be done in a way that would keep the existing migrations using their current setup. Othewise you wont be able to run migrations for things like CI servers, new environments, etc. Especially if you use foreign keys linked to those columns.

@victorlap

This comment has been minimized.

Copy link
Author

victorlap commented Nov 8, 2018

Yes, this will break previous migrations. However we can tackle this in two different ways:

  1. Let it break, write a section in the upgrade guide, that every increments() should be altered to smallIncrements(). Changes to Schema methods have been done in the past, altough not this rigorous maybe.
  2. Let users revert to the previous setting by implementing a method along the likes of Schema::disableBigIntegerIncrements() that can be put somewhere in a service provider. This allows for new apps to use bigIncrements, while migrations in older apps can still function and would produce the same output as before.
@michaeldyrynda

This comment has been minimized.

Copy link

michaeldyrynda commented Nov 9, 2018

I'd be for this, but the question is whether or not the breaking change goes into 5.8 in Feb, or if we wait for 6.0 (whenever that happens to be).

Do we know of any projects that are using Laravel at scale large enough that this will impact them?

@victorlap's suggestions on moving forward are good, though, and as long as they're covered in the upgrade guide, should be sufficient.

We've done similar before i.e. with the defaultStringLength - so I'd suggest break it and provide the fallback for those that want to preserve functionality, rather than updating things.

@tomschlick

This comment has been minimized.

Copy link

tomschlick commented Nov 9, 2018

Sounds good to me. I’d say it’s worth a PR at this point 👍

@victorlap

This comment has been minimized.

Copy link
Author

victorlap commented Nov 9, 2018

Do we know of any projects that are using Laravel at scale large enough that this will impact them?

In the company I work for, we make sure that all ids are bigints, to reduce the likelyhood of running into problems. We do have some projects where this could have gone wrong.

The default jobs and failed_jobs tables also use the bigIncrements() method.

@crynobone

This comment has been minimized.

Copy link

crynobone commented Nov 9, 2018

Why not just let the default migration stubs to use bigIncrements() so any new install can start use it by default without interfering with old applications.

@victorlap victorlap closed this Nov 9, 2018

@sisve

This comment has been minimized.

Copy link

sisve commented Nov 11, 2018

I've proposed another solution in laravel/framework#26472, which is what @crynobone suggests; to change the default migration stub instead of adding an application-wide flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.