Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Do not create schema to see if the migrations table exists. #94

Merged
merged 1 commit into from

4 participants

@kimhemsoe

Sorry for the extra changed lines.

@beberlei beberlei merged commit b38ddb1 into doctrine:master

1 check passed

Details default The Travis build passed
@rhunwicks

This commit breaks Migrations for postgresql users with a schema-specific migrations table.

I do not have a public schema in my postgresql database and my migrations table is specified as "schema_name.doctrine_migrations_table". This worked fine until this commit was merged.

The change to $this->connection->getSchemaManager()->tablesExist(array($this->migrationsTableName)) now breaks because tablesExist returns a array of table names without schema prefixes and consequently createMigrationTable attempts to create the table but fails because it already exists.

@kimhemsoe

I see if i can get pg up and running and figure something out..

have some "issues" with my PG install.

getting "PDOException: SQLSTATE[08006] [7] FATAL: sorry, too many clients already" and have not investigated what going on. Installed with macports. Any good idea's. I will try to fix the bug.

@kimhemsoe

@rhunwicks what version of dbal are you using ?

@rhunwicks
cat composer.lock | grep -A4 -i dbal
            "package": "doctrine/dbal",
            "version": "2.3.x-dev",
            "source-reference": "fdc866a37959e43620e4f7ec519dc7dd8e30fc5b",
            "commit-date": "1348120597"
        },
@rhunwicks

Further investigation shows the problem is with Doctrine\DBAL\Schema\PostgreSqlSchemaManager::_getPortableTableDefinition($table)

If the schema for the table is the same as the first schema in the search path, then it returns the bare table name without a schema qualifier. This seems fine in general, but when we call tablesExist() with a schema-qualified name in the first-searched schema it returns false because tablesExist() calls $this->listTableNames() which in turn calls _getPortableTablesList() which returns the bare table name without the schema prefix and so the array_intersect misses it

@kimhemsoe

also why im looking into maybe using listTableColumns or some other direct call to the db if possible. Havent tested it yet tho. Just an idea. That or go back to the super slow createSchema

@rhunwicks

The least intrusive way, given the error is a result of the definition of _getPortableTableDefinition() in PostgreSqlSchemaManager is to create PostgreSqlSchemaManager::tablesExist

    /**
     * Return true if all the given tables exist.
     *
     * @param array $tableNames
     * @return bool
     */
    public function tablesExist($tableNames)
    {
        foreach ($tableNames as $key => $tableName) {
            if (strpos($tableName, '.') !== false) {
                $tableName = explode('.', $tableName, 2);
                $tableNames[$key] = $this->_getPortableTableDefinition(array('schema_name'=>$tableName[0], 'table_name'=>$tableName[1]));
            }
        }
        return parent::tablesExist($tableNames);
    }
@kimhemsoe

Think i add some unit tests to tests this for each db. I never been a big user of PG or.. other dbs then mysql. Something i will change soonish...

@rhunwicks

I think this issue should be reopened - as the current situation will break Migrations for some Postgresql users the next time they do a composer.phar update and it will not be obvious if they look on GitHub that the issue is known.

@stof
Collaborator

@rhunwicks This is a PR, not an issue. So we cannot reopen it as it has been merged. If there is an issue, pleas eopen a dedicated ticket instead of using comments on a merged PR.

@kimhemsoe

I will post a fix later today you can test out for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 5 additions and 6 deletions.
  1. +5 −6 lib/Doctrine/DBAL/Migrations/Configuration/Configuration.php
View
11 lib/Doctrine/DBAL/Migrations/Configuration/Configuration.php
@@ -62,7 +62,7 @@ class Configuration
/**
* OutputWriter instance for writing output during migrations
- *
+ *
* @var OutputWriter
*/
private $outputWriter;
@@ -76,7 +76,7 @@ class Configuration
/**
* The path to a directory where new migration classes will be written
- *
+ *
* @var string
*/
private $migrationsDirectory;
@@ -159,7 +159,7 @@ public function getOutputWriter()
/**
* Returns a timestamp version as a formatted date
*
- * @param string $version
+ * @param string $version
* @return string $formattedVersion The formatted version
*/
public function formatVersion($version)
@@ -207,7 +207,7 @@ public function getMigrationsTableName()
/**
* Set the new migrations directory where new migration classes are generated
*
- * @param string $migrationsDirectory The new migrations directory
+ * @param string $migrationsDirectory The new migrations directory
*/
public function setMigrationsDirectory($migrationsDirectory)
{
@@ -464,8 +464,7 @@ public function createMigrationTable()
return false;
}
- $schema = $this->connection->getSchemaManager()->createSchema();
- if ( ! $schema->hasTable($this->migrationsTableName)) {
+ if ( ! $this->connection->getSchemaManager()->tablesExist(array($this->migrationsTableName))) {
$columns = array(
'version' => new Column('version', Type::getType('string'), array('length' => 255)),
);
Something went wrong with that request. Please try again.