transaction and migrations with error

[sanoma-zz]

This package looks promising, but I've found something problematic. The migrations don't care about errors. It would be nice a safeUp method with transaction handling. If there is a migration with error, then don't migrate it.

[davedevelopment]

Hi there, could you post an example of what you would like to work? From personal experience, I find it better to manage these things within the scope of the migration code itself.

[sanoma-zz]

I like the db migration mechanism of the Yii framework. If there is an error in the migration it stops there and rollback the other commands in the migration.
Why should I write every time the transaction and error handling? It's not DRY.

public function up()
{
    $transaction=$this->getDbConnection()->beginTransaction();
    try
    {
        if($this->safeUp()===false)
        {
            $transaction->rollback();
            return false;
        }
        $transaction->commit();
    }
    catch(Exception $e)
    {
        echo "Exception: ".$e->getMessage().' ('.$e->getFile().':'.$e->getLine().")\n";
        echo $e->getTraceAsString()."\n";
        $transaction->rollback();
        return false;
    }
} 

[davedevelopment]

I get that, but a tool like that is highly integrated in the framework, where as phpmig isn't. Phpmig doesn't know anything about the database you are migrating, or how your migrations access that database. It only knows where to record that a particular migration was run. Because I use doctrine dbal to run my migrations I can just do:

public function up()
{
    $this->get('dbal')->transactional(function ($conn) {
        $conn->query("UPDATE users SET premium = 0 WHERE premium IS NULL");
    });
}

[sanoma-zz]

Ok, I see. But it would be useful, if there would be two events. A beforeUp and afterUp. In the container i would define a trait name, which controls the before and after events. And when the phpmig create the migration file, it uses the trait in the created template.
We have CI server, and I want to make sure, if somebody makes a mistake, the automatic deployment should handle the error somehow.

Doctrine dbal throws an exception if the query fail?

[davedevelopment]

That sounds like a good idea, we could just put some noop methods in the default migration class, people can override them as they wish in a custom base migration class.

DBAL does throw exceptions when queries fail.

[sanoma-zz]

Yeah, that would be a nice feature.

[edisonnica]

I did a change in the way the migrator works in my fork. Basically, it is now the Adapter responsibility to execute the migration,

"Create execute() method in adapter that calls the ops (e.g. migration::up() and adapter::up()). Create SimpleAdapter to implement execute. In this way the adapter is the only one that has intimate knowledge about operations (e.g. if migration run in a transaction). The adapter can perform extra work like beforeUp/afterUp/beforeDown/afterDown without over engineering phpmig and polluting
migration files with boilerplate."

Example of adapter:

class TransactionalSqlAdapter extends Adapter\PDO\Sql {

public function __construct(\PDO $connection, $tableName) {
    parent::__construct($connection, $tableName);
}

/**
 * Execute Migration
 *
 * @param Migration $migration
 * @param string $direction
 * @return Boolean
 */
public function execute(Migration $migration, $direction) {
    // TODO(edison.nica): if we have migrations that are not supposed to run in a transaction:
    // (1) create "TransactionalMigration extend Migration"
    // (2) add the runInTransaction function.
    // (3) make all our migrations extend TransactionalMigration instead Migration.
    // For now we assume that all migratins are transactional going forward.
    $runInTransaction = True; //$migration->runInTransaction();
    if ($runInTransaction === True) {
        $migration->getContainer()['db']->beginTransaction();
    }
    $successfulTransaction = True;
    try {
        $successfulTransaction = parent::execute($migration, $direction);

        if ($runInTransaction === True) {
            $successfulTransaction = $migration->getContainer()['db']->commit();
        }
    } catch (\PDOException $e) {
        $successfulTransaction = False;
        if ($runInTransaction === True) {
            $migration->getContainer()['db']->rollback();
        }
    }

    return $successfulTransaction;
}

}

This has the added benefit that if the transaction fails, the migration table is intact, instead of marking the migration as executed.

Let me know what you think and if you are interested to pull this change - unfortunately some people might have to rewrite their custom adapters.

https://github.com/edisonnica/phpmig/commit/92c2f3736b86a7df2dd1cfee51d881e17673c2c8