[8.x] Allow anonymous and class based migration coexisting

[netpok]

#36906 introduces anonymous migrations but it fails when there is a class based migration too with the same name.

This PR fixes this by validating the class based migration's path against the wanted migration. Test are also updated to fail if problem is not fixed.

[taylorotwell]

Can you explain more what you mean?

[netpok]

If there is a named migration, lets say CreateUsersTable in file 2010_01_02_create_users_table.php
And an other anonymous one: 2020_02_02_create_users_table.php
And both migration should run then both is resolved to the named one.

The reason for this that only the class existence was checked but not that the class is the same as the wanted one.

With this change only the first one is resolved to the named migration and the second one will use the anonymous one.

[driesvints]

This doesn't seems right to me. I'd most definitely expect the output from the current tests, not the one that was modified by this PR.

[netpok]

@driesvints The actual error is injected in the fixture, the modified test is just to handle the modified fixture correctly.

Without the fix the testMigrate and testRollback test will fail

The problem is that the migrator will try to execute the first migration twice and that causes an SQL error.

The actual failure:

[driesvints]

Probably best to let @thiagorb review this one

[taylorotwell]

So, @thiagorb's PR did not solve the entire problem it was intended to solve in the first place?

[thiagorb]

In this case you can just modify both migrations to be anonymous.

[netpok]

It works perfectly when there is two or more anonymous class based migration with same name. But when there is one named class based migration and one or more anonymous class the named one will be executed for all.

[netpok]

@thiagorb But is there any reason to not support backward compatibility? It is only a comparison.

The other main reason in my opinion to support this is that this only shows up when a full migration happens (because migrations are not autoloaded), so if someone does not have tests that are running migrations this can cause nasty surprises.

[thiagorb]

The thing is, even if we add this extra condition, it only solves the problem for one named class. If you have 3 migrations, one being anonymous and the other 2 named there will still be an error. The benefit of anonymous migrations is that they make it possible to fix this situation easily, by just converting a named migration into anonymous.

[netpok]

A somewhat realistic scenario currently it is possible to have a migration:

• 2020_01_01_increase_url_size.php

class IncreaseUrlSize extends ...

$table->string('url', 1024)->change();

• 2021_01_01_increase_url_size.php

return new class extends ...

$table->string('url', 2048)->change();

This wont even generate a failure but on an existing db the url field will be 2048 byte and on a fresh install it will be 1024 byte. Try to debug this months later. It is almost free to prevent this in the first place.

---

This PR is to support existing codebases, in my opinion stubs should be updated to the new format rather sooner then later. Most people won't update existing migrations just because it can be updated.

[taylorotwell]

@netpok what about @thiagorb's comment that this doesn't help if you have 3 conflicting migrations?

[netpok]

Basically this PR adds support for having one named "legacy" migration and one or more new anonymous migrations. Having multiple named ones was big no-no, so the only target of this PR is to NOT force peoples to update like 120 existing migrations just to use this new feature.

---

If we update the stubs the new format the chance of someone creating a new named class is almost zero

But having existing classes which he wont update just because he can is pretty high as most people wont even notice the change. They will just fill out the up and down method like always.

And having silent errors like the example above is one of the worst since probably most people won't test the length of a field.

[thiagorb]

The changes to the tests make sense, cause one of the test migrations was converted from anonymous to named, so the output of php migrate --pretend had to be changed to use the class name instead of the migration name.

[netpok]

If required I can add a new test migration instead of changing the existing one if that's a problem that way there will be multiple anonymous ones too.

[Tofandel]

Why was this merged and why in 8.x ? This was a breaking change

The following doesn't work anymore

2020_some_migration_file.php

<?php

require_once 'some_class_with_migration.php';

When running the migration this results in

Error 

  Call to a member function getConnection() on int

I really don't understand the purpose of this PR, it was working fine as it was no?

[driesvints]

@Tofandel why are you using migrations like that? Migrations aren't loaded like that in the framework.

[netpok]

I don't really think this is the way migrations should be used so I wouldn't consider this a breaking change, but could you post some example code for better understanding?

[Tofandel]

Because I have tenants that sometimes copy some root migrations, so instead of copying the content of the file and getting a duplicate class error I just require the original migration

[Tofandel]

Consider this structure

The migrations of the admin site
database/migrations/2014_create_reset_passwords_table.php (A laravel migration)
The migrations of subdatabases
database/tenants/migrations/2014_create_reset_passwords_table.php (The tenant migration that requires the original one)

[driesvints]

I'm sorry but that's a use case we really can't account for. We don't support migrations in sub directories.

[Tofandel]

Except it was working before and that's the only way I can get it to work without copying the content of the file, renaming the migration and renaming the class as well, which leads to a lot of code duplication and a lot of work (I have 30 migrations like this that I would need to manually rename because of this 1 line of code)

[driesvints]

We can't account for every single way people use code in ways it wasn't intended to be used, sorry...

[Tofandel]

@netpok Anything that breaks existing code is a breaking change, this definitely applies here even if you don't consider it one

This should go to 9.x

[netpok]

This never came up as this was never an intended way, but I'm opening a PR right now which fixes this.

[Tofandel]

Thank you, and sorry for the trouble