Skip to content

Commit

Permalink
Testing All methods for addSql
Browse files Browse the repository at this point in the history
  • Loading branch information
@mikeSimonson
mikeSimonson committed Oct 18, 2015
1 parent dbd47fd commit d393217
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 39 deletions.
21 changes: 13 additions & 8 deletions tests/Doctrine/DBAL/Migrations/Tests/Functional/FunctionalTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,25 +180,30 @@ public function testAddSql()

public function testAddSqlInPostUp()
{
$this->config->registerMigration(1, 'Doctrine\DBAL\Migrations\Tests\Stub\Functional\MigrateAddSqlPostUpTest');
$tableName = \Doctrine\DBAL\Migrations\Tests\Stub\Functional\MigrateAddSqlPostUpTest::TABLE_NAME;
$this->config->registerMigration(1, 'Doctrine\DBAL\Migrations\Tests\Stub\Functional\MigrateAddSqlPostAndPreUpAndDownTest');
$tableName = \Doctrine\DBAL\Migrations\Tests\Stub\Functional\MigrateAddSqlPostAndPreUpAndDownTest::TABLE_NAME;

$this->config->getConnection()->executeQuery(sprintf("CREATE TABLE IF NOT EXISTS %s (test INT)", $tableName));

$migration = new \Doctrine\DBAL\Migrations\Migration($this->config);
$migration->migrate(1);

$migrations = $this->config->getMigrations();
$this->assertTrue($migrations[1]->isMigrated());

$schema = $this->config->getConnection()->getSchemaManager()->createSchema();
$this->assertTrue($schema->hasTable($tableName));
$check = $this->config->getConnection()->fetchAll("select * from $tableName");
$check = $this->config->getConnection()->fetchColumn("select SUM(test) as sum from $tableName");

$this->assertNotEmpty($check);
$this->assertEquals('test', $check[0]['test']);
$this->assertEquals(6, $check);

$migration->migrate(0);
$this->assertFalse($migrations[1]->isMigrated());
$schema = $this->config->getConnection()->getSchemaManager()->createSchema();
$this->assertFalse($schema->hasTable($tableName));
$check = $this->config->getConnection()->fetchColumn("select SUM(test) as sum from $tableName");
$this->assertNotEmpty($check);
$this->assertEquals(21, $check);


$this->config->getConnection()->executeQuery(sprintf("DROP TABLE %s ", $tableName));
}

public function testVersionInDatabaseWithoutRegisteredMigrationStillMigrates()
Expand Down
60 changes: 60 additions & 0 deletions ...s/Doctrine/DBAL/Migrations/Tests/Stub/Functional/MigrateAddSqlPostAndPreUpAndDownTest.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php

namespace Doctrine\DBAL\Migrations\Tests\Stub\Functional;

use Doctrine\DBAL\Migrations\AbstractMigration;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\DBAL\Types\Type;

class MigrateAddSqlPostAndPreUpAndDownTest extends AbstractMigration
{
const TABLE_NAME = 'test_add_sql_post_up_table';

public function preUp(Schema $schema)
{
$this->addSql(
sprintf("INSERT INTO %s (test) values (?)", self::TABLE_NAME),
[1]
);
}

public function up(Schema $schema)
{
$this->addSql(
sprintf("INSERT INTO %s (test) values (?)", self::TABLE_NAME),
[2]
);
}

public function postUp(Schema $schema)
{
$this->addSql(

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Oct 20, 2015

Member

this makes no sense. post* methods cannot add SQL as the SQL has already run. If they want to do things here, they need to use the connection

This comment has been minimized.

Sign in to view

Copy link
@mikeSimonson

mikeSimonson Oct 20, 2015

Author Contributor

Well was there any reason for the preUp, postUp, up and preDown, down, and postDown to all have the same signature but not to be able to add sql ?

Providing the same ability for all those methods allows me to greatly simplify the logic of the execute. See #366

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Oct 20, 2015

Member

none of them was meant to be places where addSql is used AFAIK. They are meant to perform actions before and after the migration itself.

and the PR does not simplify the logic at all. It only simplifies it because it is buggy.

This comment has been minimized.

Sign in to view

Copy link
@mikeSimonson

mikeSimonson Oct 20, 2015

Author Contributor

I know the PR is buggy right now.
I am also trying to figure out what should be the difference between the pre, post method and the up or down. Or if there should be any difference at all for that matter.
Can I call you sometime (skype / phone / whatever )?

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Oct 20, 2015

Member

The goal of post* method is to run some code after the SQL was executed (for instance triggering an RabbitMQ message to launch a synchronization of your data to ElasticSearch after the migration updated them, which may be a use case).
The pre* methods are allowing to manipulate the initial schema. Changes done in the Schema object there won't be applied by the migration. It compares the schema between after preUp and after up. I don't have much use case for this method actually (mostly because I never use the Schema object in my migrations).

These methods are quite edge cases though. Most migrations only need to rely on up and down.

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Oct 20, 2015

Member

IMO, a better way would be to forbid using addSql in post* methods (stable versions of the library are ignoring queries added there, so we would simply tell users they made a mistake). We cannot do it for pre* methods (as this would be a BC break), but it would make sense to warn in this case (while still accepting it for BC)

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Oct 21, 2015

Member

@mikeSimonson I suggest moving this discussion into a dedicated issue to be easier to follow it: #369
sprintf("INSERT INTO %s (test) values (?)", self::TABLE_NAME),
[3]
);
}

public function preDown(Schema $schema)
{
$this->addSql(
sprintf("INSERT INTO %s (test) values (?)", self::TABLE_NAME),
[4]
);
}

public function down(Schema $schema)
{
$this->addSql(
sprintf("INSERT INTO %s (test) values (?)", self::TABLE_NAME),
[5]
);
}

public function postDown(Schema $schema)
{
$this->addSql(
sprintf("INSERT INTO %s (test) values (?)", self::TABLE_NAME),
[6]
);
}
}
31 changes: 0 additions & 31 deletions tests/Doctrine/DBAL/Migrations/Tests/Stub/Functional/MigrateAddSqlPostUpTest.php
View file

This file was deleted.

0 comments on commit d393217

Please sign in to comment.