[5.0] Automatically Resolve Migrations Dependencies

[antonioribeiro]

This one is to automatically resolve:

class CreateUsersTable extends Migration {

    public function __construct(ClasName $class)
    {
        $this->class = $class;
    }

    ...
}

@GrahamCampbell, before you say anything, there was a bunch of spaces before Filesystem:

public function __construct(MigrationRepositoryInterface $repository,
                            Container $container,
                            Resolver $resolver,
                            Filesystem $files)

So I replaced them all with tabs.

[GrahamCampbell]

@antonioribeiro I think @taylorotwell actually put the spaces there on purpose.

[GrahamCampbell]

Taylor normally aliases contracts when he imports them.

use Illuminate\Contracts\Container\Container as ContainerContract;

[antonioribeiro]

On Filesystem but not on Resolver?

[GrahamCampbell]

Don't know. I've never understood why spaces are sometimes ok.

[antonioribeiro]

Alignment, 1-3 spaces may be needed if you want things straight. In this case there was too much, but let's see what he says about this :)

[driesvints]

He uses spaces to align his parameters neatly underneath eachother. Tabs are used for code indenting. You should use spaces starting from public. @antonioribeiro the originally code snippet was correct I believe.

[antonioribeiro]

This is the original code and it makes no sense to me:
http://i.imgur.com/ZYCQ6U4.png

Even if he chooses to use tabs here and spaces there, the snippet is using both. So you can just use tabs to align in this case.

[driesvints]

@antonioribeiro I believe it should be like this (tabs for indentation, spaces for alignment):

    public function __construct(MigrationRepositoryInterface $repository,
                                ContainerInterface $container, 
                                Resolver $resolver, 
                                Filesystem $files)
    {

[hannesvdvreken]

Why would you alias this ContainerInterface as there is no conflict with another class named Container?

[GrahamCampbell]

You're meant to use ContainerContract anyway. That's Taylor's standard. Why did you change it to interface?

[antonioribeiro]

@hannesvdvreken, because @GrahamCampbell:

Taylor normally aliases contracts when he imports them.

use Illuminate\Contracts\Container\Container as ContainerContract;

[GrahamCampbell]

This line should be aligned with spaces.

[GrahamCampbell]

Ping @antonioribeiro. Please fix the inline issues.

[antonioribeiro]

@GrahamCampbell, sorry but your comments confused me. You were talking about indentation and the difference was in the two spaces in docblocs. I also replaced those identation tabs with spaces.

All changes were amended, please take a look and tell me if there is still something missing.

[GrahamCampbell]

Looks good now. Thanks. :)

[GrahamCampbell]

Actually, no, it's not. You've accepted params to the conductor in a different order to the order you've set the properties. Just a minor inconsistency.

[GrahamCampbell]

incorrect indentation

[GrahamCampbell]

this still needs fixing

[antonioribeiro]

This is the original code:

public function __construct(MigrationRepositoryInterface $repository,
                            Resolver $resolver,
                            Filesystem $files)
{
    $this->files = $files;
    $this->resolver = $resolver;
    $this->repository = $repository;
}

I assumed it was ordered by size, I've seen this more than once, like this

public function __construct(Builder $query, Model $parent, $table, $foreignKey, $otherKey, $relationName = null)
{
    $this->table = $table;
    $this->otherKey = $otherKey;
    $this->foreignKey = $foreignKey;
    $this->relationName = $relationName;

    parent::__construct($query, $parent);
}

Should I fix original code too?

[GrahamCampbell]

Yeh. You should set properties in the same order you received them in the constructor.

[antonioribeiro]

That identation is not indentation, it's alignment, and according to @driesvints I should use spaces on them. Originally that alignment was wrong, because we had 1 line aligned with spaces and the order using tabs. I'm using spaces on all of them now. Is it wrong?

Also you are quoting a line that's not in the same order now. Resolver $resolver, was moved down.

[GrahamCampbell]

Ok, now it's correct. Thanks.

[driesvints]

@antonioribeiro now I don't want this to turn into a giant tabs vs. spaces debate but the current alignment isn't correct. I'll re-paste my previous snippet here (you can just copy/paste this)

    public function __construct(MigrationRepositoryInterface $repository,
                                ContainerContract $container,
                                Resolver $resolver,
                                Filesystem $files)
    {

Notice how the dependencies perfectly match the distance from the first dependency. You only indent with spaces which makes it look like the parameters after the first one don't align properly. Check the diff: https://github.com/laravel/framework/pull/6784/files#diff-a66c98fc0ca13ef8851970c40f8342d1R60

I've added a screenshot to show you what I mean:

Just to give you my opinion on this: this is a ridiculously discussion we're having atm and just a perfect example on why you should use spaces for indentation and not tabs. Tabs just confuses people when mixed with spaces for alignment :)

[GrahamCampbell]

I really think laravel should just take the jump and move to PSR-2, and symfony style docblocks.

[antonioribeiro]

I hope to get this right until the end of the year ;)

Pushed a new one and this is how it's looking in sublime:

[driesvints]

Perfect :)

[antonioribeiro]

Also: I don't totally like the idea of Laravel going 100% PSR-2, because I like the current Allman's Style, it's beautiful and the code flow makes more sense to me.

[GrahamCampbell]

How about psr-2 with allman style braces?

[driesvints]

I think this is a discussion which shouldn't be held here. 

[GrahamCampbell]

Yeh...

[GrahamCampbell]

Boom! #6836.

[taylorotwell]

I've kind of always avoided this as people can just use App::make from within a migration if they need anything from the container, and because one wouldn't typically unit test a migration.

[GrahamCampbell]

:( I really liked this idea. I hate using App::make in constructors. Feels really hacky.

[antonioribeiro]

Also, in my opinion, it's kind of confusing to see classes being autoresolved in some parts of Laravel and not in others. Newcomers might hit a wall until they understand some features aren't supposed to work everywhere.

[taylorotwell]

I just don't understand the point of dependency injecting migrations? Are
you ever really going to inject a mock into a migration?

On Fri, Jan 2, 2015 at 5:11 PM, Antonio Carlos Ribeiro <
notifications@github.com> wrote:

Also, in my opinion, it's kind of confusing to see classes being
autoresolved in some parts of Laravel and not in others. Newcomers might
hit a wall until they understand some features aren't supposed to work
everywhere.

—
Reply to this email directly or view it on GitHub
#6784 (comment).

[antonioribeiro]

People might need to inject classes to help their migrations to happen. Something that happened to me was a simple group of columns I needed in two different tables, so I did:

I have two (very similar) of those:

<?php

use PragmaRX\Support\Migration;
use Illuminate\Database\Schema\Blueprint;
use ConsultorioDigital\Services\Events\Database\Migrations\EventColumns;

class CreateEventsLogTable extends Migration {

    /**
     * @var EventColumns
     */
    private $eventColumns;

    public function __construct(EventColumns $eventColumns)
    {
        $this->eventColumns = $eventColumns;
    }

    /**
     * Run the migrations.
     *
     * @return void
     */
    public function migrateUp()
    {
        Schema::create('events_log', function(Blueprint $table)
        {
            $table->string('id', 64)->primary();
            $table->string('original_event_id', 64)->index()->nullable();

            $this->eventColumns->create($table);
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function migrateDown()
    {
        $this->drop('events_log');
    }

}

and this is the common columns generator:

<?php

namespace ConsultorioDigital\Services\Events\Database\Migrations;

use Illuminate\Database\Schema\Blueprint;

class EventColumns {

    public function create(Blueprint $table)
    {
        $table->string('office_room_id', 64)->nullable();
        $table->timestamp('start')->index();
        $table->timestamp('end')->index();
        $table->boolean('all_day')->default(false);
        $table->string('title')->nullable();
        $table->text('description')->nullable();
        $table->string('user_id', 64)->nullable()->index();
        $table->string('client_id', 64)->nullable()->index();
        $table->string('frequency_id', 64)->nullable();
        $table->string('authorizer_id', 64)->nullable();
        $table->timestamp('authorized_at')->nullable()->index();
        $table->timestamp('completed_at')->nullable()->index();
        $table->string('completed_by_id', 64)->nullable();
        $table->string('room_financial_id', 64)->nullable()->index();
        $table->string('user_financial_id', 64)->nullable()->index();

        $table->timestamps();
    }

}

Of course I could use inheritance or traits, but mind just went first with a common Laravel solution and it didn't worked.

[Anahkiasen]

Also something I often do is injecting migrations in other migrations. Say you add a new table that replaces an other one, in the new migration's down method I just call the old migration's up method to avoid to have to repeat the old structure's fields twice.

[franzliedke]

If I understand Taylor correctly, he's saying we don't need constructor injection here because that's only necessary if you want to unit test your classes - and you probably don't do so for migrations.

Since we already have te Schema facade in the migrations anyway, why not just use App::make() for locating your dependencies in the constructor. It's not quite that clean, but totally enough.

[antonioribeiro]

So if I'm understanding well, reversing your logic, one who doesn't unit test is not supposed to use constructor injection, and the new method injection, they are off limits? Sounds depressing, because those are fantastic Laravel features and everyone should be a allowed to use them If you are trying to teach by example, it might not be the best didactic.

I personally do not have a problem with this, every time I hit a wall I just extend the service and add what I need. I had to extend migrations to use transactions and rollback failed migrations (on PosgtreSQL), and also to be able to namespace, separate migrations per service and selectively enable them. But, when you expect people to have aha moments like "of course this feature is not supposed to work here, we don't inject mocks into migrations!", I fear exclusion and, in the future, docs may have to change a bit:

"PHP THAT DOESN'T HURT PROS"

[GrahamCampbell]

I agree. I inject dependencies even when I don't bother writing tests for
things.
On 3 Jan 2015 15:06, "Antonio Carlos Ribeiro" notifications@github.com
wrote:

So if I'm understanding well, reversing your logic, one who doesn't unit
test is not supposed to use constructor injection, and the new method
injection, they are off limits? Sounds depressing, because those are
fantastic Laravel features and everyone should be a allowed to use it. If
you are trying to teach by example, it might not be the best didactic.

I personally do not have a problem with this, every time I hit a wall I
just extend the service and add what I need. I had to extend migrations to
use transactions and rollback failed migrations (on PosgtreSQL), and also
to be able to namespace, separate migrations per service and selectively
enable them. But, when you expect people to have aha moments like "of
course this feature is not supposed to work here, we don't inject mocks
into migrations!", I fear exclusion and, in the future, docs may have to
change a bit:

"PHP THAT DOESN'T HURT PROS"

—
Reply to this email directly or view it on GitHub
#6784 (comment).

[hannesvdvreken]

I believe you can always use App::call and App::make. It is not necessary to add a dependency on illuminate/container to every other Illuminate component just for the sake of having auto dependency injection everywhere. One can always use the facade and App::wrap to bring those features to every part of the application within the Laravel framework.

Wrote a blog post on this a couple of days ago. It's not finished but feedback is welcome. hannesvdvreken.github.io

[mysteryos]

Just hit that bump. Went the typical DI fashion in migrations, ended up with App::make.

It would create consistency within the laravel application to have DI supported in all app classes.