Dbal: The changes from #3392 break indexes with a fixed length

[leofeyer]

BC Break Report

| Q | A
|------------ | ------
| BC Break | yes
| Version | 2.9.1

Summary

The changes from #3392 break indexes with a fixed length.

Previous behaviour

$table = new Table('test');
$table->addColumn('path', 'text');
$table->addIndex(['path' => 'path(768)'], 'path', [], ['lengths' => [768]]);

This added the following index:

Current behavior

Doctrine\DBAL\Schema\SchemaException:
There is no column with name 'path(768)' on table 'test'.

The exception seems to occur due to the removal of the following lines in the Table::_createIndex() method:

        if (is_numeric($columnName) && is_string($indexColOptions)) {
            $columnName = $indexColOptions;
        }

[morozov]

@leofeyer could you clarify why you're using this syntax for fixed length indices? AFAIK, the support for such indices was only added in v2.9.0, and the syntax is expected to look like the following:
https://github.com/doctrine/dbal/blob/e37fc54d91a9bad1a7579520f2ee63c90e9099e3/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php#L1565-L1569

The first argument contains column names, the last contains the lenghts which is sufficient to define the index.

[leofeyer]

We have been using it since Doctrine DBAL 2.6, however, you are right about the lengths option, which we have only added very recently. Before that the code looked like this:

$table = new Table('test');
$table->addColumn('path', 'text');
$table->addIndex(['path' => 'path(768)'], 'path');

[morozov]

@leofeyer it doesn't look like valid usage to me. The first argument of Table::addIndex() has always been array $columnNames. It accepts a list of names, not a list of SQL expressions like you're trying to pass.

[leofeyer]

it doesn't look like valid usage to me

Ok, then please tell me how to achieve the following data structure:

Because this is exactly what $connection->getSchemaManager()->createSchema() returns and also what phpMyAdmin creates:

And it cannot be created like this either:

$table->addIndex(['path(768)'], 'path', [], ['lengths' => [768]]);

So how do I accomplish it?

[morozov]

@leofeyer does the approach covered in https://github.com/doctrine/dbal/issues/3409#issuecomment-447411653 work for you?

[leofeyer]

No, it does not, because it does not set the correct column name. The column name needs to be path(768) to match the return value of $schemaManager->createSchema().

Create schema from database

CREATE TABLE test (path text NULL);
CREATE INDEX path ON test (path(768));

$connection = $this->get('database_connection'); // Symfony DI container
dump($connection->getSchemaManager()->createSchema()->getTable('test'));

Create schema manually

$table = new Table('test');
$table->addColumn('path', 'text');
$table->addIndex(['path'], 'path', [], ['lengths' => [768]]);
dump($table);

And because of this difference, Schema::getMigrateToSql() no longer handles the index correctly.

Another possible solution

Maybe $table->addIndex(['path'], 'path', [], ['lengths' => [768]]); should just add the index length to the column name? Then the manual schema would match the created schema again.

[morozov]

The column name needs to be path(768) to match the return value of $schemaManager->createSchema().

It should be the opposite. path(768) is not a column name. It's an SQL expression representing an index column (by name) and its length.

When a table is created from its definition by the DBAL, the SQL looks like:

CREATE TABLE index_length (
    path TEXT NULL,
    INDEX IDX_18E36143B548B0F (path(768))
) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB

and the introspection works correctly (the following test passes):

public function testIndexLength()
{
    $table = new Table('index_length');
    $table->addColumn('path', 'text');
    $table->addIndex(['path'], null, [], ['lengths' => [768]]);

    $this->schemaManager->dropAndCreateTable($table);

    $indexes = $this->schemaManager->listTableIndexes('index_length');
    self::assertCount(1, $indexes);

    $index = array_shift($indexes);
    self::assertEquals(['path'], $index->getColumns());
    self::assertEquals([768], $index->getOption('lengths'));
}

It also passes with your syntax:

public function testIndexLength()
{
    $this->schemaManager->tryMethod('dropTable', 'index_length');
    $this->connection->exec('CREATE TABLE index_length (path text NULL)');
    $this->connection->exec('CREATE INDEX path ON index_length (path(768))');

    $indexes = $this->schemaManager->listTableIndexes('index_length');
    self::assertCount(1, $indexes);

    $index = array_shift($indexes);
    self::assertEquals(['path'], $index->getColumns());
    self::assertEquals([768], $index->getOption('lengths'));
}

In both cases, the index is introspected correctly as having one column path and its indexed length 768. Could you please provide a failing case similar to the above? Right now I don't see where the problem is.

[leofeyer]

It should be the opposite. path(768) is not a column name. It's an SQL expression representing an index column (by name) and its length.

Either way, Schema::getMigrateToSql() is broken now in version 2.9.1 due to the column identifier being different from the one returned by $schemaManager->createSchema().

@beberlei @Ocramius Maybe you can take a look at this issue as well? I have already provided code samples and screenshots in https://github.com/doctrine/dbal/issues/3409#issuecomment-447753599 and I don't know how to better explain it. It seems we are stuck here. 🙈

[Ocramius]

@leofeyer I haven't got the slightest clue: @morozov already provided a test scenario, it is up to you to provide a failing test case now.

[leofeyer]

Ok, will do tomorrow.

[leofeyer]

So here is the failing test:

public function testUpdatesIndexLength()
{
    $this->connection->exec('CREATE TABLE index_length (path text NULL)');
    $this->connection->exec('CREATE INDEX path ON index_length (path(333))');

    $table = new Table('index_length');
    $table->addColumn('path', 'text', ['notnull' => false]);
    $table->addIndex(['path'], 'path', [], ['lengths' => [768]]);

    $fromSchema = $this->connection->getSchemaManager()->createSchema();
    $toSchema = new Schema([$table]);

    $diff = $fromSchema->getMigrateToSql($toSchema, $this->connection->getDatabasePlatform());

    self::assertContains('ALTER TABLE index_length DROP INDEX path, ADD INDEX path (path(768))', $diff);
}

[Ocramius]

Can you please send that in a PR? That would basically restart the issue from a fresh/solid ground 👍

[leofeyer]

Sure, if you tell me in which class the test belongs.

[morozov]

@leofeyer thank you for the test. I'll figure out first what exactly is broken and then we'll see where the test belongs.