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

Publishing migrations - are stubs really supported? #66

Closed
judgej opened this issue Apr 2, 2021 · 6 comments
Closed

Publishing migrations - are stubs really supported? #66

judgej opened this issue Apr 2, 2021 · 6 comments

Comments

@judgej
Copy link

judgej commented Apr 2, 2021

This documentation describes two ways of publishing migrations: as a complete directory of full migration scripts, or as named migration stubs.

The main Laravel documentation, right up to v8, does not describe the stubs method at all, which suggests that method may be discouraged.

Now the problem comes with checking if a stub migration has already been published. I am finding that, with L8 at least, the /database/migrations directory is not autoloaded by the framework. That means the method of determining if migrations are already loaded by looking for the class (e.g. class_exists('CreatePostsTable');) does not work - it always returns false with the result that the same migrations can get published multiple times.

I am using a small workaround in my package service provider to load the migrations of interest if they exist, so the class_exists() check can be used as documented here:

MyPackageServiceProvider:

    use CreatePostsTable;

    public function boot()
    {
        if ($this->app->runningInConsole()) {
            collect([
                'migrations/*_create_posts_table.php',
                'migrations/*_any_other_migrations_of_interest.php',
                'migrations/*.php', // or everything if you like playing dangerously
            ])
            ->flatMap(function ($item) {
                return glob(database_path($item));
            })
            ->each(function ($item) {
                include_once $item;
            });

            if (! class_exists(CreatePostsTable::class)) {
                // ...
            }

However, that feels like a dirty workaround that should not be necessary.

So what's the best approach? This workaround or similar, documented in this project? A fix in the core Laravel framework to load all current migration classes when running a package migration? Or deprecating stubs for package migrations completely (they are PHP, so are dynamic and can set object names from config anyway, without a stub)?

Using stubs is handy for having a timestamp that shows when it was published, but is that really a benefit? I switched a package to using migration stubs, thinking it was the right way and where the framework was headed. But was that a mistake?

@judgej
Copy link
Author

judgej commented Apr 2, 2021

This package uses wildcards to see if the migration files have been created, without actually loading them:

https://github.com/spatie/laravel-permission/blob/master/src/PermissionServiceProvider.php#L172

However, their stubs create table namess from the config data, so they do not appear to be making use of the stub concept anyway. So again - feels like there are dirty hacks to get this working, but should the migration stubs even be a thing needing workarounds?

@Jhnbrn90
Copy link
Collaborator

Jhnbrn90 commented Apr 10, 2021

Dear @judgej

Thanks for raising this question 👍 You are right, when you publish the migrations it will always publish a new migration for the reasons you mentioned. I don't think Laravel would like to include an autoload of the database/migrations directory by default. This happens for the example package but I see that also is the case for the Spatie laravel-medialibrary for example and also your mentioned examples.

I'll look into possible solutions to get the stub method working and not republishing if a published migration already exists.

The reason I don't want to just focus on the loadMigrationsFrom() method is that you can't easily modify a package's migrations while you often might want your end users to be able to do so.

I'd like to come back to your question somewhere next week.

@Jhnbrn90
Copy link
Collaborator

Jhnbrn90 commented Apr 18, 2021

I just saw this PR to the Laravel framework in which migration classes no longer have a name but anonymously extend the base migration class.

Therefore, the check if the classname already exists will no longer work anyway.

I'll keep an eye on the development of this feature and will update the documentation accordingly.

@judgej
Copy link
Author

judgej commented Apr 18, 2021

That's an interesting read. Looks like duplicate migration publishing has been an issue for some time.

I have, for my package, abandoned using stubs for the migrations. Even with workarounds, there were still many cases where it gets more complex. For example, I have one project where migrations have been grouped into sub-directories for a multi-tenanted application. The tenant package may know where it has put the published migrations for tenants, but no other package is expected to know where they are.

@lordisp
Copy link

lordisp commented Apr 18, 2022

I just run into the same issue trying to setup the testing environment and saw that laravel changes the migrations to anonymous classes which is not considered yet in the doc.

@judgej did you find a way to run the migrations?

@judgej
Copy link
Author

judgej commented Apr 19, 2022

No @lordisp , I have stayed away from stubs for now. I haven't explored using them since anonymous migrations were introduced, and it's probably worthwhile doing that. There won't be the duplicate migration class names now, but duplcate migrations could still be created. I think migrations need to protect themselves from being run multiple times, if they are created from stubs (e.g. check if the table/columns etc exist before running) so that they don't fail when run. What is improved is that the whole migration process won't fail withe the duplicate class errors.

@Jhnbrn90 Jhnbrn90 closed this as completed Sep 3, 2022
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

No branches or pull requests

3 participants