[6.x] Resolve migration classes via the container

[DuckThom]

• Resolve migration classes via the container to allow DI in migrations

[driesvints]

We can't use a global foundation helper here. This couples the database component directly to the framework and will prevent it from being used outside of it.

[driesvints]

Can you maybe explain more in detail why this is needed? Provide some use cases.

[DuckThom]

@driesvints

Personally, I'm not a big fan of using Facades, so in our apps we tend use DI everywhere to inject our classes, with the exception being our migrations. Aside from this, I'd say having the possibility to use DI here would never be a bad thing considering it's possible to use it virtually everywhere else as well.

As for the global helper, I forgot that illuminate/database is also a separate package. Seeing as this package is also depending on illuminate/container, I can change resolve($class) to Container::getInstance()->make($class) with a use Illuminate\Container\Container; statement at the top, to not break BC by changing the constructor.

[taylorotwell]

It's not needed. Use Facades. It seriously does not matter at all if you use facades in your migrations. It's good to just move past the superstition in this case.

[DuckThom]

I don't see why it's a problem to resolve the classes using the container, it doesn't remove any current features, it only adds new possibilities.

Besides that, the main reason I don't like Facades is because IDE's can't autocomplete their methods without a third party module, and I don't want to install or add unnecessary dependencies to my apps if I can help it. So honestly, I don't see the benefits of using Facades over DI.

[driesvints]

@DuckThom I personally also don't use facades much myself but in this particularly use case I don't see how DI provides benefit here. The point of a migration is to change a DB structure which you generally want to test as an integration test with a real database. I don't think that DI provides any benefit here. If you need more logic inside your migration than the default structure changes it might be best to extract to a separate class and unit test that class.

[DuckThom]

@driesvints I agree that it doesn't provide a benefit to migrations but it also doesn't remove any existing functionality. It's more to be able to keep the code consistent with the rest of your codebase and to be able to use autocompletion in IDEs without adding extra dependencies in the app. This 2 line change just allows you to inject classes in the constructor of a migration in case it's needed.

I'll just keep my override then, just in case anyone else is looking for the same functionality, these are my changes:

In my service provider's register method I have:

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

    return new Database\Migrator($repository, $app['db'], $app['files'], $app['events']);
});

And the custom migrator class (removed some newlines to keep the codeblock shorter):

use Illuminate\Database\Migrations\Migrator as LaravelMigrator;

class Migrator extends LaravelMigrator {
    public function resolve($file) {
        $class = Str::studly(implode('_', array_slice(explode('_', $file), 4)));
        return Container::getInstance()->make($class);
    }
}

[driesvints]

@DuckThom there's no need in adding code which won't be used.

[DuckThom]

@driesvints Well that's kind of my point for having to use the extra dependency for facade IDE autocompletion aswell 😛