-
-
Notifications
You must be signed in to change notification settings - Fork 48
Mysql bulk update/insert dialect support #197
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
Conversation
|
amazing @mleczakm ! Could you please take a look at failing jobs? |
|
oh I know what is wrong, you need to add MySQL to the testsuite workflow file as well, it's not using docker-compose https://github.com/flow-php/flow/blob/1.x/.github/workflows/test-suite.yml#L74 |
This comment was marked as outdated.
This comment was marked as outdated.
src/lib/doctrine-dbal-bulk/src/Flow/Doctrine/Bulk/Dialect/MySQLDialect.php
Outdated
Show resolved
Hide resolved
src/lib/doctrine-dbal-bulk/src/Flow/Doctrine/Bulk/Dialect/MySQLDialect.php
Outdated
Show resolved
Hide resolved
|
some tests are failing now :( |
This comment was marked as resolved.
This comment was marked as resolved.
|
not sure if I fully get the tests part, but let me explain my reasoning behind
The reason why I decide to go with And about the tests, you don't need to replicate all PostgreSQL tests, it would be enough to just test all options supported by MySQL. As long as |
src/lib/doctrine-dbal-bulk/src/Flow/Doctrine/Bulk/Dialect/MySQLDialect.php
Outdated
Show resolved
Hide resolved
…rt with fails allowed
| */ | ||
| private function updateAllColumns(Columns $columns) : string | ||
| { | ||
| return \implode( |
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 looks to me like a logic that could be moved to Columns object
| * @return string | ||
| */ | ||
| private function updateSelectedColumns(array $updateColumns, Columns $columns) : string | ||
| { |
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 think I would use the same approach as above except that I would first filter Columns to only have updateColumns
|
Thanks @mleczakm! I left two small comments but they are something that can be improved later, nothing critical, mostly code organization. |
Change Log
Added
Fixed
Changed
Removed
Deprecated
Security
Description
Support for mysql db for loaders.