Framework: [5.8][6.0] Migrations Out of Order

[brunogaspar]

• Laravel Version: 5.8. & 6.0. (Yes, tested this on both versions to be sure)
• PHP Version: 7.2.22
• Database Driver & Version: not related

Description:

For some reason, with the paths & files i'll show below, the migrations are returned out of order when the same migrations are in the vendor/ directory (from a package) and also when they are on database/migrations/ (if those same migrations are published using the vendor:publish command).

After some code sniffing, the culprit so far is the sortBy here.

Steps To Reproduce:

The code below is to pretty much simulate what the getMigrationFiles() method does without the last part which is not necessary here, since the response in the end will be wrong too.

So this can be placed anywhere on the migrator class.

$files = [
    // Path from a package
    "/vendor/billing/resources/migrations/2019_08_08_100000_billing_rename_table_one.php",
    "/vendor/billing/resources/migrations/2019_08_08_200000_billing_rename_table_two.php",
    "/vendor/billing/resources/migrations/2019_08_08_300000_billing_rename_table_three.php",
    "/vendor/billing/resources/migrations/2019_08_08_400000_billing_rename_table_four.php",
    "/vendor/billing/resources/migrations/2019_08_08_500000_billing_rename_table_five.php",
    "/vendor/billing/resources/migrations/2019_08_08_600000_billing_rename_table_six.php",

    "/vendor/billing/resources/migrations/2019_08_09_100000_billing_create_table_one.php",
    "/vendor/billing/resources/migrations/2019_08_09_200000_billing_create_table_two.php",
    "/vendor/billing/resources/migrations/2019_08_09_300000_billing_create_table_three.php",
    "/vendor/billing/resources/migrations/2019_08_09_400000_billing_create_table_four.php",
    "/vendor/billing/resources/migrations/2019_08_09_500000_billing_create_table_five.php",
    "/vendor/billing/resources/migrations/2019_08_09_600000_billing_create_table_six.php",
    "/vendor/billing/resources/migrations/2019_08_09_700000_billing_create_table_seven.php",
    "/vendor/billing/resources/migrations/2019_08_09_800000_billing_create_table_eight.php",
    "/vendor/billing/resources/migrations/2019_08_09_900000_billing_create_table_nine.php",

    // Path from the app
    "/database/migrations/2019_08_08_100000_billing_rename_table_one.php",
    "/database/migrations/2019_08_08_200000_billing_rename_table_two.php",
    "/database/migrations/2019_08_08_300000_billing_rename_table_three.php",
    "/database/migrations/2019_08_08_400000_billing_rename_table_four.php",
    "/database/migrations/2019_08_08_500000_billing_rename_table_five.php",
    "/database/migrations/2019_08_08_600000_billing_rename_table_six.php",

    "/database/migrations/2019_08_09_100000_billing_create_table_one.php",
    "/database/migrations/2019_08_09_200000_billing_create_table_two.php",
    "/database/migrations/2019_08_09_300000_billing_create_table_three.php",
    "/database/migrations/2019_08_09_400000_billing_create_table_four.php",
    "/database/migrations/2019_08_09_500000_billing_create_table_five.php",
    "/database/migrations/2019_08_09_600000_billing_create_table_six.php",
    "/database/migrations/2019_08_09_700000_billing_create_table_seven.php",
    "/database/migrations/2019_08_09_800000_billing_create_table_eight.php",
    "/database/migrations/2019_08_09_900000_billing_create_table_nine.php",
];

$files = collect($files)->filter()->sortBy(function ($file) {
    return $this->getMigrationName($file);
})->dd();

When this is ran, it will spit something like this:

Illuminate\Support\Collection^ {#683
  #items: array:30 [
    0 => "/vendor/billing/resources/migrations/2019_08_08_100000_billing_rename_table_one.php"
    15 => "/database/migrations/2019_08_08_100000_billing_rename_table_one.php"

    16 => "/database/migrations/2019_08_08_200000_billing_rename_table_two.php"
    1 => "/vendor/billing/resources/migrations/2019_08_08_200000_billing_rename_table_two.php"

    17 => "/database/migrations/2019_08_08_300000_billing_rename_table_three.php"
    2 => "/vendor/billing/resources/migrations/2019_08_08_300000_billing_rename_table_three.php"

    3 => "/vendor/billing/resources/migrations/2019_08_08_400000_billing_rename_table_four.php"
    18 => "/database/migrations/2019_08_08_400000_billing_rename_table_four.php"

    19 => "/database/migrations/2019_08_08_500000_billing_rename_table_five.php"
    4 => "/vendor/billing/resources/migrations/2019_08_08_500000_billing_rename_table_five.php"

    5 => "/vendor/billing/resources/migrations/2019_08_08_600000_billing_rename_table_six.php"
    20 => "/database/migrations/2019_08_08_600000_billing_rename_table_six.php"

    21 => "/database/migrations/2019_08_09_100000_billing_create_table_one.php"
    6 => "/vendor/billing/resources/migrations/2019_08_09_100000_billing_create_table_one.php"

    22 => "/database/migrations/2019_08_09_200000_billing_create_table_two.php"
    7 => "/vendor/billing/resources/migrations/2019_08_09_200000_billing_create_table_two.php"

    8 => "/vendor/billing/resources/migrations/2019_08_09_300000_billing_create_table_three.php"
    23 => "/database/migrations/2019_08_09_300000_billing_create_table_three.php"

    9 => "/vendor/billing/resources/migrations/2019_08_09_400000_billing_create_table_four.php"
    24 => "/database/migrations/2019_08_09_400000_billing_create_table_four.php"

    10 => "/vendor/billing/resources/migrations/2019_08_09_500000_billing_create_table_five.php"
    25 => "/database/migrations/2019_08_09_500000_billing_create_table_five.php"

    11 => "/vendor/billing/resources/migrations/2019_08_09_600000_billing_create_table_six.php"
    26 => "/database/migrations/2019_08_09_600000_billing_create_table_six.php"

    12 => "/vendor/billing/resources/migrations/2019_08_09_700000_billing_create_table_seven.php"
    27 => "/database/migrations/2019_08_09_700000_billing_create_table_seven.php"

    13 => "/vendor/billing/resources/migrations/2019_08_09_800000_billing_create_table_eight.php"
    28 => "/database/migrations/2019_08_09_800000_billing_create_table_eight.php"

    14 => "/vendor/billing/resources/migrations/2019_08_09_900000_billing_create_table_nine.php"
    29 => "/database/migrations/2019_08_09_900000_billing_create_table_nine.php"

  ]
}

Note: I've added a few line breaks between the files combination so it's easier to follow.

As you can see, some of the migrations are out of order starting from the index 16 and 1 as the vendor one is coming after the one from the app, which is wrong.

I would fire a pull request to remove the sortBy as it seems to be superfluous there but maybe there was a particular edge case for it 🤷‍♂

Any pointers?

Thanks!

[Funfare]

Migrations must be sorted, so that the first created ones are executed first.

So the problem with your migrations are, that they have the same timestamps, so 2019_08_08_100000 will be executed first, no matter if it comes from vendor or database.

Are you registering and publishing the migrations or why do you have both vendor and app migrations with the same name?

[brunogaspar]

Migrations must be sorted, so that the first created ones are executed first.

Yes, i'm aware of that of course, but as you can see above, they are returned out of order, the vendor ones should always come first, regardless if the name is the same or not, which does not happen for all of them.

So the problem with your migrations are, that they have the same timestamps, so 2019_08_08_100000 will be executed first, no matter if it comes from vendor or database.

No, not really, that's not a problem with my migrations and i guess you missed the point there, which is not the problem with file names or timestamps being the same or even being on vendor and published on the app.

Laravel allows publishing migrations of packages, even they have the exact same file name and then the paths (from packages or any other custom paths you might want to have) are mixed together with the app database/migrations one here.

As i mentioned on the OP, the problem is that they are being sorted incorrectly. The glob method already returns them pretty much sorted, so the sortBy is mixing them up again and returning them in a very incorrect order for some of them.

Are you registering and publishing the migrations or why do you have both vendor and app migrations with the same name?

That's mentioned on the OP, but i'm publishing migrations from a package. Reason for that is that i need to augment a few migrations, for several reasons that are unrelated to this.

So yes, this is clearly a bug, because if you pay attention to what's returned, you see that the majority of the migrations are sorted correctly, but a few of them have the incorrect order, which is the problem here.

[driesvints]

Seems like this hasn't been changed within three years so I'd thread carefully with any possible changes. Is it really a problem that the indexes are wrong? Seems like the migrations are still being returned in the proper order?

[brunogaspar]

Seems like this hasn't been changed within three years so I'd thread carefully with any possible changes.

Yup, it hasn't changed in a while, but i think i got hit by this before.

Is it really a problem that the indexes are wrong?

I thought about it, reseting the indexes does not seem to fix it, at least what i tried yesterday did not yield different results.

Seems like the migrations are still being returned in the proper order?

Why do you think that? They are clearly not being returned in the proper order.

Check the following ones (which are above too)

0 => "/vendor/billing/resources/migrations/2019_08_08_100000_billing_rename_table_one.php"
15 => "/database/migrations/2019_08_08_100000_billing_rename_table_one.php"

16 => "/database/migrations/2019_08_08_200000_billing_rename_table_two.php"
1 => "/vendor/billing/resources/migrations/2019_08_08_200000_billing_rename_table_two.php"

17 => "/database/migrations/2019_08_08_300000_billing_rename_table_three.php"
2 => "/vendor/billing/resources/migrations/2019_08_08_300000_billing_rename_table_three.php"

Clearly, they are out of order.

• The first one is correct
• The second one is incorrect
• The third one is also incorrect

Reason i say they are incorrect is because the vendor ones comes after the published migrations, which is not the expected behaviour.

Then here it's going to map the migrations incorrectly, running the one from vendor/ instead of the published one.

Does it help to properly understand the problem now?

I can look into a fix if you prefer.

[driesvints]

Does it help to properly understand the problem now?

It does! Thanks for clarifying.

I agree with you that we should look into this further. Feel free to send in a PR but definitely include some tests for it.

Thanks!

[MattStrauss]

I got bit by this once a while ago. If I remember correctly, the issue is this:

sortBy(function ($file) {
            return $this->getMigrationName($file);

So, the sortBy functions seems necessary to put the migration files in the correct order under typical use cases. But the way that the items are sorted with the getMigrationName method, below, only the basename is taken into account.

    public function getMigrationName($path)
    {
        return str_replace('.php', '', basename($path));
    }

So it seems that this method would strip away the /vendor/billing/resources/migrations/ from the vendor migration file names and the /database/migrations/ from the "normal" migration file names during the sorting process, which effectively leaves the disparate file names equivalent. And the apparent random sorting as shown by the OP, may be the result.

[brunogaspar]

Yes, that's where i got too @570studio :)

Hopefully i can try to get into this tomorrow or saturday to see if i can find a clean solution for this, but i'm open for ideas/suggestions :)

[MattStrauss]

So I was initially thinking that this could be solved by adding the sortByDesc collection method to sort the full file names (which would include the paths) before the other sortBy method was called. That way all of the vendor/migration files would appear before the database/migration files and the final sorting should work the way the OP desires, like this:

    public function getMigrationFiles($paths)
    {
        return Collection::make($paths)->flatMap(function ($path) {
            return Str::endsWith($path, '.php') ? [$path] : $this->files->glob($path.'/*_*.php');
        })->filter()->sortKeysDesc(function ($file) {
            return $file; 
        })->sortBy(function ($file) {
            return $this->getMigrationName($file);
        })->values()->keyBy(function ($file) {
            return $this->getMigrationName($file);
        })->all();
    }

But as I though about it more, some devs, use custom migration paths. And since you can use any naming convention that you like in creating a custom migration path, this change could potentially cause those cases some issues. But then again, it would only be a problem if they were using the exact same migration names in their custom path and the regular (or some other) migration path, and their app migration strategy is customized for the current sorting behavior.

Perhaps a more practical solution is simply to update the name of your migrations, if possible, so that they would be sorted after their vendor counterparts under the current scheme. Or perhaps I am not understanding the issue correctly, in which case, you can disregard pretty much all of this ;)

[brunogaspar]

But as I though about it more, some devs, use custom migration paths. And since you can use any naming convention that you like in creating a custom migration path, this change could potentially cause those cases some issues.

Not seeing this as a problem with Laravel itself, but rather how a person name their migrations. But either way, if the files are sorted better on the Migrator class, the problem i'm having would not be a problem anymore since the application migrations would always come after the vendor or any other paths you might be loading the migrations from.

Perhaps a more practical solution is simply to update the name of your migrations

Renaming the migrations does not solve the problem because you can't rename published migrations that belongs to a package as that would introduce other problems like trying to create the same table twice, since the migrations will be pretty much unique.

Or perhaps I am not understanding the issue correctly, in which case, you can disregard pretty much all of this ;)

No, your thinking is correct. We just need to figure out a proper (clean if possible heh) way to sort the files without them to be mixed case in the end :)

Thanks for the inputs so far, really appreciate it :)

[brunogaspar]

@driesvints I have this so far:

public function getMigrationFiles($paths)
{
    return Collection::make($paths)->map(function ($path) {
        $files = Str::endsWith($path, '.php') ? [$path] : $this->files->glob($path.'/*_*.php');

        return Collection::make($files)->filter()->sortBy(function ($file) {
            return $this->getMigrationName($file);
        })->toArray();
    })->flatten()->filter()->values()->keyBy(function ($file) {
        return $this->getMigrationName($file);
    })->all();
}

Basically instead of doing a flatMap i'm doing a map and sorting the files for each path in a more "separated" way i guess, then files from both paths are combined.

With this change, the returned result is what i would expect.

Any clue on how to test it? Since the end result does not contain any valuable data to assert against i'm afraid. There's the possibility of adding a new method i suppose to return the files before being keyed by.. but i don't know what's preferable here

[MattStrauss]

@brunogaspar

Renaming the migrations does not solve the problem because you can't rename published migrations that belongs to a package as that would introduce other problems like trying to create the same table twice, since the migrations will be pretty much unique.

Sorry if I was unclear, I was suggesting you change the name of your migrations, not the vendor migrations. For example, changing the final "0" in your migration files (where you have this conflict) to a "1" to assure that they are sorted after the vendor migration files without making any changes to the framework.

0 => "/vendor/billing/resources/migrations/2019_08_08_100000_billing_rename_table_one.php"
15 => "/database/migrations/2019_08_08_100001_billing_rename_table_one.php"

16 => "/database/migrations/2019_08_08_200001_billing_rename_table_two.php"
1 => "/vendor/billing/resources/migrations/2019_08_08_200000_billing_rename_table_two.php"

17 => "/database/migrations/2019_08_08_300001_billing_rename_table_three.php"
2 => "/vendor/billing/resources/migrations/2019_08_08_300000_billing_rename_table_three.php"

[brunogaspar]

Sorry if I was unclear, I was suggesting you change the name of your migrations, not the vendor migrations. For example, changing the final "0" in your migration files (where you have this conflict) to a "1" to assure that they are sorted after the vendor migration files without making any changes to the framework.

Ohh sorry, i misunderstood you, i was thinking the other way around, my bad.

But to the problem, if i publish the migrations, i shouldn't need to modify any migration file name if i just want to augment them in any way, as that's a bit counter intuitive, in my opinion of course.

Considering that i have this problem for a few migrations, not just one, that will be a bit annoying to perform just because the framework has a bug.

But either way, that would be of course, a hack, and if there's a possibility to fix this issue, i really would like to have it fixed instead of having to hack around it heh

[MattStrauss]

Totally makes sense. I agree that the sorting is a bit off, per your OP.

Perhaps for a test you can try something like below as a starting point. I suggest using the array keys (as the values will be specific to your local setup/location). Also, you may want to add an extra migration file to the current test stub migrations if it will help to clearly define your testing purpose and show that it works correctly.

    public function testMigrationsCanSortsFilesFromMultiplePaths()
    {
        $migrationFileKeys = array_keys($this->migrator->getMigrationFiles([__DIR__.'/migrations/one', __DIR__.'/migrations/two']));
        $this->assertEquals(['2016_01_01_000000_create_users_table', '2016_01_01_100000_create_password_resets_table', '2016_01_01_200000_create_flights_table'], $migrationFileKeys);
    }

That being said, I'm not 100% understanding the logic of the map->sort->flatten strategy you outline below. I get that it works in your scenario, but I'm confused as to what dictates the order in which the migration paths will be sorted.

One last question on your specific example. Am I correct in stating that you publish the vendor migrations files from the given package, and that is how you end up with the duplicately named files? I think this may be a bug within the package (or possibly how the framework handles migrations when published from a vendor), because once a package has its vendor migration files published to the app, then the migration path to the vendor/migrations should no longer be run for the exact reason of the issues that you are currently facing. Does that make sense?

[brunogaspar]

That being said, I'm not 100% understanding the logic of the map->sort->flatten strategy you outline below. I get that it works in your scenario, but I'm confused as to what dictates the order in which the migration paths will be sorted.

Here's a slight modified version of the above using the flatMap still

return Collection::make($paths)->flatMap(function ($path) {
    $files = Str::endsWith($path, '.php') ? [$path] : $this->files->glob($path.'/*_*.php');

    return Collection::make($files)->filter()->sortBy(function ($file) {
        return $this->getMigrationName($file);
    })->toArray();
})->filter()->values()->keyBy(function ($file) {
    return $this->getMigrationName($file);
})->all();

Not sure if it can be made a tad simpler..

But basically, i'm ordering the files inside the map call, instead of ordering them after, which was where the problem was being caused, because it was sorting all the files from all the paths combined.

Am I correct in stating that you publish the vendor migrations files from the given package, and that is how you end up with the duplicately named files?

Yes, that's correct.

I think this may be a bug within the package (or possibly how the framework handles migrations when published from a vendor),

No bug in the package as far as i know. This can in fact happen with any other package.

because once a package has its vendor migration files published to the app, then the migration path to the vendor/migrations should no longer be run for the exact reason of the issues that you are currently facing.

If the package migrations are published, they will not run during any php artisan migrate call, but the framework still needs to do the lookup within all the migration paths, because imagine, you can publish the migrations and delete a couple of the published ones.

I actually tried this approach, publish all migrations for this package, delete the ones i didn't actually need and keep the ones i wanted to augment. Unfortunately it yields the exact same problem for me :

[brunogaspar]

Fired a pull request for this.