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

[5.8] Default to big increments #26454

Merged
merged 3 commits into from Nov 9, 2018

Conversation

victorlap
Copy link
Contributor

@victorlap victorlap commented Nov 9, 2018

For the discussion see laravel/ideas#1384

Basically this makes people less likely to integer overflows when using the default increments() method on the schema builder.

I've introduced a new method Schema\Builder::useIntegerIncrements() to revert to old behaviour for people upgradinig their app and who don't want to edit existing migration files.

This also adds a new method to the Blueprint class, which adds an integerIncrements that resembles the old behaviour of increments

Edit:
I am not too happy with the name integerIncrements as well as useIntegerIncrements. If you have any ideas about a good name for those methods, please let me know!

@taylorotwell taylorotwell merged commit 22e1f7c into laravel:master Nov 9, 2018
@victorlap victorlap deleted the default-big-increments branch November 9, 2018 14:09
@sisve
Copy link
Contributor

sisve commented Nov 11, 2018

I don't understand this change. I have some migrations where I vary between increments and bigIncrements depending on need. Why should the new default of increments be to create bigIncrements? Why have we broken several years of my migrations so I need to go back and rewrite them to keep the old behavior?

Why didn't we just change the migration stub from using increments to using bigIncrements? That would have change the behavior of new migrations, but keep consistency with every existing migration...

taylorotwell pushed a commit that referenced this pull request Nov 11, 2018
…ion stub (#26472)

* Revert "Add tests for switching the default increments() method"

This reverts commit 22e1f7c.

* Revert "Make tests use bigincrements as default"

This reverts commit fd6002f.

* Revert "Switch increments to use unsignedBigInteger"

This reverts commit a406533.

* Use bigIncrements
@grey-dev-0
Copy link

grey-dev-0 commented Mar 1, 2019

Well, why normal sized integer type is still around in the standard RDBMS's we have today? because, a database might never scale too big to hit an integer overflow, so setting the default primary key type to Big Integer is NOT really helping to avoid an overflow, on the contrary, I see that it'll reserve bigger storage space than required for most case scenarios, where a standard integer field would suffice.

Otherwise the big integer should be the default type in the RDBMS level rather than in the application. Besides this change has broken all migration files used with earlier Laravel versions that need to be either rewritten, or go with the provided work around, which are not specified in the Laravel upgrade guide in the documentation as of now.

@driesvints
Copy link
Member

driesvints commented Mar 4, 2019

@grey-dev-0 this PR was reverted here: #26472

The only thing that changed now is the stub which you have full control over: f34ebdc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants