-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix length type in MySQLSchemaManager with ATTR_STRINGIFY_FETCHES enabled
#7028
New issue
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
Fix length type in MySQLSchemaManager with ATTR_STRINGIFY_FETCHES enabled
#7028
Conversation
|
removed test for varchar type as there seems to be no type mapping for that |
|
i'm having trouble to add a pure reproducer in the testing setup here, see #7029 However I verified the fix locally inside shopware, see e.g. this workflow run for the issue: https://github.com/shopware/shopware/actions/runs/16268473345/job/45929737083?pr=11222 |
I don't know much about Shopware's internals, but I'd like to understand why you're having this problem. Is Shopware maybe operating DBAL with the PDO::ATTR_STRINGIFY_FETCHES option enabled? |
|
@derrabus yes exactly we are still relying on that option and it seems that the issue only shows with that option, so far I was unable to get that option into the testsuite here and for now stopped work on the reproducer, will happily continue with that when you provide the general direction e.g. if we even want to have a complete unit test run with that option? |
|
Maybe we should add a CI job that runs the whole test suite with that option enabled. This option could be a potential footgun for every operation DBAL itself performs on the database. 🤔 |
|
I can confirm the (important) bug. The error can be found on the phpstan-doctrine CI https://github.com/phpstan/phpstan-doctrine/actions/runs/16344462764/job/46174534513 |
|
@VincentLanglet The failure in PHPStan's CI that you're linking to shows a test failure that happens with Postgres. This issue discusses a bug inside the MySQL schema manager. Are you sure you provided the correct link? |
|
There is one failure, but also 950 errors like I thought this was related to the fact |
You're right, I missed those. My bad. Yes, this seems to be the same error. Same question for you then: #7028 (comment) |
I dunno a lot about the logic behind the phpstan-doctrine tests, but the failure happen on some code with the map So yes @derrabus indeed the failure occurs because there is a testcase with the option |
|
i got the issue reproduced in the doctrine test suite by adding a job with stringify fetches, you can see it failing with the same error here: #7029 I'll cherry-pick the new testsuite to this PR and will make it green Anything else you need before this is ready to merge? |
| */ | ||
| private function prepareExpectedRows(array $rows): array | ||
| { | ||
| if (! TestUtil::isDriverOneOf('ibm_db2', 'pdo_oci', 'pdo_sqlsrv', 'oci8')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took quite some time for me to understand all the negated ifs, IMHO keeping the flow as straight forward as possible makes it easier to understand what is going on
| if (! TestUtil::isDriverOneOf('ibm_db2')) { | ||
| if ( | ||
| TestUtil::isDriverOneOf('pdo_oci', 'pdo_sqlsrv', 'oci8') | ||
| || (TestUtil::getConnectionParams()['driverOptions'][PDO::ATTR_STRINGIFY_FETCHES] ?? false) === true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally did not check for pdo_mysqldriver as this would allow us to easily add the same job for other DB engines, if the ATTR_STRINGIFY_FETCHES option is relevant as well
|
@derrabus tests and CI is green now, I consider this finished so far, happy to get your review ;) |
derrabus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thank you! Would you be interested in adding a CI job for the other PDO drivers as well?
MySQLSchemaManager with ATTR_STRINGIFY_FETCHES enabled
|
Thanks for quickly merging this! 🎉 PR for more jobs with that option is here: #7052 |
<!-- Fill in the relevant information below to help triage your pull request. --> | Q | A |------------- | ----------- | Type | improvement #### Summary Add more unit test jobs that use the PDO::STRINGIFY_FETCHES option, as discussed in this PR: #7028 (review) I didn't add a job for `pdo_oci` and `pdo_sqlsrv` as for those drivers the result in `QueryBuilderTest` was already stringified, therefore I assume those drivers always stringify the fetches, therefore a separate job is not needed
* 4.4.x: Document the JSON_OBJECT type (doctrine#7054) [RFC] Introduce Types::JSON_OBJECT (doctrine#7053) feat: add more pdo jobs with stringify fetches (doctrine#7052) Fix case sensitivity in SQLite column types (doctrine#7050) Fix length type in `MySQLSchemaManager` with `ATTR_STRINGIFY_FETCHES` enabled (doctrine#7028) Fix Symfony 8 compatibility issues (doctrine#7009) Quote MySQL constraint names for foreign keys
* 4.4.x: Document the JSON_OBJECT type (doctrine#7054) [RFC] Introduce Types::JSON_OBJECT (doctrine#7053) feat: add more pdo jobs with stringify fetches (doctrine#7052) Fix case sensitivity in SQLite column types (doctrine#7050) Fix length type in `MySQLSchemaManager` with `ATTR_STRINGIFY_FETCHES` enabled (doctrine#7028) Fix Symfony 8 compatibility issues (doctrine#7009) Quote MySQL constraint names for foreign keys
Fixes #7027
Summary
The length var was type string, which broke on the constructor of
Column