laravel / passport

Laravel Passport provides OAuth2 server support to Laravel.
https://laravel.com/docs/passport
MIT License
3.25k stars 780 forks source link

Update InstallCommand.php #1741

Closed justasSendrauskas closed 2 months ago

justasSendrauskas commented 2 months ago

The documentations says that passport no longer publishes migrations (it did not publish in version 11.x) here https://github.com/laravel/passport/blob/12.x/UPGRADE.md#migration-changes

However the code in InstallCommand.php clearly has publish migrations, so or documentation is wrong, or code should be removed, i prefer the latter though, as always publishing migrations into a default directory is not good practice.

Please and thank you

driesvints commented 2 months ago

This is incorrect. Those upgrade notes says passport no longer LOADS migrations which is a different thing. You still need to publish your migrations, which is what the install command does. The link you posted is for people upgrading from 11.x to 12.x

driesvints commented 2 months ago

as always publishing migrations into a default directory is not good practice.

This is the way things work now and how all our packages behave.

justasSendrauskas commented 2 months ago

@driesvints thank you for your prompt reply and action.

when you refer to loading do you mean 'Would you like to run all pending database migrations?' in here https://github.com/laravel/passport/blob/12.x/src/Console/InstallCommand.php#L44 ?

However if people have different structure in migrations folder, now you are by default dumping migrations and asking should you 'load' them

driesvints commented 2 months ago

In Passport v11 and before, it "loaded" migrations like this: https://github.com/laravel/passport/blob/09f543ece2bb07da13efccd8de24aa5aa1453669/src/PassportServiceProvider.php#L85

But now we always publish migrations into the app. It's a much more frictionless way to make sure the package doesn't interferes much with the app's lifecycle. Just also makes sense to keep everything migrations related in a single place instead of being hidden away in a package.

justasSendrauskas commented 2 months ago

@driesvints I see, thank you for explanation.

Before when when install did not publish migrations into the app, we used it in docker setup, as it would configure the clients etc, and we would not need migrations as this is required only once. I guess I can pick and choose functions now from install for setup, I do not think there is something like configure, which would run everything except migrations?