Prep work for enabling support for foreign keys on SQLite #9799
Conversation
| @@ -857,12 +856,9 @@ public function dropDatabase() | |||
| */ | |||
| public function getDropDatabaseSQL() | |||
| { | |||
| $schema = $this->schemaManager->createSchema(); | |||
|
|
|||
| $visitor = new DropSchemaSqlCollector($this->platform); | |||
There was a problem hiding this comment.
DropSchemaSqlCollector will be deprecated in favor of another class. It's better if the ORM doesn't depend on such low-level implementation details at all and uses the schema manager for managing schema.
| $this->_em->getClassMetadata(CompanyManager::class), | ||
| ] | ||
| ); | ||
| self::assertCount(4, $sql); |
There was a problem hiding this comment.
The SQL that the DBAL uses for managing schema is an implementation detail that may and will change.
| @@ -41,6 +42,11 @@ public function tearDown(): void | |||
| $sm->dropTable($platform->quoteIdentifier('TREE_INDEX')); | |||
| $sm->dropTable($platform->quoteIdentifier('INDEX')); | |||
| $sm->dropTable($platform->quoteIdentifier('LIKE')); | |||
|
|
|||
| if ($platform instanceof PostgreSQLPlatform) { | |||
There was a problem hiding this comment.
The problem with this code is that the test suite ignores the exceptions thrown by the schema tool:
orm/tests/Doctrine/Tests/OrmFunctionalTestCase.php
Lines 348 to 356 in 4f1072e
...and relies on the fact that CreateSchemaSqlCollector creates all tables before all sequences.
Without this change, if each sequence is created after its corresponding table, for each test except for the first, the setUp() will fail to create the first sequence (because it already exists) and thus will not create all three tables.
We could implement a dropSchemaForModels() method similar to createSchemaForModels() and use it instead but on the DBAL versions prior to 3.4 it would use DropSchemaSqlCollector internally which attempts to drop foreign keys before dropping the table, even if it's not necessary, but SQLite doesn't support that. The smallest of all evils, for now, seems to work around it.
| @@ -57,6 +57,11 @@ public function testNotInSync(): void | |||
|
|
|||
| public function testNotInSyncVerbose(): void | |||
| { | |||
| $schemaManager = $this->createSchemaManager(); | |||
| if ($schemaManager->tablesExist('cache_login')) { | |||
There was a problem hiding this comment.
This is also a mess. This test will fail if SQLite starts reporting that it supports foreign keys and if some other tests are executed before it (e.g. DDC2943Test). The cache_login table will already exist at this point and won't be part of the diff.
The test currently passes because even by the time when it's executed, the cache_login table already exists, its foreign key is not introspected, so the schema diff always contains a missing foreign key.
The proper solution here would be instead of deleting data from all tables in the functional test teardown, to drop those tables but the hard-coded list of tables is inconsistent with the models, so reworking it requires additional effort.
No description provided.