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

Use unicode rather than general encoding #9246

Closed
wants to merge 1 commit into from
Closed

Conversation

wilsonge
Copy link
Contributor

Summary of Changes

This changes the utf8mb4 general encoding to uft8mb4 unicode encoding - it is generally encouraged over general encoding because it sorts better in foreign languages (see http://stackoverflow.com/a/766996 for example)

Testing Instructions

This shouldn't affect either installation or upgrading from a previous version of Joomla.

@richard67
Copy link
Member

Ah, just opened #9245 for the same issue. My code is already ok.

Question: What about unit tests?

I found "utf8_general_ci" in following files:

tests\unit\suites\database\driver\mysql\JDatabaseDriverMysqlTest.php: 'utf8_general_ci',
tests\unit\suites\database\driver\mysql\JDatabaseExporterMysqlTest.php: 'Collation' => 'utf8_general_ci',
tests\unit\suites\database\driver\mysql\JDatabaseImporterMysqlTest.php: 'Collation' => 'utf8_general_ci',
tests\unit\suites\database\driver\mysqli\JDatabaseDriverMysqliTest.php: $this->equalTo('utf8_general_ci'),
tests\unit\suites\database\driver\mysqli\JDatabaseExporterMysqliTest.php: 'Collation' => 'utf8_general_ci',
tests\unit\suites\database\driver\mysqli\JDatabaseImporterMysqliTest.php: 'Collation' => 'utf8_general_ci',
tests\unit\suites\database\driver\pdomysql\JDatabaseDriverPdomysqlTest.php: $this->equalTo('utf8_general_ci'),
tests\unit\suites\database\driver\pdomysql\JDatabaseExporterPdomysqlTest.php: 'Collation' => 'utf8_general_ci',
tests\unit\suites\database\driver\pdomysql\JDatabaseImporterPdomysqlTest.php: 'Collation' => 'utf8_general_ci',


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9246.

@richard67
Copy link
Member

And if we use yours, not forget to change the create table statement I have added to the installer's database model, and this one is still created as utf8_ ..., not as utf8mb4, same as my new update sql, see my files in PR #9245

@wilsonge wilsonge closed this Feb 28, 2016
@wilsonge wilsonge deleted the sql_unicode branch February 28, 2016 20:17
@wilsonge
Copy link
Contributor Author

We'll use yours

@richard67
Copy link
Member

And what shall we do with the unit tests?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9246.

@richard67
Copy link
Member

Does the database used for those support utf8mb4?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9246.

@wilsonge
Copy link
Contributor Author

We use SQLite for the unit tests. This Drupal issue suggests that it is supported https://www.drupal.org/node/1314214

@richard67
Copy link
Member

Does it mean we should change the unit tests, too, to either utf8mb4_unicode_ci?

And if so, in this or in a new PR?

Or leave them untouched because they just check what they created before themselves, e.g. test tables, so what Joomla! Core tables normally have is not relevant for them?

I did not look into them yet and also not know anything about Joomla!'s unit tests.

@mbabker
Copy link
Contributor

mbabker commented Feb 28, 2016

The MySQL servers on Jenkins and Travis should be running MySQL 5.5 so for
MySQL specific tests it should be safe to change. Basically only the
database driver tests should still be using MySQL connections. Everything
else in the unit test suite should be using a SQLite in-memory database
that's set up when the tests start and goes away when it ends.

Presumably, the tests should validate both utf8 and utf8mb4 configurations
at some point.

On Sunday, February 28, 2016, Richard Fath notifications@github.com wrote:

Does it mean we should change the unit tests, too, to either utf8mb4_
_unicode__ci?

And if so, in this or in a new PR?

Or leave them untouched because they just check what they created before
themselves, e.g. test tables, so what Joomla! Core tables normally have is
not relevant for them?

I did not look into them yet and also not know anything about Joomla!'s
unit tests.


Reply to this email directly or view it on GitHub
#9246 (comment).

@wilsonge
Copy link
Contributor Author

The problem is the filter runs based on whether the database supports utf8mb4. Not whether they are actually set up with that configuration or not... :(

@richard67
Copy link
Member

Oh my god ... gets too complicated for me ... have no knowledge about unit testing of Joomla! ... I think I am out here ... all I hope is that our recent fixes for unicode collations and utf8mb4 not break any unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants