Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] Configure Schema on migration instantiation #29983

Closed
wants to merge 2 commits into from
Closed

[7.x] Configure Schema on migration instantiation #29983

wants to merge 2 commits into from

Conversation

antonkomarev
Copy link
Contributor

@antonkomarev antonkomarev commented Sep 13, 2019

This PR adds to abstract Migration class $schema property with configured connection from Migration's getConnection() method.

Motivation

Migration has getConnection() method which used inside of the Migrator class to determine if schema transactions are supported:

protected function runMigration($migration, $method)
{
    $connection = $this->resolveConnection(
        $migration->getConnection()
    );

    $callback = function () use ($migration, $method) {
        if (method_exists($migration, $method)) {
            $this->fireMigrationEvent(new MigrationStarted($migration, $method));

            $migration->{$method}();

            $this->fireMigrationEvent(new MigrationEnded($migration, $method));
        }
    };

    $this->getSchemaGrammar($connection)->supportsSchemaTransactions()
        && $migration->withinTransaction
                ? $connection->transaction($callback)
                : $callback();
}

But when migration is running Schema facade is being used and it doesn't care about getConnection() method of the migration. Migrator checks connection and enables transactions, but in fact migrations could be executed on another connection.

To respect database connection, instead of this:

Schema::create('users', function (Blueprint $table) {
    $table->bigIncrements('id');
    $table->timestamps();
});

Schema::dropIfExists('users');

We should write this:

Schema::connection($this->getConnection())->create('users', function (Blueprint $table) {
    $table->bigIncrements('id');
    $table->timestamps();
});

Schema::connection($this->getConnection())->dropIfExists('users');

New Implementation

$this->schema->create('users', function (Blueprint $table) {
    $table->bigIncrements('id');
    $table->timestamps();
});

$this->schema->dropIfExists('users');

Similar approach implemented in Laravel Telescope, but there it doesn't get connection name from getConnection() method, what means that Migrator wouldn't determine correctly if schema transactions are supported.

@taylorotwell
Copy link
Member

No plans to change this right now.

@antonkomarev
Copy link
Contributor Author

@taylorotwell this solution is not how you see it should work or what?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants