Skip to content
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

Add Mysql per-column charset support #881

Closed
wants to merge 1 commit into from

Conversation

mRoca
Copy link
Contributor

@mRoca mRoca commented Jul 15, 2015

Actually with MySQL we can define a collation per column, but the charset seems not supported, whereas specified in the documentation : http://doctrine-dbal.readthedocs.org/en/latest/reference/schema-representation.html#vendor-specific-options

I've added the CHARACTER SET *** sql declation for the MySQL platform.

Use case with Doctrine/ORM :

/**
 * @Column(name="foo", type="text", options={"collation"="utf8mb4_unicode_ci", "charset"="utf8mb4"})
 **/

Linked PR : #850

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1264

We use Jira to track the state of pull requests and the versions they got
included in.

@fgueguen
Copy link

👍

- **collate** (string): The collation to use for the column. Currently only supported on
SQL Server.
- **collation** (string): The collation to use for the column. Supported by MySQL, PostgreSQL,
Sqlite, SQL Server and Drizzle.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs fix is correct, but is unrelated to this PR. Please PR this separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc fix removed from this PR

$columns = $this->_sm->listTableColumns('test_charset');

$this->assertArrayNotHasKey('charset', $columns['id']->getPlatformOptions());
$this->assertEquals('latin1', $columns['text']->getPlatformOption('charset'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong. The charset is set implicit here through the table's default charset. However why is it latin1 here? As far as I can see, we set it implicitly to utf8 here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, somehow didn't see that you're using custom table collation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've set the default table charset to latin1 : https://github.com/doctrine/dbal/pull/881/files#diff-921db63349be598a376afb1584ccc8b5R206, the test is verifying the default charset behavior.

@greydnls
Copy link

👍

@@ -199,6 +199,10 @@ protected function _getPortableTableColumnDefinition($tableColumn)

$column = new Column($tableColumn['field'], Type::getType($type), $options);

if (isset($tableColumn['characterset'])) {
$column->setPlatformOption('charset', $tableColumn['characterset']);
Copy link

@dolmen dolmen Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the option named characterset instead of charset?

Hint: if I'm asking this, it means that at least a comment in the code is needed.

@mRoca
Copy link
Contributor Author

mRoca commented Nov 6, 2019

Closing this PR, as @altertable copied its content in #3418

@mRoca mRoca closed this Nov 6, 2019
@mRoca mRoca deleted the feature/mysql-charset branch November 6, 2019 10:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants