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 UTF8MB4 support to the database drivers. #9045

Merged
merged 3 commits into from Feb 18, 2016

Conversation

roland-d
Copy link
Contributor

@roland-d roland-d commented Feb 1, 2016

Added UTF8MB4 support to the database drivers.

Testing instructions

Only needed if you are using a mysql* database

mysql/mysqli drivers

  1. Apply patch on a mysql/mysqli installation
  2. Check in the global configuration which database driver you have set
  3. Open the file libraries/joomla/database/driver/X.php where X matches the database driver you have found in step 2.
  4. Do the server version check.
    Find the line that says
$server_version = $this->getVersion();

after that add the following code:

echo 'Client version: ' . $client_version . '<br />';
echo 'Server version: ' . $server_version . '<br />';

Next find the line that says:

if (version_compare($server_version, '5.5.3', '<'))
{

after that add the code:

echo 'Server version lower than 5.5.3';
exit();

Next find the line that says:

$client_version = preg_replace('/^\D([\d.]).*/', '$1', $client_version);

after that add the code:

if (version_compare($client_version, '5.0.9', '>='))
{
   echo 'Client version greater than 5.0.9';
   exit();
}

Next find the line that says:

return version_compare($client_version, '5.5.3', '>=');

BEFORE that add the code:

if (version_compare($client_version, '5.5.3', '>='))
{
   echo 'Client version greater than 5.5.3 for mysqlnd';
   exit();
}

Load any page and you should see one of the messages. You have now tested the server version and client version check for MySQL.

You can also verify if the reported client and server versions are correct for what is used on your server.

pdo driver
Here we can only check the server version.

Find the line that says:

$this->utf8mb4 = version_compare($serverVersion, '5.5.3', '>=');

After that add the line:

if ($this->utf8mb4)
{
   echo 'Server version greater than 5.5.9';
   exit();
}

Load any page and you should see the message if the server supports UTF8MB4 on the PDO driver.

Make sure to undo your changes to make sure the site loads normal again :)

@brianteeman
Copy link
Contributor

Can you provide some simple code snippet please to help testers


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

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2016

This code is designed to detect the server mysql support for something. You can't just magically run some code and see a "right" or "wrong" answer. You literally just have to check the value of that snippet in step 2 and see if it's true or false and then check whether your mysql actually has support for utf8mb4 or not

@brianteeman
Copy link
Contributor

Again - as you know most people who are testing are not developers. Please provide full test instructions how to test that value. Otherwise its just not going to get tested

@uglyeoin
Copy link
Contributor

uglyeoin commented Feb 3, 2016

Agree with @brianteeman I don't understand what is required.

Where do I put the snippet in step 2?
How do I check the value?

@richard67
Copy link
Member

The snippet from step 2 can be added nowhere because the serverClaimsUtf8mb4Support functions of the drivers are private, so you cannot just add a PHP "echo JFactory::getDbo()->serverClaimsUtf8mb4Support();" in the index.php of your frontend template.

Furthermore, the title is not correct because this PR does not really add the utf8mb4 support to the driver, it just corrects detection of capability of both server and client according to their version numbers for the different databases and clients.

So it is not really clear how this shall be tested. It would need some combination of database and driver where the detection of utf8mb4 capability was wrong before and is correct after the patch, e.g. like it was for some of the testers for PR #8472.

I compared the changes in this PR here with the corresponding changes in PR #8472 and see they are same except a small difference in the regex check in line 525 of file libraries/joomla/database/driver/mysql.php ("$client_version = preg_replace('/^\D([\d.])./', '$1', $client_version);"), where 2 "+" operators are missing in this PR here, while in PR #8472 there were those 2 operators ("$client_version = preg_replace('/^\D+([\d.]+)./', '$1', $client_version);").

@roland-d Can you check and let me know if this is desired or not? If desired, this PR here could be tested by code review, e.g. comparing with the corresponding changes in PR #8472, where things have been tested with different databases and drivers.


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

@richard67
Copy link
Member

@roland-d Anything new? Did you check my previous comment? Or maybe @wilsonge should check it?


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

@wilsonge
Copy link
Contributor

I'm not confident enough with regex to know exactly whether Roland intended that change or not. If he could comment that would be awesome. Else we'll have to do it by good old fashioned testing :)

@nikosdion if you could review that also would be helpful

@wilsonge wilsonge self-assigned this Feb 14, 2016
@roland-d
Copy link
Contributor Author

@wilsonge To be honest, I don't remember why the change so I looked into it. The mysqli_get_client_info returns an integer, so it will never match mysqlnd. Doing a version_compare() seems enough to me.

@richard67 George already checked it but I had time issues as I had to deal with private stuff but things are getting back to normal. So I can follow up on this again.

@richard67
Copy link
Member

@roland-d Sorry, I did not wanted to put any pressure. Only wanted to help.

@roland-d
Copy link
Contributor Author

@richard67 No pressure, no worries. Just wanted to explain my lack of presence :)

@roland-d
Copy link
Contributor Author

@brianteeman @richard67 @uglyeoin I have updated the test instructions, hope this is clearer.

@richard67
Copy link
Member

@roland-d @wilsonge mysqli_get_client_info returns a string, mysqli_get_client_version would be the integer one.


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

@richard67
Copy link
Member

@roland-d I have done it a bit different with debug output in order to find out which of the regular expressions used in case of mysqlInd is the right one.

This is how I modified code in function serverClaimsUtf8mb4Support in the mysql.php file:

$server_version = $this->getVersion();
echo 'mysql.php<br />';
echo 'Client version: ' . $client_version . '<br />';
echo 'Server version: ' . $server_version . '<br />';

if (version_compare($server_version, '5.5.3', '<'))
{
    echo 'Server version lower than 5.5.3';
    return false;
}
else
{
    if (strpos($client_version, 'mysqlnd') !== false)
    {
        $client_version = preg_replace('/^\D([\d.]).*/', '$1', $client_version);
        echo 'Client version replaced: ' . $client_version . '<br />';
        if (version_compare($client_version, '5.0.9', '>='))
        {
            echo 'Mysqlnd client version greater than 5.0.9';
        }
        return version_compare($client_version, '5.0.9', '>=');
    }
    else
    {
        if (version_compare($client_version, '5.5.3', '>='))
        {
            echo 'Client version greater than 5.5.3';
        }
        return version_compare($client_version, '5.5.3', '>=');
    }
}

The same kind of modifications I did in the mysqli.php file.

And now look wat results I have got for both mysql and mysli database:

mysqli.php
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.5.46-0+deb7u1-log
Client version replaced: 5.0.11
Mysqlnd client version greater than 5.0.9 

mysql.php
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.5.46-0+deb7u1-log
Client version replaced: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $

So the regex in mysqli.php seems to be correct, while the one in mysql.php is not anymore with your PR, but it was before the PR, because in mysql.php you removed the 2 "+" postfix operators from the regex, while in mysqli.php you not changed the regex.

So from my point of view you should replace the regex in mysql.php by the one from before this PR (or the one from mysqli.php, is the same).


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

@richard67
Copy link
Member

I have created a PR to your branch with the regex change I mentioned in my previous comment, see roland-d#8.

Please accept and merge.


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

@richard67
Copy link
Member

I have tested this item 🔴 unsuccessfully on b4feb02

For the mysqli driver, client utf8mb4 capability detection works for msqlInd client.

For the mysql driver (the old fashioned, depreceated one without i) it does not work due to incorrect regex, see my previous comments and my PR to your branch.

The PDO driver I haven't tested yet.


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

@nikosdion
Copy link
Contributor

The problem is caused by Debian and Ubuntu adding crap to the MySQL version string that's not supposed to be there. A quick'n'dirty way to work around that without regex is:

  • Explode version by dots with at most three parts, e.g. $parts = explode(',', $version, 3);
  • Take the third part, truncate it to 3 characters, e.g. if (isset($parts[2])) $parts[2] = substr($parts[2], 0, 3)
  • Now convert the third part to integer, e.g. if (isset($parts[2])) $parts[2] = (int) $parts[2];
  • And finally glue together the version parts, e.g. $version = implode('.', $parts[2]);
    Unless MySQL subminor version numbers go over 999 this will work perfectly.

Don't overengineer your solution. Nobody can read RegEx and 3-4 years, when you'll be unreachable, some poor sod will have to figure out WTF you were doing. Explore and implode is fairly obvious but PLEASE put a comment like // This code addresses non-standard MySQL version numbers in Debian / Ubuntu such as 5.5.46-0+deb7u1-log so that future maintainers will have a clue.

Oh, BTW, the code we are fixing was copied from WordPress. I guess we ended up fixing a bug in their code :D

@richard67
Copy link
Member

@nikosdion Well of course you or @roland-d or whomever can start now to develop a new way without regex, but maybe this should be done with another PR.

This one here did not introduce the regex stuff, it only changed order of processing, and with my proposed correction the regex will work for the mysql as well as it already does for the mysqli.

So before we wait another month for a solution, why not just accept my correction and text this PR here so we come forward?

@wilsonge
Copy link
Contributor

Well @nikosdion introduced the regex in the first place :P So I agree with his judgement (and Roland's regex won't cover the debian cases he mentioned without further changes anyhow - and these are required to get thigns working). @roland-d can you do Nic's fixes tonight please?

@nikosdion
Copy link
Contributor

If you can't I can assign my paid staff. It will definitely NOT take a month.

Let me summarize why this issue is still open. I deliberately didn't want to get involved in the code after a former PLT member told me in October that I have a conflict of interest by contributing code to the Joomla! core while also making Joomla! extensions. In these 4 months you were bouncing around hard walls because you never asked the affected users for details and / or access to an affected server. Now that this PLT member is out I feel I can contribute without being called out for offering my code, experience and time free of charge. Therefore, 36 hours ago I asked George to get the missing information. It was indeed provided (as it should've been 4 months ago) and I could immediately see what the problem is with the RegEx.

I am sorry I am not currently in the office with a proper code editor to write the code fix myself AND TEST IT AGAINST A DEBIAN INSTALLATION (the latter part being MOST important!). I am still in London, returning to the office on Wednesday.I wrote my suggestion here so that you can implement it tonight and not wait for me. And, for crying out loud, let me enjoy my one day of vacation...

@richard67
Copy link
Member

With the mysqli driver we could use the mysqli_get_client**_version** instead of the mysqli_get_client**_info** function for the client detection and same with mysqli_get**_server_version** for the server. They are supported in PHP 5 and 7 and return integer numbers, so no need for any "if (strpos($client_version, 'mysqlnd') !== false)" or "preg_replace(..." anymore for mysqli.

I will check soon if that works.

So regex crap or whatever would remain to be done only for the old fashioned and depreceated msysql-without-i driver.


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

@richard67
Copy link
Member

P.S.: ... because for the old fashioned and depreceated msysql-without-i driver those fancy functions "..._version" do not exist.


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

@richard67
Copy link
Member

P.P.S.: I just see if there are problems with Debian or other crap for the server version, too, and not only for the client version, then either the functions getVersion() in mysql.php and mysqli.php would have to be changed, too, or they should not be called to get the server version for the check of the utf8mb4 capability of the server but mysqli_get_server_version should be used instead.


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

@richard67
Copy link
Member

P.P.P.S.: On the other hand my test output from above shows that for the server version, the Debian crab not seems to be a problem.


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

@richard67
Copy link
Member

I've just checked doku of the functions mysqli_get_client_version and mysqli_get_server_version functions, see e.g. http://php.net/manual/en/mysqli.get-server-version.php.

As long as both minor version "y" and sub version "z" in a version with x.y.z not exceed 99, the results of the functions can be checked with integer comparisons. I think this is fulfilled, or do we have somewhere minor or sub-versions of more than 99?

If you all agree, we could use these functions then for the mysqli driver, and only for the mysql driver something has to be done with strings.

The mysqli solution then could look like this:

private function serverClaimsUtf8mb4Support()
{
    $this->connect();
    $server_version = mysqli_get_server_version($this->connection);

    // Mysql server supports utf8mb4 since version 5.5.3
    if ($server_version < 50503)
    {
        // No utf8mb4 support
        return false;
    }
    else
    {
        // Server supports utf8mb4: Check client
        $client_info = mysqli_get_client_info();
        if (strpos($client_info, 'mysqlnd') !== false)
        {
            // The mysqlnd client supports utf8mb4 since version 5.0.9
            return (mysqli_get_client_version() >= 50009);
        }
        else
        {
            // The mysqli client supports utf8mb4 since version 5.5.3
            return (mysqli_get_client_version() >= 50503);
        }
    }
}

The

$this->connect();
$server_version = mysqli_get_server_version($this->connection);

could also be put into a new separate function, as it is now with the getVersion, e.g. a getVersionInteger or getVersionNumber or so.

What do you think, guys?


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

@richard67
Copy link
Member

Sorry, had to correct some typos in code proposed in my previous comment, have done that on github so it can take a while until visible on the issue tracker, so pls. check it via github.


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

@nikosdion
Copy link
Contributor

@wilsonge You guys broke Joomla! all along :p The code I had provided you was correct. Good thing I have a good memory and an even better Git client. Here's the code that works:

if (version_compare($server_version, '5.5.3', '<'))
{
    return false;
}
else
{
    if (strpos($client_version, 'mysqlnd') !== false)
    {
        $client_version = preg_replace('/^\D+([\d.]+).*/', '$1', $client_version);

        return version_compare($client_version, '5.0.9', '>=');
    }
    else
    {
        return version_compare($client_version, '5.5.3', '>=');
    }
}

Please note the distinct change I made: I unfscked the preg_replace code you people had screwed up and replaced it with my original code. Guess what? My original code works. So exactly why you have not released Joomla! 3.5 yet is beyond me. George, when in doubt, use my original code ;)

(and you owe us a beer, mate, for making me tell Crystal to leave me alone while I hack around in a code editor in the middle of the night)

@Kubik-Rubik
Copy link
Member

Okay, so we have a valid solution?

if(version_compare($server_version, '5.5.3', '<'))
{
    return false;
}

if(strpos($client_version, 'mysqlnd') !== false)
{
    $client_version = preg_replace('/^\D+([\d.]+).*/', '$1', $client_version);

    return version_compare($client_version, '5.0.9', '>=');
}

return version_compare($client_version, '5.5.3', '>=');

@wilsonge @richard67 Could you please check?
@roland-d Could you please also check and eventually update your PR.

If all is okay, we should release Beta 3 asap! @wilsonge

@nikosdion Thank you for your efforts! I will invite you both to many beers if we finally get this done. :-)

@Kubik-Rubik Kubik-Rubik added this to the Joomla! 3.5.0 milestone Feb 16, 2016
@richard67
Copy link
Member

Haha now I must laugh: Since the beginning I pointed out that the regex in mysql.php was screwed up, while in mysqli.php it was still correct. @nikosdion do a diff and you will see that the one in mysqli.php was exactly what you provided in your last comment's code snippet, and comparing with mysql.php shpows exactly the difference I mentioned in my comment once.

I provided a PR against @roland-d branch to solve that, see this comment above, which exactly does what @nikosdion found out at the end.

It all was ignored or not taken serious.

When @wilsonge tested the regex, he used a wrong code snippet where the debug output within the if condition for "mysqlnd" check is not correct, see snippet here. I immediately mentioned that in 2 comments below (which also were not 100% correct but should have pointed on the mistake at least. Also this was ignored it seems.

So guys, if you would have checked my comments at the beginning and would have taken me serious, you could have had the solution 13 days ago.

The reason for this post now is not because I just wanted to say "haha I was right".

The reason is that I had the feeling not being taken serious (maybe because my github is not full of commits for Joomla, or because I am not a frequent contributor). If this is true and others are treated in the same way, then I can understand why some people get disappointed and stop to contribute and help after a while, and this is what not should happen, and to point on this problem in order to avoid it in future is why I write now.

@Kubik-Rubik Please check from a neutral point of view and let me know if I am right or wrong, so I can learn and change my point of view if necessary.

And sorry, I not wanna point on someone and say "you made some mistake so you are the bad guy, and I am the good guy". Nobody who makes non-trivial software never makes mistakes, and reading such conversations on a mobile phone is not easy or comfortable.

All I wanna do is to increase the awareness of this, that mistakes can happen to everbody, and every coder should be open for others to point on the mistakes (not on the people), whoever these others are.

And that's the other reason why I write: Maybe I made a mistake? Maybe I did not describe clearly enough what I meant? Or maybe I appeared to be impatient or stubborn or whatever? If so, please let me know how I can do it in a better way.

Sorry for any inconvenience and wasting your time, if you think I did so.

@Kubik-Rubik
Copy link
Member

@richard67 It is nothing against you personally. To be honest, I was not really involved in this bug fixing process until recently, so I can not tell you why your comments were not taken seriously. I'm really sorry if you feel sad now. Don't be too unhappy, trust and awareness will grow over time!

@wilsonge
Copy link
Contributor

@richard67 Actually I do consider you a regular (I've been seeing you around for a while now).

I did see your comment about the echo. But I then missed (and this is completely my bad) the follow up comment where you meant clarified the 4th comment - I just assumed at the time you meant the 3rd comment which I checked and thought was fine.

In terms of Nic's proposed code and what nobody seems to have mentioned so far is that actually the code proposed (by everyone?) is actually the code that we have in the repo at the moment. The only thing that is changed is that in the mysqli driver we seem to have lost the server check (which was correctly as @richard67 said in the mysql driver). I double checked back and that has been missing since the first commit of @nikosdion 's PR e3c8bda#diff-21345ddf44224657f5e2f7a0ea63d7d5R912

So basically let's summarize by I owe everyone in this PR beers and that the mysql and mysqli drivers can be fixed by just adding the server version check into the mysqli drivers and all we need to do is agree what needs to happen for the changes Roland made to the pdomysql driver :)

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @richard67


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

@nikosdion
Copy link
Contributor

With no IDE access trough most of the day I thought that:

  • you were changing my original reg ex which I knew worked, ergo that would be a bad idea
  • thought that my reg ex did miss the crap stuff, hence the proposed code

As soon as I had access to an IDE I saw that the problem was that this PR had screwed up my perfectly functional reg ex for no reason.

@richard67
Copy link
Member

@wilsonge Regarding the beer: In my case please a zero alcohol one 😄

I will test again today for mysql and mysqli and this time also for pdo for both uft8mb4-cabable and not capable db server today, but it may take a bit time ...

@roland-d ... or shall I wait for any further commits? I think no, but let me know if I shall wait with testing.

@nikosdion Well, one of my weak points is that I describe things in too long sentences where it could be done shorter, which is not very comfy to read on a mobile phone or so. So maybe this caused a bit confusion, too.

@roland-d
Copy link
Contributor Author

@richard67 I have been around and for sure not ignoring your feedback. In all honesty I can say I completely missed the PR against this branch. I have now merged it, thank you. This was the last change I think. If I missed anything else, don't hesitate to point it out to me :D Yes, we all deserve beer after this !!

@richard67
Copy link
Member

@roland-d I think I can start testing again in some 1 .. 2 hours and then will need another 1 .. 2 hours for all 6 tests (3 kinds of client and 2 kinds of database).

For mysql.php and mysqli.php I modified the debug output a bit, see the example for mysql.php:

private function serverClaimsUtf8mb4Support()
{
    $client_version = mysql_get_client_info();
    $server_version = $this->getVersion();
    echo 'mysql.php<br />';
    echo 'Client version: ' . $client_version . '<br />';
    echo 'Server version: ' . $server_version . '<br />';
    if (version_compare($server_version, '5.5.3', '<'))
    {
        echo 'Server version lower than 5.5.3.';
        return false;
    }
    else
    {
        if (strpos($client_version, 'mysqlnd') !== false)
        {
            $client_version = preg_replace('/^\D+([\d.]+).*/', '$1', $client_version);
            if (version_compare($client_version, '5.0.9', '>='))
            {
                echo 'Client version greater than 5.0.9';
            }
            return version_compare($client_version, '5.0.9', '>=');
        }
        else
        {
            if (version_compare($client_version, '5.5.3', '>='))
            {
                echo 'Client version greater than 5.5.3';
            }
            return version_compare($client_version, '5.5.3', '>=');
        }
    }
}

As you see, I added an echo for the name of the file so I later can see which was the result of which test (mysql.php or mysqli.php), and I do not do the exit after the debug output.

I can test for database with and another database without utf8mb4 support, both with a mysqlnd client having utf8mb4 support, for drivers mysql, mysqli and pdo.

In addition, we need someone to test the combination "database with utf8mb4 support and client without utf8mb4 support", and maybe tests for non-mysqlnd clients, too.


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

@roland-d
Copy link
Contributor Author

@richard67

As you see, I added an echo for the name of the file so I later can see which was the result of which test (mysql.php or mysqli.php), and I do not do the exit after the debug output.

Good idea.

In addition, we need someone to test the combination "database with utf8mb4 support and client without utf8mb4 support", and maybe tests for non-mysqlnd clients, too.

I don't have a real server to test this with but on my local machine I should be able to test this. Although as the PR creator, not sure if my tests are valid xD

@wilsonge
Copy link
Contributor

So the server checks implemented here fix at least this issue (#8267) where the client was running mysqlnd 5.0.10 (so compat) but the server (5.1.73) wasn't compat so incorrectly passed the checks.

@richard67
Copy link
Member

@roland-d @wilsonge

Just have finished my tests for all 3 drivers on the new database (with utf8mb4 support) with success.

Now wanted to prepare testing of the same for the old database (whithout utf8mb4 support).

Because this one is a 3.4.8, I prepared an update container based on latest staging plus this PR here.

But because latest staging includes PR #9131, it failed.

See my new issue on the tracker: https://issues.joomla.org/tracker/joomla-cms/9132

Since my new database is already converted to utf8mb4 I cannot just export this and import this into my old database, so I had to do the Joomla Update thing.

I think I can continue testing this PR here with the broken updated system, where database is inconsistent but code is ok.

If I can and have results I will update my test result here.


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

@richard67
Copy link
Member

I have tested this item ✅ successfully on dbaaa2a

Tested with success, here the results:

New database:

mysql.php
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.5.46-0+deb7u1-log
Client version greater than 5.0.9

mysqli.php
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.5.46-0+deb7u1-log
Client version greater than 5.0.9

pdomysql.php: Server version greater than 5.5.3 (repeated many times)

Old database:

mysql.php
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.1.73-log
Server version lower than 5.5.3.

mysqli.php
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.1.73-log
Server version lower than 5.5.3.

pdomysql.php: No output, i.e. the pdo driver correctly detected that the server does not support utf8mb4.


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

@roland-d
Copy link
Contributor Author

@richard67 I saw the failure of the latest build but this is not because of the PR. It is an issue with PHP 7 not being available.

The error you found is unexpected. @wilsonge Do we need to add a max length of 100 to the primary key here: https://github.com/joomla/joomla-cms/pull/9131/files#diff-135c6f58583408312a709459b19594c7R1561

I think that may be needed because this primary key is not an integer but a varchar field.

@wilsonge
Copy link
Contributor

Spinning around at work. But basically session id is a unique key of 128 maxlength anyhow according to php docs so it's fine having that at 191 again (it could go lower tbh). The #__user_keys i'm not 100% about wtf it's for.... so i'll just keep it at 191 and be sane

@roland-d
Copy link
Contributor Author

Just going to recap again for my sanity :)

The issue found by @richard67 has been fixed and as such closed. The test results by Richard are all OK.

Final tests to be done are:

In addition, we need someone to test the combination "database with utf8mb4 support and client without utf8mb4 support", and maybe tests for non-mysqlnd clients, too.

Is that correct?

@richard67
Copy link
Member

@roland-d Yes, this is correct as far as I am concerned.

@roland-d
Copy link
Contributor Author

I did some testing with these results:

PHP 5.5.14
MySQL 5.1.41

Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: bf9ad53b11c9a57efdb1057292d73b928b8c5c77 $
Server version: 5.1.41-community-log
Server version lower than 5.5.3 

PHP 5.5.14
MySQL 5.5.29
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: bf9ad53b11c9a57efdb1057292d73b928b8c5c77 $
Server version: 5.5.29
Client version greater than 5.0.9

PHP 5.3.28
MySQL 5.5.29
Client version: mysqlnd 5.0.8-dev - 20102224 - $Id: 731e5b87ba42146a687c29995d2dfd8b4e40b325 $
Server version: 5.5.29
Client version lower than 5.0.9

As for testing on non-mysqlnd clients, I am afraid I don't have any here. The tests themselves look good here.

@wilsonge
Copy link
Contributor

I've tested this as well - but my servers all support utf8mb4 - so kinda hard to do more than that. I think at this point I'm just going to merge this and release a beta tomorrow night so we can get wider testing (once I've fixed the install/upgrade sql issue and the com_contact dispatcher issue - both unrelated to this)

wilsonge added a commit that referenced this pull request Feb 18, 2016
Add UTF8MB4 support to the database drivers.
@wilsonge wilsonge merged commit e7aaae9 into joomla:staging Feb 18, 2016
@richard67
Copy link
Member

@wilsonge What about the old PR #8472 for UTF8MB4? Should that be closed, too (without merge of course)?

@wilsonge
Copy link
Contributor

Thanks Richard! Closed it :)

@roland-d roland-d deleted the utf8mb4-drivers branch April 13, 2016 08:30
@wilsonge wilsonge removed their assignment Oct 25, 2019
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

9 participants