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

[5.8] Migration Events #28342

Merged
merged 11 commits into from May 2, 2019

Conversation

Projects
None yet
9 participants
@alexbowers
Copy link
Contributor

commented Apr 25, 2019

This PR adds events to migrations.

Events added:

migrations.beforeAll:up
migrations.afterAll:up
migrations.beforeAll:down
migrations.afterAll:down
migrations.before:up
migrations.after:up
migrations.before:down
migrations.after:down

The benefit of having these methods is it will allow users to clear cache on migrations, perform maintainence if necessary.

The before:up, before:down, after:up and after:down methods all receive the migration that was ran as a parameter, so that you can check which migration you are running to see if you need to perform any specific action.

Fixes laravel/ideas#854

@driesvints
Copy link
Member

left a comment

You're on the right track but I believe we can do better. First of all: there's no need for the static keyword anywhere. Check my comments.

I also believe it might be better if we used dedicated classes instead of the string based ones. There's an Illuminate\Database\Events where you can add these.

Based on your list above these could be:

  • UpMigrationsStarted
  • UpMigrationsEnded
  • DownMigrationsStarted
  • DownMigrationsEnded
  • UpMigrationStarted
  • UpMigrationEnded
  • DownMigrationStarted
  • DownMigrationEnded

And might I suggest adding:

  • MigrationsStarted
  • MigrationsEnded
  • MigrationStarted
  • MigrationEnded

Maybe we only need the last four above? Do we really need to know if a migration is a down migration or not?

Nice job so far 👍

Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@driesvints I have made those changes. Couple of things to note.

For the events, in the old system there was wildcard support, which is why I have put a Contract in there, since that allows the same thing:

Events::listen(\Illuminate\Contracts\Database\MigrationEvent::class, ...);

I have also changed how data is passed around, the events now instead store the migration and the direction, instead of it being a separate parameter.

@driesvints
Copy link
Member

left a comment

Nice job man. Just a few small things and the StyleCI failures.

Show resolved Hide resolved src/Illuminate/Database/Events/MigrationEvent.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
@driesvints
Copy link
Member

left a comment

A few more things. Also: this PR could also use some tests.

Show resolved Hide resolved src/Illuminate/Contracts/Database/MigrationEvent.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
@deleugpn

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I would like this as well. I have some migrations that change information inside the database and I wanted to write tests to make sure the migration was doing the expected process, I ended up doing this:

    private function afterMigration(string $migrationName, callable $callback)
    {
        DB::listen(function (QueryExecuted $query) use ($migrationName, $callback) {
            if (isset($query->bindings[0]) && $query->bindings[0] === $migrationName) {
                $callback();
            }
        });
    }


    private function migrate()
    {
        $this->afterMigration('2018_11_14_154917_some_migration', function () {
            // Add some seed data at this point before changing the database format
        });

        Artisan::call('migrate');
    }

@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@driesvints I have done the above, but I was having some trouble with the tests. Not sure why, but Event:assertDispatched was not working for me.

Any advice? As it stands, it basically is fine if it has assertions.

Not ideal, but it does test it.

@driesvints
Copy link
Member

left a comment

Any advice? As it stands, it basically is fine if it has assertions.

Current assertions are fine 👍

I see you've added a merge commit. Can you try to purge this commit and use rebase instead? Because this adds unnecessary extra commits to the PR.

Show resolved Hide resolved tests/Integration/Database/MigrateWithRealpathTest.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@driesvints I can try to collapse the commits down. Do you want me to just squash them all?

@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@driesvints I haven't been able to get the merge commit fixed. Any chance when this is merged it just gets squashed instead? Or can you help me with fixing the merge commit? Either or is fine by me :D

@driesvints
Copy link
Member

left a comment

Almost there.

Show resolved Hide resolved src/Illuminate/Database/Migrations/Migrator.php Outdated
@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@alexbowers squashing can't be done anymore because you're used merged commits. If you want to fix this you'll need to start a new branch from 5.8 and cherry pick your commits on top. You've used multiple merge commits atm.

@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Ok i'll figure it out this evening. Might have to be a new PR or a force push or something.

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@alexbowers should be able to do it with a force push. Don't need to open up a new pr. Don't forget to backup your branch first to prevent losing stuff.

@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Ok @driesvints I think thats worked.

@driesvints
Copy link
Member

left a comment

Nice job 👍

@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Any more changes, or are you good with this?

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@alexbowers see the approval above :)

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

You could perhaps squash your commits.

@alexbowers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Ah missed that. It should be able to be squashed by the merger, since if I was to squash now that'd be another force push. I'd rather avoid that just incase I bin it :D

@devcircus

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Just a thought, but in Illuminate/Auth/Events the namespace delineates the classes within as events. The classes themselves are "Login", "Logout". etc. So Illuminate/Contracts/Database/Events/Migration makes sense in that context.

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@devcircus right. Let's just leave it at this.

@ttomdewit

This comment has been minimized.

Copy link

commented May 1, 2019

Thanks for everyone's hard work this past week, hopefully it'll be considered and merged. 👍

@taylorotwell taylorotwell merged commit 3dd96ad into laravel:5.8 May 2, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexbowers alexbowers deleted the alexbowers:feature/migration-events branch May 2, 2019

@jasonmccreary

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Were these added to the docs anywhere? Glad to do it. Just didn't want to duplicate work.

@driesvints

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@jasonmccreary I don't think they were.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.