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

[8.x] Add anonymous migrations #36906

Merged
merged 3 commits into from Apr 13, 2021
Merged

[8.x] Add anonymous migrations #36906

merged 3 commits into from Apr 13, 2021

Conversation

thiagorb
Copy link
Contributor

@thiagorb thiagorb commented Apr 7, 2021

This would solve #5899.

With this approach, migrations won't have class names anymore (they didn't serve any purpose and cause some tricky to solve issues).

After a project was maintained for a long time, the chance that two migrations were created with the same name is very high. For example, consider the following migrations:

  • 2019_01_01_000000_add_content_to_posts_table
  • 2020_01_01_000000_remove_content_from_posts_table
  • 2021_01_01_000000_add_content_to_posts_table

With the current approach, the first and last migration have the same name. After the third migration is created, it is possible to run it just fine, unless you are trying to run all these migrations at the same time (like after creating an empty database, or while running tests). In a project where there were always tests since the beginning this will be barely an inconvenience, but it causes a lot of trouble for a project that has been developed for many years without tests.

With this new approach this won't be a problem, cause the names can be dropped completely. The new Migrator is still able to run named migration classes, so the solution is backward compatible.

I believe it would be possible to backport it to 6.x too, in case it is approved.

@crynobone
Copy link
Member

This PR changes some of the framework migrations such as batches, failed_jobs. Is there anyway preventing the app from generating the migration more than once?

image

Also wouldn't think we should change the default stubs on a patch release, but supporting anonymous migrations itself on patch release should be fine if there no breaking change risk 👍🏼 .

@thiagorb
Copy link
Contributor Author

thiagorb commented Apr 8, 2021

Hey @crynobone ! Yeah, I thought that changing the stubs could be too much, so I added it as a separate commit. We can drop this last commit then.

But your preference is to throw if creating a migration with duplicated name instead of anonymous migrations?

@crynobone
Copy link
Member

crynobone commented Apr 8, 2021

But your preference is to throw if creating a migration with duplicated name instead of anonymous migrations?

I worry that it potentially causes an issue if developers try to run php artisan queue:failed-table more than once since it should generate the same file content with a different timestamp and no longer check for the identical class name as if right now (see above picture).

My guess all framework migration should remain as it is. and maybe ship with separate anonymous migration stubs for 8.x.

php artisan make:migration -a create_posts_table --create

@driesvints
Copy link
Member

I really like this idea. I think @crynobone indeed makes a good point about the duplicate framework shipped migrations.

I believe it would be possible to backport it to 6.x too, in case it is approved.

Laravel 6 is closed for new features.

@taylorotwell
Copy link
Member

taylorotwell commented Apr 8, 2021

It's a cool feature but yeah I would drop the commit of renaming existing migrations - at least until 9.x.

@driesvints driesvints changed the title Add anonymous migrations [8.x] Add anonymous migrations Apr 8, 2021
@thiagorb
Copy link
Contributor Author

thiagorb commented Apr 8, 2021

Ok. I reverted the commit with the stubs update.

@taylorotwell
Copy link
Member

There is a breaking change by changing the public resolve method on migrator to protected.

@thiagorb
Copy link
Contributor Author

thiagorb commented Apr 8, 2021

There is a breaking change by changing the public resolve method on migrator to protected.

I restored the public method, and created a new protected one for the new implementation. The old method is not being used anymore though.

Due to backward compatibility, classic migrations (non-anonymous) should
have their class name displayed while running migrations with --pretend,
instead of the migration name, like anywhere else in the code.
@thiagorb
Copy link
Contributor Author

I just pushed one more change, cause I realized that while running php artisan migrate --pretend, the queries would be printed differently:

2014_10_12_000000_create_people_table: create table "people"...

Instead of:

CreatePeopleTable: create table "people"...

For the anonymous migrations I left it as:

2015_10_04_000000_modify_people_table: alter table "people" add column

It would be easy to transform the name into ModifyPeopleTable (by reusing existing methods), but I think it doesn't make sense to create this class name if the class doesn't exist. Besides, the class name is displayed only while running with --pretend, so I think it would be better to change it in the future to always display the migration name, to be consistent with how it is displayed while migrating or rolling back for real:

Migrating: 2014_10_12_000000_create_people_table
Rolling back: 2014_10_12_000000_create_people_table

@taylorotwell taylorotwell merged commit 64272b4 into laravel:8.x Apr 13, 2021
@driesvints
Copy link
Member

@thiagorb this is a fantastic PR. Thanks for your work 👍

@lloricode
Copy link
Contributor

lloricode commented Apr 15, 2021

this should be mandatory in laravel 9

edit:
so migrations in laravel 9 won't have class names anymore

@lloricode
Copy link
Contributor

this should be mandatory in laravel 9

edit:
so migrations in laravel 9 won't have class names anymore

hope to merge this #37352

@BenjaminStuermer
Copy link

Hey, @thiagorb, I just came across this PR after running into exactly the issue you describe: I developed a project migration by migration, had multiple identical migration names, and never noticed until I tried to create a new instance of the application. You saved me some serious pain with this PR, thank you so much.

@parasw3nuts
Copy link

parasw3nuts commented May 17, 2022

Laravel 8 migration not working on different database different migration folder and namespace
Same class name error getting
Example : migration/first_migration
migration/second_migration

above folders has same classname file with namesapce

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

8 participants