fix: add missing drop schema when dropping database #6576
Conversation
| if ($namespace === 'public') { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
not sure what to do with the "main" public schema, I don't know if this is specific to PG or not.
In PG, this schema is automatically created when the db is created, so I don't think we should drop it with doctrine:schema:drop
There was a problem hiding this comment.
Good question. Schemas can have different permissions in newer PostgreSQL versions and removing the defult public would also remove those with it, as it would with the custom schemas. I guess I'd expect it to be removed too when I want to drop schemas. 🤔
There was a problem hiding this comment.
ok, I've removed the exception for the public schema
There was a problem hiding this comment.
Since I have removed the exception for public schema, I got an error in PostgreSQLSchemaManagerTest::testDropWithAutoincrement():
Doctrine\DBAL\Driver\PgSQL\Exception: cannot drop schema public because other objects depend on it
in PostgreSQLSchemaManagerTest.php:364
And given this comment, I think we will have problems dropping schemas which contains views, and schema "that need to be quoted".
I'm wondering if we can mitigate these problems:
- schema which contains view: shouldn't we drop views as well in
DropSchemaObjectsSQLBuilder(because we don't refer to views in\Doctrine\DBAL\Schema\Schema, this seems a little bit complicated, but we can pass a list of the views toDropSchemaObjectsSQLBuilder) - schemas that need to be quoted: in
AbstractAsset::getQuotedName(), maybe we can also check if the asset's name starts with an integer? Or if this rule is dependent of the current platform, we could add a method for this inAbstractPlatform?
WDYT?
There was a problem hiding this comment.
hi @SenseException @greg0ire , any chance to have some insights about this please? thanks! 🙏 🙏
There was a problem hiding this comment.
It looks like in order to correctly drop the schemas containing views, we need to implement introspection of the views in the schema.
The following is not by design, it was just never implemented.
dbal/tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
Lines 341 to 343 in c91166a
There was a problem hiding this comment.
dbal/tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
Lines 339 to 340 in c91166a
I don't remember what exactly the problem is. Quoting names that start with a digit is a workaround. I don't think it's a strict requirement if your code will be unable to drop such schemas as long as it doesn't introduce new problems. The issues with quoted names need to be fixed separately.
There was a problem hiding this comment.
yes, I was planning to fix both problems in separate PRs, thanks for your hints!
nikophil
force-pushed
the
fix/add-missing-drop-schema
branch
from
841f800 to
1944fff
Compare
nikophil
force-pushed
the
fix/add-missing-drop-schema
branch
2 times, most recently
from
67cb9fe to
bc07845
Compare
nikophil
force-pushed
the
fix/add-missing-drop-schema
branch
from
bc07845 to
ad2a79f
Compare
|
Hi @SenseException, thanks for your review
super weird, the places where codcov has added annotations are covered by the new test 🤔 If I add dumps in the code, when running this command, I can see it is covered |
|
I believe this is caused by codecov/codecov-action#1580 I'm assuming that @SenseException was refering to a passing build, because the codecov upload job does not even run when one of the github job fails like it's the case right now. |
| @@ -366,6 +366,25 @@ public function testDropWithAutoincrement(): void | |||
| self::assertFalse($schemaManager->tablesExist(['test_autoincrement'])); | |||
| } | |||
|
|
|||
| public function testDropWithSchema(): void | |||
There was a problem hiding this comment.
Since the code affects all platforms, the test should cover all platforms as well.
There was a problem hiding this comment.
hi @morozov
I'm OK with this, but I'm stucked because I don't know how to resolve this problem
Summary
fixes doctrine/orm#11701 which was actually a dbal issue.
When we'll be OK with the implementation, I'll provide another PR to fix this in dbal 3