? Success

User tests: Successful: Unsuccessful:

avatar 810
810
5 Mar 2016

Test Instructions

Install Joomla. Then check in the database all tables are utf8_unicode_ci or ut8mb4_unicode_ci

avatar 810 810 - open - 5 Mar 2016
avatar richard67
richard67 - comment - 5 Mar 2016

Changing collations for both utf8mb4 and utf8 cases to _unicode_ci was agreed with @wilsonge and @aschkenasy

avatar 810
810 - comment - 5 Mar 2016

ok updated the code

avatar andrepereiradasilva
andrepereiradasilva - comment - 5 Mar 2016

@810 test instructions please

avatar 810
810 - comment - 5 Mar 2016

updated

avatar andrepereiradasilva
andrepereiradasilva - comment - 5 Mar 2016

the tables or the database default collation?
the tables where already utf8mb4_unicode_ci before this PR.

avatar richard67
richard67 - comment - 5 Mar 2016

@andrepereiradasilva I agree with your latest comment, Code looks ok now, but as far as I understand it, it will be only relevant if Joomla! is installed with the otpion to create the new database when it does not exist already, or am I wrong? The default collation of existing databases is not touched by Joomla! as far as I know up to now.

avatar 810
810 - comment - 5 Mar 2016

getAlterTableCharacterSet is only used on alterTableCharacterSet, so you should see no changes

avatar andrepereiradasilva
andrepereiradasilva - comment - 5 Mar 2016

since it's in the db main driver, i wonder if this could have impact on other databases .... like postgresql, for instance.

avatar richard67
richard67 - comment - 5 Mar 2016

@andrepereiradasilva That's what I asked myself, too, because I also wanted to make a PR for this file, but then not seen where it was used ... but it cannot have any impact on the other database kinds, otherwise this "utf8_general_ci" all the time would also have confused them, or not? The joomla.sql for postgresql and sqlazure not contain any character set stuff, so maybe it is in the main driver but it seems to have an effect only for mysql.

Because this confused me and was less urgent for me than other utf8mb4 problems to be fixed, I have it then up for the moment to understand it.

avatar richard67
richard67 - comment - 5 Mar 2016

@wilsonge Do you know what this mysql specific character and collation stuff is doing in this file handled with this PR here? Or do you know who could know?

avatar wilsonge
wilsonge - comment - 5 Mar 2016

I mean this is where shit starts to get hard :P getAlterTableCharacterSet (and related sub method) are new for 3.5 - so I'm guessing they don't work on postgres and mssql - I'll try and verify on my postgres testbed in a bit (or maybe @alikon can advise as the postgres guru). @nikosdion any thoughts as you put in these methods? Maybe we need to do a mysql check at the top of these two methods

For the other methods (for getting the create statement) again I think getCreateDatabaseQuery needs to have it's second parameter ($utf) as false on non-mysql databases (as the utf8mb4 collations will not work on postgres afaik)

avatar wilsonge wilsonge - change - 5 Mar 2016
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 5 Mar 2016
Labels Added: ?
avatar nikosdion
nikosdion - comment - 6 Mar 2016

Actually, since there is no concept of utf8 vs utf8mb4 in PostgreSQL and MS SQL Server (as far as I can tell) you can simply implement this simple getAlterTableCharacterSet method in each of these drivers:

public function getAlterTableCharacterSet($tableName)
{
    return array();
}

REALITY CHECK: Then the alterTableCharacterSet method will return false immediately. However, do note that these methods are NOT used anywhere in the CMS.

Regarding the getCreateDatabaseQuery you should override it in the PostgreSQL and MS SQL Server drivers with this simple thing:

protected function getCreateDatabaseQuery($options, $utf)
{
    return 'CREATE DATABASE ' . $this->quoteName($options->db_name);
}

Please do remember that the special syntax to add a character only makes sense in MySQL (mysql, mysqli and pdomysql). In other db drivers it doesn't make sense.

REALITY CHECK: The core code as it was before my changes (last modified June 2nd 2013 by @mbabker) would still fail on non-MySQL databases when $utf was true. We never bumped into this problem because you can only create a database if you have root privileges in your database. Remember that this is never the case unless You Are Doing Something Completely Stupid™. For 2 1/2 years nobody reported a problem ergo it's not the "OMG! The sky is falling! Release blocker!" issue you made of it. It's just a long standing pesky bug which is there to remind us that either nobody creates databases with JDatabaseDriver or nobody uses Joomla! on non-MySQL hosts (or both, which is the evidence I have been collecting the last 2 years through Akeeba Backup but nobody is willing to listen to me, you had to reproduce stats collection and then wait another 1-2 years to make sense of them – but I digress).

So, don't panic! Implement two one-liner methods in four classes (JDatabaseDriverPostgresql, JDatabaseDriverSqlsrv, JDatabaseDriverSqlazure, JDatabaseDriverOracle) and you're done. Why four classes? If you are changing code which is NOT used anywhere in the CMS just for correctness' sake you should address all of the additional databases supported by JDatabase, not just the two that came first to your mind. And no, none of these is actually a release blocker since the code isn't actually used anywhere in the CMS.

avatar wilsonge
wilsonge - comment - 6 Mar 2016

@810 can you add in the methods into the database driver sub classes that Nic mentioned please?

avatar wilsonge wilsonge - change - 6 Mar 2016
Labels Removed: ?
avatar wilsonge wilsonge - change - 6 Mar 2016
Milestone Added:
avatar wilsonge wilsonge - change - 6 Mar 2016
Status New Closed
Closed_Date 0000-00-00 00:00:00 2016-03-06 23:36:34
Closed_By wilsonge
avatar wilsonge wilsonge - close - 6 Mar 2016
avatar wilsonge wilsonge - close - 6 Mar 2016
avatar alikon
alikon - comment - 7 Mar 2016

imo just needed the override of those methods for non-mysql drivers

Add a Comment

Login with GitHub to post a comment