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

[10.x] Remove autoload dumping from make:migration #46215

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Feb 21, 2023

This PR removes the composer dump call when making a migration files. Dumping the autoloader adds a very noticeable (an frustrating) delay and adds no benefit as generated migrations are now all "require" based.

The make:migration command is no longer capable of generating class based migrations (it doesn't replace {{ class }} anymore, so we don't even need to do any checks.

I've left the composer dependency on the classes even though they are no longer used.

Delay example on a fresh project

Screen.Recording.2023-02-21.at.7.00.22.pm.mov

After this change

Screen.Recording.2023-02-22.at.10.46.29.am.mov

@timacdonald timacdonald marked this pull request as ready for review February 22, 2023 02:27
@ankurk91
Copy link
Contributor

ankurk91 commented Feb 22, 2023

You could remove the Composer $composer from constructor as well.
Since it is not being used.

@taylorotwell
Copy link
Member

@timacdonald the make:migration could still be capable of generating migrations with a class if they published the stubs before anonymous classes were introduced as the default. We will have to check the string content of the migration to know.

@taylorotwell taylorotwell marked this pull request as draft February 22, 2023 12:17
@timacdonald
Copy link
Member Author

timacdonald commented Feb 22, 2023

@taylorotwell I thought the same thing when I set out on this journey, however class based migrations are no longer supported - unless I am missing something.

Using the following script will:

  1. Create a fresh Laravel install
  2. Publish all stubs
  3. Download the migration stubs as they were before moving to anonymous classes
composer create-project laravel/laravel stub-testing \
  && cd stub-testing \
  && php artisan stub:publish \
  && curl --output stubs/migration.stub https://raw.githubusercontent.com/laravel/framework/d923676eeed5f08464ce7306d6fcb29e7ad4f13b/src/Illuminate/Database/Migrations/stubs/migration.stub \
  && curl --output stubs/migration.create.stub https://raw.githubusercontent.com/laravel/framework/d923676eeed5f08464ce7306d6fcb29e7ad4f13b/src/Illuminate/Database/Migrations/stubs/migration.create.stub \
  && curl --output stubs/migration.update.stub https://raw.githubusercontent.com/laravel/framework/d923676eeed5f08464ce7306d6fcb29e7ad4f13b/src/Illuminate/Database/Migrations/stubs/migration.update.stub

Running any of the following you should see that {{ class }} is no longer replaced in the migration file.

php artisan make:migration bar
php artisan make:migration create_foo_table
php artisan make:migration add_baz_to_foo_table

It seems this functionality was removed in 9.x. Here is the same script but not to create a 9.0.0 project.

composer create-project laravel/laravel:9.0.0 stub-testing \
  && cd stub-testing \
  && php artisan stub:publish \
  && curl --output stubs/migration.stub https://raw.githubusercontent.com/laravel/framework/d923676eeed5f08464ce7306d6fcb29e7ad4f13b/src/Illuminate/Database/Migrations/stubs/migration.stub \
  && curl --output stubs/migration.create.stub https://raw.githubusercontent.com/laravel/framework/d923676eeed5f08464ce7306d6fcb29e7ad4f13b/src/Illuminate/Database/Migrations/stubs/migration.create.stub \
  && curl --output stubs/migration.update.stub https://raw.githubusercontent.com/laravel/framework/d923676eeed5f08464ce7306d6fcb29e7ad4f13b/src/Illuminate/Database/Migrations/stubs/migration.update.stub

Same deal.

@timacdonald
Copy link
Member Author

@ankurk91 I didn't remove the dependency as I thought a user-land class may be extending these and potentially interacting with the $composer property - so removing it may be a breaking change.

I might deprecate it, though 💡

@timacdonald timacdonald marked this pull request as ready for review February 22, 2023 22:08
@taylorotwell taylorotwell merged commit 2f8a22c into laravel:10.x Feb 23, 2023
@timacdonald timacdonald deleted the migration branch April 13, 2023 00:47
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

3 participants