Make SchemaTool use the new DBAL API #12113
Conversation
|
I'll look into the PHPStan failures later. |
|
@morozov The new schema API distinguished between "quoted" and "unquoted" identifiers (e.g. index or column names). I'm using "unquoted" here in the sense of "if this needs to be quoted, the I did this already, so don't automatically quote this for me". Please advice if that's correct. For ORM 4, we should probably find a way to leverage the quoted identifiers properly. |
derrabus
force-pushed
the
improvement/schema-tool-new-api
branch
from
23ae208 to
0142977
Compare
Answering my own question, my assumption is not correct, looking at the test failures I get with DBAL 5. I guess, I'll dig a bit more into this matter. I think, our |
|
|
||
| $table->addPrimaryKeyConstraint(new PrimaryKeyConstraint(null, $primaryKeyColumnNames, true)); | ||
| } else { | ||
| /** @param string[] $primaryKeyColumns */ |
There was a problem hiding this comment.
Should we use the same advanced types you used somewhere else in this PR? Maybe it will address the baselined issue.
| /** @param string[] $primaryKeyColumns */ | |
| /** @param non-empty-list<non-empty-string> $primaryKeyColumns */ |
|
|
||
| - | ||
| message: '#^Parameter \#2 \$columns of class Doctrine\\DBAL\\Schema\\Index constructor expects non\-empty\-list\<string\>, list\<string\> given\.$#' | ||
| message: '#^Parameter \#1 \$firstColumnName of method Doctrine\\DBAL\\Schema\\PrimaryKeyConstraintEditor\:\:setUnquotedColumnNames\(\) expects non\-empty\-string, string given\.$#' |
There was a problem hiding this comment.
Now that we have identifiers at our disposal, I think we should not add new issues to the baseline, and instead resort to e.g. /** @phpstan-ignore argument.type (reason why we ignore this) */
There was a problem hiding this comment.
Yes, I'll try to do that when I rework the PR.
I haven't had a chance to look at the code, but this not how this API is intended to be used. First, from the DBAL 5's standpoint, all identifiers (whether they are quoted or not) will end up surrounded with quotes in SQL – this is how we eliminate the need to maintain dictionaries of keywords. Quoting identifiers (as in surrounding them with quotes) is not the responsibility of the user, and this is not portable anyways. Second, on those platforms that don't respect SQL'92 (SQLite, MySQL and SQL Server), whether the identifier is quoted or not doesn't make any difference. Whether the identifier is quoted or not, only determines the resulting case of the identifier when it's rendered to SQL for SQL'92 compliant databases (Postgres, Oracle, Db2):
The bottom line is you need to use quoted identifiers only if your application uses identifiers in a case that isn't natural for supported platforms or it supports platforms with non-matching natural case of the identifiers. For instance:
|
|
|
||
| return $index->getColumns(); | ||
| return Index::editor() | ||
| ->setUnquotedName($indexName) |
There was a problem hiding this comment.
I would double-check how this code behaves if the index name contains quotes (e.g. "123_index"). Previously, Index would parse it resulting in quoted name 123_index. In the new version, it will result in an unquoted name "123_index" (i.e. quotes will be included in the name, not part of the syntax).
If that's the case, you'll need to parse the string with UnqualifiedNameParser and use setName() instead of setUnquotedName().
This is my attempt at getting our
SchemaToolready for DBAL 5. Not all tests are green with DBAL 5 yet, but we're getting there.