Implement the column collation for Mysql#2551
Implement the column collation for Mysql#2551mikeSimonson wants to merge 2 commits intodoctrine:masterfrom
Conversation
|
@mikeSimonson should this fix (since it fixes #2538, if I understand it correctly) come with a test case then? |
|
@Ocramius Do you mean that the test should be removed or that it needs a higher level test case ? |
|
@mikeSimonson higher level test case reflecting #2538 |
|
@Ocramius What is the right approach here for the test ? |
Ocramius
left a comment
There was a problem hiding this comment.
One last change probably needed: verifying that the comparator will pick up the collation (unless that is already covered)
| /** | ||
| * https://github.com/doctrine/dbal/issues/2538 | ||
| */ | ||
| public function testFixGithub2538_autoincrement() |
There was a problem hiding this comment.
This test seems unrelated to the changes, or am I missing something?
There was a problem hiding this comment.
Please use a method name describing the issue being covered. Also, use @group #2538 for labeling it
| { | ||
| $this->markTestSkipped(); | ||
|
|
||
| $tableName = "public.foo"; |
| /** | ||
| * https://github.com/doctrine/dbal/issues/2538 | ||
| */ | ||
| public function testFixGithub2538Collation() |
There was a problem hiding this comment.
Please use a method name describing the issue being covered. Also, use @group #2538 for labeling it
| ); | ||
| } | ||
|
|
||
| public function testFixGithub2538Collation() |
There was a problem hiding this comment.
Please use a method name describing the issue being covered. Also, use @group #2538 for labeling it
|
What is the progress with this one? And maybe there should be column charset implemented as well? |
|
I don't see the point of this PR. Column collations are working in mysql already in general.
The requested change seems useless. MysqlPlatform already implements and the rest is handled by the default implementation of AbstractPlatform. There are two bugs with column collations:
|
Tests seem to say otherwise, but feel free to provide more tests or comment on the ones provided in this PR. |
Related to #2538