Framework: About migrations and multiple class names

[joaocagnoni]

I'll try to simplify my problem with a common case:

1- On the version 1.0 of my app I create a migration "create_news_table".
2- On 1.1, I create a migration which removes the news table.
3- On 1.2, I try to create the migration "create_news_table" with a totally different table.

On the step 3, the class name of the migration is the same of the first migration (causing an error). If I rename the class name of the last migration, the first one will be created.

Any solution for this? Why do we have to use names for migrations? They should have been using lambdas intead of classes as they are not unique and don't have an identity.

I found another issue talking about this, but the comment of taylorotwell is ridiculous: https://github.com/laravel/framework/issues/5459

You shouldn't really answer issues like this if you don't want to see Laravel being used only for static sites and/or landing pages like CodeIgniter was. It's a problem that affects all the users that's using Laravel on complex websites/systems with a short to medium sized versions story.

[franzliedke]

Just use different names? recreate_news_table?

[joaocagnoni]

No, this is still a bug. If I have to recreate it again, I'd have to use recreate_news_table_again, and then recreate_news_table_again_again. This is not what we're looking for.

As I said, migrations should not have an unique name, because this is a wrong concept. Also, the migrations filename has a timestamp on it, meaning it should NOT be unique, but the classes are (???).

[GrahamCampbell]

This is not a bug. It's just an inconvenience.

[jsanc623]

Can we have core just add the timestamp to the class name? Something like class RecreateNewsTable20140922142354 extends Migration {} would solve the issue.

Edit: I've modified MigrationCreator.php to reflect my idea above, though I doubt Taylor would accept it as it wouldn't jive with the LR philosophy: 99fb2c452fb3d7b07945ce3f738f1872278f8842

[joaocagnoni]

@jsanc623 It seems a good alternative with much less code, but I'll ask again, why do migrations uses classes rather than lambdas? They're very similar to routes. Just imagine if each route had one class...

[jsanc623]

@joaocagnoni Not quite sure on the design decision behind using classes - though I would imagine its easier to organize the migration. For example, in a project I worked on, this was one of the migrations:

use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class Users extends Migration {

    public $table = "users";

    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up() {
        # Check if table exists, if not create it and call this function again
        if ( Schema::hasTable( $this->table ) ) {
            # Update table
            Schema::table( $this->table, function ( $table ) {
                $table->string( "first_name", 40 )->nullable();
                $table->string( "last_name", 50 )->nullable();
                $table->string( "username", 30 )->unique();
                $table->string( "password", 128 );
                $table->string( "uuid_private", 128 );
                $table->string( "uuid_public", 64 );
                $table->integer( "facebook_id" )->nullable();
                $table->string( "email", 156 )->unique();
                $table->integer( "birth_month" )->nullable();
                $table->integer( "birth_day" )->nullable();
                $table->integer( "birth_year" )->nullable();
                $table->string( "gender", 1 )->nullable();
            } );
        } else {
            # Create the base users table
            Schema::create( $this->table, function ( $table ) {
                $table->engine = 'InnoDB';
                $table->increments( "id" );
                $table->timestamps();
            } );

            # Call this function again now that the table exists
            $this->up();
        }
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down() {
        Schema::drop( $this->table );
    }

}

Doing the above with lambdas would be a pain and would create very ugly code.

[aik099]

The timestamp is added to migration filename automatically. Why just we can't add same timestamp to migration class name as well?

[GrahamCampbell]

Maybe something to discus on the forums/irc.

[joaocagnoni]

@jsanc623 Why are you checking if a table exists? Are you changing the DB without migrations? It's a bad practice.

I think the code i'll look much cleaner with closures. There's no sense at creating a class using only 2 methods in 99% of the time. So here it is:

Migration::create(function() {
    Schema::create('news', function ($table) {
        $table->string("title");
        $table->string("category");
        $table->string("author");
    });
}, function() {
    Schema::drop('news');
});

Syntax looks like a "hover" on jQuery. Everybody understands a hover.

[jsanc623]

@joaocagnoni I didn't write the migration - I took over the project and this was in there

@aik099 This is exactly what I referenced in an earlier comment: 99fb2c452fb3d7b07945ce3f738f1872278f8842

[crynobone]

Why not just extends MigrationCreator and maintain it per app.

<?php namespace App\Database\Migrations;

class MigrationCreator extends \Illuminate\Database\Migrations\MigrationCreator 
{
     // customize.
}

Create your own MigrationServiceProvider:

<?php namespace App\Providers;

use App\Database\Migrations\MigrationCreator;

class MigrationServiceProvider extends \Illuminate\Database\MigrationServiceProvider
{
    /**
     * Register the migration creator.
     *
     * @return void
     */
    protected function registerCreator()
    {
        $this->app->bindShared('migration.creator', function($app)
        {
            return new MigrationCreator($app['files']);
        });
    }
}

This however will not work if you use any package that use migration, and I don't think most package developer would love the idea to change the migration structure.

p/s: Also maintaining a SaaS, and don't have a problem with current implementation.

[isometriq]

@crynobone Thanks for that, I've been able to override the MigrationCreator class.
For example, now the command artisan make:migration add_fieldname will create the file 2016_02_07_161200_add_fieldname.php and the class name will be AddFieldnameMigration_20160207161200

For reference, here's how the class looks like

<?php

namespace App\Database\Migrations;

class MigrationCreator extends \Illuminate\Database\Migrations\MigrationCreator
{
    protected function getClassName($name)
    {
        return parent::getClassName($name) . 'Migration_' . str_replace('_', '', $this->getDatePrefix());
    }
}

[isometriq]

Forgot a thing, for the migrate to work, I had to override the Migrator class, to apply the same migration class name pattern. Essentially it tries to use my custom class name, but if it fails, it will try with the default behavior of the parent class. That should allow the vendors/packages migrations to work. The override is also done in the MigrationServiceProvider class, as described by @crynobone

For reference, here's how the class looks like

<?php

namespace App\Database\Migrations;

use Illuminate\Support\Str;

class Migrator extends \Illuminate\Database\Migrations\Migrator
{
    /**
     * Resolve a migration instance from a file.
     *
     * @param  string  $file
     * @return object
     */
    public function resolve($file)
    {
        $fileParts = explode('_', $file);
        $class = Str::studly(implode(' ', array_slice($fileParts, 4))) . 'Migration_' . implode('', array_slice($fileParts, 0, 4));

        if (!class_exists($class)) {
            return parent::resolve($file);
        }

        return new $class;
    }
}

[asvae]

Had a fairly nasty deploy because of that "inconvenience".
On dev I did migrations one by one. I causes no class name clashes that way and I didn't notice one name is duplicate. Then, on production, bad things happened. Namely, failed migration for no reason. Troubles.

@isometriq That's sensible approach. Thanks for your solution.

[voveson]

@Asvae I had the exact same experience yesterday.

I think when php artisan make:migration ... is run, it should either check for class name conflicts, or just add the timestamp to the class name as was suggested above.

[moebaca]

Was anything ever made of this? I am going to go ahead and just start using the filename as the classname with the big timestamp appended to it.

[asvae]

@moebaca it doesn't seem so. It appears for me that doctrine migrations are much more suited for serious development than laravel ones.

Here's an article to get started.

[emptimd]

For php7+ and laravel 5.4 This code worked great for me.

`

namespace App\Providers;

class MigrationServiceProvider extends \Illuminate\DatabaseMigrationServiceProvider
{

protected function registerMigrator()
{
    $this->app->singleton('migrator', function ($app) {
        $repository = $app['migration.repository'];

        return new class($repository, $app['db'], $app['files']) extends \Illuminate\Database\Migrations\Migrator {
            public function resolve($file)
            {
                $fileParts = explode('_', $file);
                $class = \Illuminate\Support\Str::studly(implode(' ', array_slice($fileParts, 4))) . 'Migration_' . implode('', array_slice($fileParts, 0, 4));

                if (!class_exists($class)) {
                    return parent::resolve($file);
                }

                return new $class;
            }
        };
    });
}

protected function registerCreator()
{
    $this->app->singleton('migration.creator', function ($app) {
        return new class($app['files']) extends \Illuminate\Database\Migrations\MigrationCreator {
            protected function getClassName($name)
            {
                return parent::getClassName($name) . 'Migration_' . str_replace('_', '', $this->getDatePrefix());
            }
        };
    });
}

}`

And add it to config/app.php
App\Providers\MigrationServiceProvider::class, // Unique class name migrations.

[isometriq]

This sucks.. on Lumen the MigrationServiceProvider gets registered AFTER by the kernel ..I will have to override another thing Illuminate\Database\Console\Migrations\MigrateMakeCommand ..if that works 👎

Why isn't fixed!? maybe you could do at least something like ./config/migrate.php if the fear of breaking changes is the cause...

@asvae indeed ..i can hear Doctrine calling me

[isometriq]

In case someone is interested, I was able to hack it by overriding the Kernel included in my app ./app/Console/Kernel.php, like this:

<?php

namespace App\Console;

use Laravel\Lumen\Application;
use Illuminate\Console\Scheduling\Schedule;
use Laravel\Lumen\Console\Kernel as ConsoleKernel;

class Kernel extends ConsoleKernel
{
    /**
     * The Artisan commands provided by your application.
     *
     * @var array
     */
    protected $commands = [
        //
    ];

    public function __construct(Application $app)
    {
        parent::__construct($app);

        $app->register(\App\Providers\MigrationServiceProvider::class);
    }

    /**
     * Define the application's command schedule.
     *
     * @param  \Illuminate\Console\Scheduling\Schedule  $schedule
     * @return void
     */
    protected function schedule(Schedule $schedule)
    {
        //
    }
}

The MigrationCreator, Migrator and MigrationServiceProbider described above are still needed. This is a hack so that my registration of the provider is the last one ..sadly it's not elegant, but does the job right?

Now I'm able to create migrations with timestamp and then use them with all migrate:* commands. If you want to use this in your Lumen installation, please do some test before...

[poisa]

Yeah, I'm maintaining a project with about 200 migrations and the migration names at this point are just ridiculous.

On the topic of using lambdas (what @joaocagnoni mentioned), I don't think that would be a better solution. In our case we have "system" migrations (the central database) and we have "tenant" migrations in a different folder (each client's database). Some tenant tables are exactly the same as the system ones so we just instantiate the system migration class and run it in a tenant (we subclassed our own migration class to be able to pass the DB connection around). This would not have been pretty with closures. Still I feel your pain and wish the problem you mention had a better solution.

[tostercx]

Same situation as above. What's worse is that adding and executing migrations one by one used to work but after upgrading laravel (unsure which version changed this, went from 5.2->6.x) you can't create new migrations from artisan without fixing all previous name conflicts.

_edit_

May be a good thing now that I think about it.

[KingComp]

Thanks everyone!

Laravel 7.
Did as in Yii2.

filename: m200703_124707_create_user_table
classname: m200703_124707_create_user_table

<?php 


namespace App\Providers;


use Illuminate\Database\Migrations\MigrationCreator;
use Illuminate\Database\Migrations\Migrator;

class MigrationServiceProvider extends \Illuminate\Database\MigrationServiceProvider
{
    protected function registerMigrator()
    {
        $this->app->singleton('migrator', function ($app) {
            $repository = $app['migration.repository'];

            return new class($repository, $app['db'], $app['files']) extends Migrator {
                public function resolve($file)
                {
                    $class = $file;

                    if (!class_exists($class)) {
                        return parent::resolve($file);
                    }

                    return new $class;
                }
            };
        });
    }

    protected function registerCreator()
    {
        $this->app->singleton('migration.creator', function ($app) {
            return new class($app['files'], $app->basePath('stubs')) extends MigrationCreator {
                protected function getDatePrefix()
                {
                    return 'm' . date('ymd_His');
                }

                protected function getClassName($name)
                {
                    return $this->getDatePrefix() . '_' . $name;
                }
            };
        });
    }
}

And add it to config/app.php
App\Providers\MigrationServiceProvider::class,

[eduardoturconi]

Why is this closed yet?
That "inconvenience" isn't just an inconvenience.

[EdgarSedov]

Fully agree. What's the problem with making this behavior bc-compatible with config param?
Just because taylorotwell doesn't like it?