Move schema split for SQLite CREATE INDEX only #6352
Conversation
mvorisek
force-pushed
the
fix_primary_index_schema_split
branch
4 times, most recently
from
c4a7206
to
0d1617e
Compare
|
There is no functional test included. We're making assertions purely on the generated SQL. Thus, the test does not really reproduce a bug that's fixed with this change. Furthermore, you're testing an implementation detail: how the table name is passed from If I'm not mistaken, this change is supposed to fix a problem regarding the creation of primary keys on tables outside of the |
| $indexDef = new Index('i', ['a', 'b']); | ||
| self::assertSame( | ||
| 'CREATE INDEX main.i ON mytable (a, b)', | ||
| $this->platform->getCreateIndexSQL($indexDef, 'main.mytable'), | ||
| ); |
There was a problem hiding this comment.
This method contains two test scenarios in a single test. Furthermore, this first scenario is green without your changes. It does not really cover anything.
There was a problem hiding this comment.
I have split the test into two.
The first test tests if expected SQL is generated when a table name with schema is given.
The second test tests if unmodified table/column is passed to SqlitePlatform::getCreatePrimaryKeySQL().
mvorisek
force-pushed
the
fix_primary_index_schema_split
branch
from
0d1617e
to
d24cf43
Compare
mvorisek
force-pushed
the
fix_primary_index_schema_split
branch
from
d24cf43
to
20fbd75
Compare
|
This comment hasn't been addressed as far as I can tell: #6352 (comment) |
|
Function test (ie. test with DB) cannot be made as https://github.com/doctrine/dbal/blob/3.8.4/src/Platforms/SqlitePlatform.php#L995 always throws. Thus the test is coded to test the behaviour using overrided method. |
|
If the scenario is an overridden platform, can't we run a functional test with an overridden platform? |
|
Adding/altering primary key is not supported by SQLite using one SQL statement, thus I belive the current test cannot be coded better. In anycase, we need to override the platform method to test this. This is what the current test does. It does not change the behaviour in sense of any currently generated SQL, but fixes what is passed to a public overridable method. |
|
@greg0ire @SenseException can I please have your review here? |
|
@greg0ire the moved part is in the code solely to create right I hope it is clear the code needs to be moved and I have asserted that with a test which fail otherwise. If the impl. is fine, let's merge it please. |
* 3.8.x: Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection" CI: Update MariaDB versions (doctrine#6426) CI MariaDB: add 11.4, remove 11.0 (doctrine#6432) Fix typo in the portability documentation (doctrine#6430)
* 3.8.x: Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection"
* 4.0.x: PHPUnit 10.5.21 (doctrine#6447) Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection"
* 4.1.x: (25 commits) Simplify signature of fetchTableOptionsByTable Add MariaDb1010Platform for fetchTableOptionsByTable PHPUnit 10.5.21 (doctrine#6447) Move schema split for SQLite CREATE INDEX only (doctrine#6352) PHPStan 1.11.5 (doctrine#6446) Default to distinct union queries (doctrine#6439) Revert "Merge pull request doctrine#6413 from achterin/bugfix/foreign_key_name_change_detection" Add `QueryBuilder` support for `UNION` clause (doctrine#6369) CI: Update MariaDB versions (doctrine#6426) CI MariaDB: add 11.4, remove 11.0 (doctrine#6432) Display warnings when running PHPUnit in CI (doctrine#6431) Fix typo in the portability documentation (doctrine#6430) Fix MariaDB fetching of default table character-set (doctrine#6361) (doctrine#6425) Fix the portability documentation (doctrine#6429) Update tests/Platforms/AbstractPlatformTestCase.php Update tests/Platforms/AbstractPlatformTestCase.php add test Fix: Skip type comparison if disableTypeComments is true Remove redundant variable (doctrine#6326) Fix test names to reflect their actual purpose ...
Summary
SqlitePlatform::getCreatePrimaryKeySQLmethod must receive original table name, the split is intended for SQLiteCREATE INDEXstatement only.