Allow to disable SQLite schema emulation in SqliteSchemaManager #6337
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Can we cover this with a test please? |
|
Is your request for a one functional test into |
|
@derrabus may I understand your wishes better? |
|
A test please. |
6245c62 to
49f86b9
Compare
1c6cc29 to
150fa23
Compare
|
@derrabus may I ask you for a review? |
|
Sure. It's on my todo list, but I don't have much time to spend on open source work at the moment. |
150fa23 to
19b8299
Compare
|
I don't quite understand this test, tbh. You're testing with a dot in a table name that SQLite to my astonishment seems to accept when quoted. But that doesn't have much to do with schema emulation. Shouldn't the test work with a table from an actual secondary schema? |
|
The fixed methods accept database name using 1st parameter, the table name is accepted as 2nd parameter, thus the 2nd parameter can be unquoted and dot must be preserved (it cannot contain schema). As the fixed methods cannot be reached easily from public API as table with schema and dot is not fully supported yet, the test is coded to call the fixed protected methods. |
So, what does this PR fix then? An implementation detail? |
|
The schema manager can be extended by user classes and the protected methods must behave as expected. This PR fixes the behaviour and it is tested with functional test. For more details, see the #4804 and #5517 PRs, ie. the schema emulation is deprecated for 3.x and removed in 4.x. Can we agree on this? Is there anything else I should do? |
913f249 to
ee4cf6f
Compare
ee4cf6f to
b480519
Compare
|
@derrabus the test was adjusted, can I have your final review? |
|
I'm touring conferences this week. Please ping me again if I haven't reviewed the PR by the end of next week. |
b480519 to
a13e983
Compare
a13e983 to
77b6081
Compare
|
@derrabus rebased and pinging you as requested, I hope you enjoyed the conferences! |
@derrabus pinging you again :) |
|
@derrabus pinging you again |
|
Please squash your commits. Once this is merged, nobody needs to know you wrote non functional tests at some point. While squashing, please improve your commit message according to the contributing guide. Right now it says that you "fix something unwanted"… unwanted stuff is avoided, not fixed. More importantly, you should state why you're doing this. Reading the test, I can see that you need to have a custom schema manager to even hit the bug, so this warrants an explanation, it's not obvious this only ever causes issue when using a custom schema manager |
| static fn (array $row) => [$row['table_name'], $row['name']], | ||
| $customSqliteSchemaManager->selectTableColumnsWithSchema(), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
To reviewers: only this assertion fails without the bugfix… it fails like so:
1) Doctrine\DBAL\Tests\Functional\Schema\SqliteSchemaManagerTest::testListTableNoSchemaEmulation
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 (
- 0 => Array &1 (
- 0 => 'list_table_no_schema_emulation.test'
- 1 => 'id'
- )
- 1 => Array &2 (
- 0 => 'list_table_no_schema_emulation.test'
- 1 => 'parent_id'
- )
-)
+Array &0 ()
The schema emulation is a deprecated feature. The SqlitePlatform does support disabling the schema emulation already, the SqliteSchemaManager should support it too to ease the migration path between 3.x and 4.x. The test requires custom SqliteSchemaManager because the current SqliteSchemaManager impl. does not support tables with schema yet.
77b6081 to
7d710c9
Compare
|
Squashed and repushed. |
There was a problem hiding this comment.
Great commit message, please write them like this from now on.
A concern I have is that this change is here to support a feature that can only be enabled by overriding the schema manager, but why not… there is not much harm IMO since it will be gone in 4.0 (right?).
|
@derrabus @SenseException please review |
|
Thanks @mvorisek ! |
Summary
Missed in #4804 and fix related #5517.
When merging up (into 4.x), simply remove the
str_replace.