Dbal: Symfony CI red after "[GH-1204] Add full support for foreign key constraints in SQLite Platform and Schema"

[nicolas-grekas]

Commit https://github.com/doctrine/dbal/commit/85a983c3f8d6447c4441832477def1c28d57bee9 broke the CI of Symfony

see eg https://travis-ci.org/github/symfony/symfony/jobs/683266277

./phpunit src/Symfony/Bridge/Doctrine on any branch is the reproducer for now.

[greg0ire]

This commit was in 2.10.x and only there yesterday, I remember merging it up in all other branches since then:

• https://github.com/doctrine/dbal/pull/3985
• https://github.com/doctrine/dbal/pull/3987
• https://github.com/doctrine/dbal/pull/3988

EDIT: but since you probably force to <= 2.10.2 this is probably irrelevant :thinking:

[greg0ire]

I reproduce the issue, and I see 2 of them:

PDOException: SQLSTATE[HY000]: General error: 1 no such table: SingleIntIdEntity

/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:64
/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Statement.php:71
/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:861
/home/greg/dev/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php:273
/home/greg/dev/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:1087
/home/greg/dev/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:400
/home/greg/dev/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:368
/home/greg/dev/symfony/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php:114
/home/greg/dev/symfony/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php:138

and this one, which is less confusing:

Doctrine\DBAL\DBALException: Sqlite platform does not support alter foreign key, the table must be fully recreated using getAlterTableSQL.

/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php:797
/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Visitor/CreateSchemaSqlCollector.php:62
/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Table.php:806
/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Schema.php:454
/home/greg/dev/symfony/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/Schema.php:400
/home/greg/dev/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/SchemaTool.php:116
/home/greg/dev/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/SchemaTool.php:92
/home/greg/dev/symfony/src/Symfony/Bridge/Doctrine/Tests/Validator/Constraints/UniqueEntityValidatorTest.php:159
/home/greg/dev/symfony/src/Symfony/Bridge/Doctrine/Tests/Validator/Constraints/UniqueEntityValidatorTest.php:75

[greg0ire]

I have found a solution but I don't understand why it works.
In this line: https://github.com/doctrine/dbal/blob/4c03ed81471c62f178581eb325339a3c34f3b71a/lib/Doctrine/DBAL/Schema/Visitor/CreateSchemaSqlCollector.php#L56-L58

if I s/supportsForeignKeyConstraints/supportsCreateDropForeignKeyConstraints, the test suite passes.

[jrushlow]

We are getting the same Sqlite platform does not support alter foreign key, the table must be fully recreated using getAlterTableSql CI Results https://github.com/SymfonyCasts/reset-password-bundle/runs/646695175

Below is a snippet which triggers the issue.

private function configureDatabase(): void
    {
        $metaData = $this->manager->getMetadataFactory();

        $tool = new SchemaTool($this->manager);
        $tool->dropDatabase();
        $tool->createSchema($metaData->getAllMetadata());
    }

[greg0ire]

Great, you can try the above and report back… meanwhile, I think I'm starting to understand: acceptForeignKey is not supposed to attempt to create the FOREIGN KEY for Sqlite, because it is supposed to do so when creating the table and only there, not afterwards.

[greg0ire]

Now, let's see how we could write a test for this. I think attempting to create a table with a foreign key might be enough.

[jrushlow]

We are not doing anything fancy over in the reset bundle. But this https://github.com/SymfonyCasts/reset-password-bundle/blob/96391ea830a24bc2dcfc4437edd6648cc55ebcfd/tests/Fixtures/Entity/ResetPasswordTestFixtureRequest.php#L47 is triggers the error when we create the schema off the metadata.

[greg0ire]

I tried this but I don't reproduce the issue:

    public function testCreatingATableWithAForeignKey() : void
    {
        if (! extension_loaded('sqlite3')) {
            $this->markTestSkipped('This test requires the SQLite3 extension.');
        }

        $referencedTable = new Table('referenced');
        $referencedTable->addColumn('id', 'integer');

        $sm = $this->connection->getSchemaManager();
        $sm->createTable($referencedTable);

        $referencingTable = new Table('referencing');
        $referencingTable->addColumn('id', 'integer');
        $referencingTable->addColumn('referenced_id', 'integer');
        $referencingTable->addForeignKeyConstraint(
            $referencedTable,
            ['referenced_id'],
            ['id']
        );

        $sm->createTable($referencingTable);
    }

I'm probably not at the right abstraction level. Since it appears in the stack trace, I should probably use the schema tool rather than the schema manager.

EDIT: ah but the schema tool is in the ORM, I should probably use Schema::toSql() instead.

[greg0ire]

I'm reproducing the issue in a failing test:

    public function testCreatingATableWithAForeignKey() : void
    {
        $schema = new Schema\Schema();

        $referencedTable = $schema->createTable('referenced');
        $referencedTable->addColumn('id', 'integer');

        $referencingTable = $schema->createTable('referencing');
        $referencingTable->addColumn('id', 'integer');
        $referencingTable->addColumn('referenced_id', 'integer');
        $referencingTable->addForeignKeyConstraint(
            $referencedTable,
            ['referenced_id'],
            ['id']
        );

        $schema->toSql($this->connection->getDatabasePlatform());
    }

[jrushlow]

if I if (! $this->platform->s/supportsForeignKeyConstraints/supportsCreateDropForeignKeyConstraints()) { = Undefined property: Doctrine\DBAL\Platforms\SqlitePlatform::$s

Did I misunderstand your suggestion?

[greg0ire]

lol yeah pretty much. s/foo/bar is vim parlance that means replace "foo" with "bar". There is even a Slack command nowadays /s/foo/bar will replace foo by bar in your last chat. Anyway, I have a fix, I'm going to send a PR very soon.

[jrushlow]

HAHA duh! your fix works..

momma always said I should learn vim....

[greg0ire]

momma always said I should learn vim....

Try it for some time, that way you will be able to understand more jokes ;)