Add an optional shouldRun method to migrations.

[danmatthews]

What's this?

I've added the ability to declare a shouldRun function in your migration files to determine wether or not a migration should run when php artisan migrate or php artisan rollback is called.

<?php

return new class extends Migration
{
   public function shouldRun()
   {
        return Feature::active(Flights::class);
   }

    /**
     * Run the migrations.
     */
    public function up(): void
    {
        Schema::create('flights', function (Blueprint $table) {
            $table->id();
            $table->string('name');
            $table->string('airline');
            $table->timestamps();
        });
    }
 
    /**
     * Reverse the migrations.
     */
    public function down(): void
    {
        Schema::drop('flights');
    }
};

Why?

This adds the ability to selectively run migrations that you may have shipped to various environments based on:

• Feature flags
• Environment checks
• Config values
• Database adapter configured / other config values.]

etc.

This allows you to ship code to environments without running potentially damaging migrations before you're ready.

Backwards compatibility.

The shouldRun function is added as an implemented function on the abstract Migration class and returns true by default, so ALL migration classes should work as usual, with no need to adjust the stub file either.

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[taylorotwell]

One thing I'm curious about is we are not instantiating the migration a bit earlier, and we're not using the requireFiles method first like we do other places. What implications does that have?

[danmatthews]

@taylorotwell ah, i see what you're getting at.

• For non-anonymous class migrations, it shouldn't matter (as long as the file name matches the class name), because resolvePath will require it, using require_once under the hood, anyway, so when requireFiles is called, it shouldn't require it twice.

• For anonymous migrations - not 100% sure on this one, the file might be 'required' twice, as the line $migration = static::$requiredPathCache[$path] ??= $this->files->getRequire($path); uses require, but assigns to a var, so it wouldn't be required into the global scope, but it would happen more than once, i think.

The alternative option is to move this out of a collection filter method in pendingMigrations(), and filter them out just before they're passed to runPending() in run() AFTER requireFiles has been ran?

[ziadoz]

We currently workaround this limitation in our app with custom migration code/commands, so having this built into Laravel would be fantastic. If there's anything I can do to help here let me know.

[inmanturbo]

@taylorotwell ah, i see what you're getting at.

• For non-anonymous class migrations, it shouldn't matter (as long as the file name matches the class name), because resolvePath will require it, using require_once under the hood, anyway, so when requireFiles is called, it shouldn't require it twice.
• For anonymous migrations - not 100% sure on this one, the file might be 'required' twice, as the line $migration = static::$requiredPathCache[$path] ??= $this->files->getRequire($path); uses require, but assigns to a var, so it wouldn't be required into the global scope, but it would happen more than once, i think.

The alternative option is to move this out of a collection filter method in pendingMigrations(), and filter them out just before they're passed to runPending() in run() AFTER requireFiles has been ran?

Another option is to wait till Migrator::runUp. I've used (and encountered) something to this effect in past projects:

class ConditionalMigrator extends Migrator
{
    protected function runUp($file, $batch, $pretend): void
    {
        // First we will resolve a "real" instance of the migration class from this
        // migration file name. Once we have the instances we can run the actual
        // command such as "up" or "down", or we can just simulate the action.
        $migration = $this->resolvePath($file);

        $name = $this->getMigrationName($file);

        if ($pretend) {
            $this->pretendToRun($migration, 'up');

            return;
        }

        // check here if migration should run
        if (! $migration->shouldRun()) {
            $this->write(Info::class, "Skipped migrating {$name}");

            return;
        }

        $this->write(Task::class, $name, fn () => $this->runMigration($migration, 'up'));

        // Once we have run a migrations class, we will log that it was run in this
        // repository so that we don't try to run it next time we do a migration
        // in the application. A migration repository keeps the migrate order.
        $this->repository->log($name, $batch);
    }
}

Incidentally, in another project I'm currently placing feature or environment dependent migrations at a separate path and running them with a custom command (similar need, different type of solution).

[danmatthews]

@inmanturbo nice, thanks!

I think i was avoiding that because it had the possibility to fire migration events before then being skipped, but i'll take another look on my lunch today!

Another thing i'd love some feedback on is having the migration show as "SKIPPED" when running the command in the same way we show RAN and FAILED results.

[donnysim]

For anonymous migrations - not 100% sure on this one, the file might be 'required' twice, as the line $migration = static::$requiredPathCache[$path] ??= $this->files->getRequire($path); uses require, but assigns to a var, so it wouldn't be required into the global scope, but it would happen more than once, i think.

You cannot require_once the same file twice that returns a value (in this case anonymous class) instead of defining a global class etc. as the second time you require_once it, the it will return 1 instead of the value. I don't think this would be a problem as long as the current logic is kept.

Also, I'm for the SKIPPED status to be visible, would be nice if it could show a "string" reason too instead of just a bool value.

[danmatthews]

A string reason would be cool, but not sure it would fit within the two column layout of the current migration status run. Maybe we could a --show-skipped flag that would output a reason at the end, but i think being able to grab the migration name and open the file would be enough to look into why for now.

[donnysim]

Maybe it could just add a separate empty name row:

for safety reasons could be behind a flag --with-skip-reason if anyone parses the output. I'm not sure how the 2-col reacts with long strings that clash, maybe it already has some logic and could be just inlined with the status.

[inmanturbo]

Showing

@inmanturbo nice, thanks!

I think i was avoiding that because it had the possibility to fire migration events before then being skipped, but i'll take another look on my lunch today!

Another thing i'd love some feedback on is having the migration show as "SKIPPED" when running the command in the same way we show RAN and FAILED results.

Showing skipped is a good idea. As to showing a reason for skipping, yet another option still is to use an exception that will be caught by the migrator, similar to how validation works. This would allow you to $fail('Feature not enabled') from within the Migration::up and print a warning or error yet continue running other migrations. Could introduce a --bail flag to the migrate commands for stopping on the first $fail(). This would have a wide range of applications, including safely handling transactions inside migrations, etc.

But I think simply skipping without providing a reason is enough for the scope of this PR.

In any case it's a usefull feature to have in the framework and I appreciate you working on it.

[danmatthews]

Alright:

• Moved the detection until after the migrations have already been loaded to avoid any issues with changing that.
• Added a MigrationResult integer backed enum to store the new possible third state of a migration run/call.
• Tests

Only thing left might be that the migration still shows as "Pending" in migrate:status, i think this is okay personally, but we might wanna add some more reasoning here - i'd be nervous about writing the reason to the migrations table though.

[danmatthews]

@taylorotwell this is ready to merge as is, but i'd love your opinion on:

• Providing a "reason" for skipping a migration in the CLI as some have mentioned: personally I don't think we need it, but if you think it would be handy, might be better to add it now rather than later for backwards compatibility.
• If you have any issue with the migration showing as "Pending" in the migrate:status command, as it technically is - but in order to show something like "will be skipped", there are some odd thing like what "tense" do we present that info in - i.e. has it been ran + skipped already, has it NOT ran yet, but will be skipped, and keeping track of that - as well as loading the migration files themselves again to evaluate the result of the method, might be overkill.

[danmatthews]

Looks like CI run fails are to do with Docker issues, not actual test fails 🤞🏻