[5.8] Revert #26454 and change default to bigIncrements in the migration stub

[sisve]

The previous PR #26454 broke existing migrations by changing the behavior of the increments method. It was also incomplete; changing the primary key alone would have caused issues at the first sight of a foreign key which was still an "small" integer.

Also, keep in mind that this flag is an application-wide flag. There will be developers that think that their increments become bigIncrements and still uses increments, and there will be installations that sets this flag, ending up with code that does something unexpected (by those expecting the increments to become bigIncrements).

This is comparable to $table->string('email') that once upon a time created a 255 character large string, and then suddenly became 191 characters because some service provider (perhaps even in a third party package), used Builder::defaultStringLength(191);, and we had to go back and change all our migrations to specifically say $table->string('email', 255) to make sure that we got what we expected.

I propose a revert of #26454 and instead change the default migration file generated by php artisan make:migration ... to use bigIncrements instead. This means that all existing migrations are still working as-is, and this new behavior will only affect new migrations. It also means that the method can be changed back to increments by the developer to get the old behavior when needed.

It is my personal belief that the migration system has even stricter requirements on backward compatibility than releases in other parts of the framework. Migrations are usually written, published and then never changed. (Any change needed goes into another migration. Migrations are like ogres, they have layers...).

[sisve]

The failing tests are unrelated to my change, the master branch seems failing at the moment.

[antonkomarev]

Change old behavior is overkill. I agree with @sisve solution. We'd better keep it simple and just generate migrations with bigIncrements by default. It's not that much to write 3 extra letters, but it's much more obvious.

[hastinbe]

I don't understand the rationale for the change to the stub. I have 4 questions:

• Who is saying that 2.1 billion integers is not enough for the average table?
• Where is the evidence which shows that?
• Why is it acceptable to increase the size of indexes, memory and storage requirements without reason?
• Why can't users who actually require bigint auto increment columns not just specify that in their migrations, instead of changing it for all tables that don't need it?

[sisve]

@hastinbe I agree with your all your points. However, keep in mind that the idea of using bigint was already accepted in the previous PR, but in a way I disagree with. Assuming that we still want to default to bigIncrements, then a change to the stub seems to me like the reasonable way to go, and not a application-wide flag that toggles the behavior of increments.

[hastinbe]

I agree that this solution is better than the other, but it doesn't answer the question why any change is necessary at all

[BrandonSurowiec]

It was probably inspired by this: https://m.signalvnoise.com/update-on-basecamp-3-being-stuck-in-read-only-as-of-nov-8-9-22am-cst-c41df1a58352

TLDR: Ruby of Rails updated to use bigInt as the default auto increment column two years ago. Basecamp never implemented the change themselves and had downtime as a result.

[mfn]

Thanks for the PR/mindset.

This is an important issue which usually discovered at later stage cycles when it becomes a burden. There's been enough "evidence" over the internet of companies gotten bitten by this.

I was leading a mid-sized MySQL->PgSQL migration last year and we already put us on the bigIncrements track and we're far away from reaching the limits.

But: being aware of this "future problem" and being not constrained by external factors and storage space not being an issue, we already decided to go with that train.

As such, I applaud this, thanks!

[hastinbe]

@BrandonSurowiec which is exactly why this change makes no sense. The rationale is based on "because rails did it" and that Basecamp ran out of IDs because they didn't update their table schemas as their data grew too large for 32-bit ints.

If we look at the original article about why Rails moved to BIGINT (http://www.mccartie.com/2016/12/05/rails-5.1.html), they also have no rationale other than to do it because there's a chance that you might become Twitter one day. So why penalize everyone who don't have these sorts of problems?

If you're growing to hundreds of millions of rows of data, you should be well aware that these sort of problems will happen and adjust accordingly. The problem was negligence of the person managing their application and database in both cases and not their framework.

This change isn't an improvement, this is a deoptimization to everyone and every table you make that doesn't need BIGINT, which is the majority of cases.

[antonkomarev]

Just one small notice about id BIGINT vs INT questions: we already using unsignedBigInteger in morph columns since 5.6 #23320
And it's indirectly related to laravel/ideas#1172

[mallardduck]

Just putting in my 2cents that this is a much more elegant solution than the current one. The original change has caused more issues than it likely solved. The only main benefit is that it solved a whole load of theoretical issues. That doesn't make up for things breaking more tho.