? Pending

User tests: Successful: Unsuccessful:

avatar roland-d
roland-d
1 Feb 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 :)

avatar roland-d roland-d - open - 1 Feb 2016
avatar roland-d roland-d - change - 1 Feb 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Feb 2016
Labels Added: ?
avatar brianteeman
brianteeman - comment - 2 Feb 2016

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.

avatar wilsonge wilsonge - change - 2 Feb 2016
The description was changed
avatar wilsonge
wilsonge - comment - 2 Feb 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

avatar brianteeman
brianteeman - comment - 3 Feb 2016

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

avatar uglyeoin
uglyeoin - comment - 3 Feb 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?

avatar richard67
richard67 - comment - 3 Feb 2016

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.

avatar richard67
richard67 - comment - 14 Feb 2016

@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.

avatar wilsonge
wilsonge - comment - 14 Feb 2016

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

avatar wilsonge wilsonge - assigned - 14 Feb 16
avatar roland-d
roland-d - comment - 14 Feb 2016

@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.

avatar richard67
richard67 - comment - 14 Feb 2016

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

avatar roland-d
roland-d - comment - 14 Feb 2016

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

avatar roland-d
roland-d - comment - 14 Feb 2016

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

avatar richard67
richard67 - comment - 15 Feb 2016

@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.

avatar richard67
richard67 - comment - 15 Feb 2016

@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.

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67 richard67 - test_item - 15 Feb 2016 - Tested unsuccessfully
avatar richard67
richard67 - comment - 15 Feb 2016

I have tested this item :red_circle: 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.

avatar nikosdion
nikosdion - comment - 15 Feb 2016

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

avatar richard67
richard67 - comment - 15 Feb 2016

@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?

avatar wilsonge
wilsonge - comment - 15 Feb 2016

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?

avatar nikosdion
nikosdion - comment - 15 Feb 2016

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...

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67
richard67 - comment - 15 Feb 2016

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.

avatar richard67
richard67 - comment - 15 Feb 2016

Now my proposed changes from 2nd comment before this one are ok on the issue tracker, too.


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

avatar wilsonge
wilsonge - comment - 15 Feb 2016

@richard67 are we confident that the debian mysql stuff actually adhere's to the php documentation? I wouldn't call it a given

avatar nikosdion
nikosdion - comment - 15 Feb 2016

Hey George. No. PHP's documentation assumes that MySQL reports a proper MySQL version number in the format x.y.z where x, y and z are integers. Debian "hacks core" with MySQL and appends its own crap to z after a + sign. That's the entire problem all along. That's why I asked you to give me full details on Saturday night, I expected something like that to be going on.

By the way, the code that I gave you above to work around the issue is the exact code we are using in the Akeeba Usage Stats collection server code for over 18 months. And yeah, it works. It's not an untested theory based on some generic documentation that nobody tested anywhere but insists it's "better" for some reason no sane person understands.

avatar richard67
richard67 - comment - 15 Feb 2016

@wilsonge The problem is not only debian stuff, it is also "mysqlnd" prepended to the client info. Some hosting providers make own compilations of database drivers and prepend or append stuff.

As I proposed above, for mysqli we could solve that in an easy way using the new mysqli_get_server_version and mysqli_get_client_version functions. If necessary we also could correct the existing getVersion function of the driver to use mysqli_get_server_version to obtain a correct version string without crap appended or prepended.

I really would like to know what you think about this solution.


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

avatar richard67
richard67 - comment - 15 Feb 2016

@wilsonge P.S.: I have tested it of course here with some debug output added, and it works well, e.g. in my case with the mysqlnd driver it returns following results:

Client version: 50011
Server version: 50546
Server version greater or equal 5.5.3: Has utf8mb4 support.
Mysqlnd client greater or equal 5.0.9: Has utf8mb4 support.


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

avatar richard67
richard67 - comment - 15 Feb 2016

@nikosdion Do you know if the public getVersion function of the driver is supposed to return a correct version string without any crap, so it can be checked with version_compare?

If so, then we should not correct the server version detection in the serverClaimsUtf8mb4Support function, we should correct it instead in the getVersion function itself, using mysqli_get_server_version as base for building the correct version string.

But if getVersion is supposed to return just a string info which may contain any crap, and someone wants to see this crap then being displayed somewhere, then we should not touch it and do as I proposed above for mysqli.

And for the old mysql either the regex has to be changed, or other string processing has to be done, but also either in serverClaimsUtf8mb4Support or in getVersion, depending on the purpose of getVersion.

Please let me know your opinion.

avatar nikosdion
nikosdion - comment - 15 Feb 2016

Unfortunately that bit was unspecified, implying we are meant to return the raw version - including any crap. Since this crap is used in things like stats collection we can't touch the driver :(

avatar roland-d
roland-d - comment - 15 Feb 2016

@wilsonge Already updating the PR at this moment, will commit the changes soon. @nikosdion Thanks for the code snippet.

avatar mbabker
mbabker - comment - 15 Feb 2016

Since this crap is used in things like stats collection we can't touch the driver

I don't know who all is running stats collection, but for Joomla's stats we're stripping all the extra version crap off string and just storing a X.y.z format version number. https://github.com/joomla-extensions/jstats-server/blob/master/src/Controllers/SubmitControllerCreate.php

Also PHP's version_compare() will deal fine with all that extra crap because of how it standardizes things so there isn't a major need to change what getVersion() returns. http://php.net/manual/en/function.version-compare.php

avatar roland-d
roland-d - comment - 15 Feb 2016

I can confirm what @mbabker is saying, version_compare doesn't care about the extra crap. So I guess we don't need to be difficult and just let version_compare() do it's thing.

avatar roland-d
roland-d - comment - 15 Feb 2016

I just have one question, when do we get a value of just mysqlnd?

avatar richard67
richard67 - comment - 15 Feb 2016

Sure, version_compare could be ok to compare the versions without having to to regex stuff before ... but it still needs the "if (strpos($client_info, 'mysqlnd') !== false)" and the check for version 5.0.9 in this case, as it is with my database driver here with utf8mb4 support:
Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $
Server version: 5.5.46-0+deb7u1-log

avatar roland-d
roland-d - comment - 15 Feb 2016

Thanks for the heads-up @richard67

avatar richard67
richard67 - comment - 15 Feb 2016

And for the mysqli driver I still think that what I proposed above using the "..._version" instead of the "..._info" functions and comparing integers would be the better solution.

avatar nikosdion
nikosdion - comment - 15 Feb 2016

OK, I am back to a real computer. Let me comment on all your questions.

When you do NOT have mysqlnd (i.e. you are using the legacy mysql and mysqli drivers) the version reported is the MySQL version. In this case you need MySQL version 5.5.3 or later for UTF8MB4 support.

When mysqlnd is being used in PHP then the mysql and mysqli drivers report the mysqlnd driver's version, NOT the MySQL server version! This is very important. In this case we MUST make sure that mysqlnd is at version 5.0.9 or later. If we do not get a mysqlnd version at all we have to assume it's an older version that does not support UTF8MB4.

Please note that if you are using mysqlnd YOU CANNOT BLINDLY CHECK THE MYSQL VERSION! This is why using the _version instead of _info functions is WRONG! IF you have mysqlnd LOWER than 5.0.9 AND you have MySQL HIGHER than 5.5.3 THEN you DO NOT have utf8mb4 support. Do bear in mind that mysqlnd is a native driver; it does not use the MySQL client library (which is what mysql and mysqli used to do).

What I do not know is what happens when you are using mysqlnd 5.0.9 or later on MySQL 5.5.2 or lower. Can anyone please test that combination? Basically you need a recent PHP version (e.g. 5.6) with mysqlnd support and an ancient MySQL server, e.g. of the 5.0 version range. This is the one case that's not being addressed by our code and one I suspect MAY cause some problems (I am not entirely sure, hence I want someone to test).

The other case we must test is using a legacy (mysql / mysqli) driver on Debian because Debian adds crap to the version number.

As for PDO, you don't need to do detection (YAY!). You try connecting with UTF8MB4 collation. If the server does not support UTF8MB4 it throws a runtime exception which we catch in lines 113:134, switch back to plain old UTF8 and connect. Plain, simple, clean. MAYBE we could do something similar for our MySQL/MySQLi drivers but that would probably take a month to test and I agree with Richard that we do NOT have that much time in our hands.

avatar roland-d
roland-d - comment - 15 Feb 2016

The regex is working fine on a mysqlnd version string like mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $ so I rather leave it as-is.

Since version_compare() can deal with crap in version strings it seems we are OK there too. Here is what I tested:

$res = version_compare('5.5.46-0+deb7u1-log', '5.6.9', '>='); var_dump($res);
bool(false)

$res = version_compare('5.5.46-0+deb7u1-log', '5.0.9', '>='); var_dump($res);
bool(true)

Unless I am mistaken we do not need to make any further changes to this PR, unless something comes up with the final tests proposed by @nikosdion.

Other than that I can only second what @nikosdion has written.

avatar richard67
richard67 - comment - 15 Feb 2016

@roland-d We still need the change in the regex for mysql which I proposed with my PR to your branch if we wanna continue to use regex. For mysqli the regex was ok, for mysql not. See my comment above, where you can see results for bith drivers mysql and mysqli, both with mysqlnd driver.

@nikosdion As my test result in another comment above shows, the _version function correctly returns the version of the mysqlnd driver, in my example 50011, so what you wrote above regarding not to use them because they would only check the mysql version is not correct.

avatar richard67
richard67 - comment - 15 Feb 2016

@roland-d @nikosdion Beside the combination I used for the tests I previousply mentioned (server 5.5.46 and client mysqlnd 5.0.11), I have such a combination which you requested for an additional test (server 5.1.73 and client mysqlnd 5.0.10).

So I can test this later today or tomorrow, but first the error in the regex has to be solved for the mysql.php (without i), as I mentioned in my previous comment for Roland and here.

Roland just would have to accept my PR to his branch for that.


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

avatar richard67
richard67 - comment - 15 Feb 2016

@roland-d @nikosdion Another thing I have just tested is that in case of the myqslnd driver (i.e. "if (strpos($client_info, 'mysqlnd') !== false)"), it really needs the preg_replace. When not using it, the "version_compare($client_version, '5.0.9', '>=')" does not return true as it should be and as it does after the preg_replace.

avatar nikosdion
nikosdion - comment - 15 Feb 2016

Can you please post the contents of $client_info in your failed case? If it is what I think it is we do not need preg_replace (see my comments from 6 hours ago).

avatar richard67
richard67 - comment - 15 Feb 2016

@nikosdion The variable you mean is not called $client_info, it is called $client_version in code.

And I already posted everything in my comment above the test invalid result, as I pointed out several times, just look to the snippet at the end with the results. So I have really the feelings my comments are not really read.

As the post I mentioned shows, $client_version is "mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $".

The preg_replace we have in this PR in mysqli.php correctly extracts the version from that, the preg_replace we have in this PR in mysql.php (without i) does not, as py post has shown, and just before I have tested that we need the preg_replace, because the version_compare with the "mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $" does not work!

I explained all this before and it can be viewn in my posts above, so I really have the feeling those are not carefully read or my English writing knowledge is much worse than I always thought.


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

avatar nikosdion
nikosdion - comment - 15 Feb 2016

Hello Richard

As I explicitly stated above I am on vacation in London. As I said I am NOT on my proper computer - only had access to it for a short amount of time. I am replying from a freaking iPad ON MY ONE DAY OF VACATION.

Per my comments from over 6 hours ago your regular expression is an overkill and needs be replaced by four lines of straightforward PHP code. The same lines I posted 6 hours ago!!!

The only reason I asked you AGAIN for the contents of the string (using the name you posted, hoping to not confuse you with a different variable name) was to confirm that indeed nothing changed. Therefore I confirmed that you are simply being stubborn, not only ignoring my suggestion but wasting the time of everyone on this thread, repeatedly, despite a PLT member telling you in no uncertain terms what the next steps are. Bonus points for being an arse towards me.

George, I am unsubscribing from this. If you need my help please email me.

avatar richard67
richard67 - comment - 15 Feb 2016

@nikosdion

Sorry, Nik, but where have I been an arse towards to you? It was my English language knowledge I mentioned, not yours, just in case you got this wrong, sorry if so.

Secondly, it is not on me to decide whether to stay with the regex or use your suggested code, because it is not my PR. I only pointed out an existing mistake in this PR and provided detailed text code and results and maybe repeated myself from time to time because I had the feeling it was ignored somehow.

Please let me know where I was an arse, if I was offensive or whatever else made you say this.

If I really did, I apologize, but I think you really do me wrong here.

@wilsonge Please check too and let me know if I was offensive or some other kind of arse, and also let me know if I waste people's time, then I will stop contributing to this topic.

But I really think Nik does me wrong.

avatar nikosdion
nikosdion - comment - 15 Feb 2016

I think we are getting confused.

The reg ex is a wrong idea. Seeing the data I am sure we need the explode and implode thing. But in any case we do need to test against real servers.

Do we all agree on that?

avatar brianteeman
brianteeman - comment - 15 Feb 2016

Lets all try and stay discussing the code

On 15 February 2016 at 19:13, Nicholas K. Dionysopoulos <
notifications@github.com> wrote:

I think we are getting confused.

The reg ex is a wrong idea. Seeing the data I am sure we need the explode
and implode thing. But in any case we do need to test against real servers.

Do we all agree on that?

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

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar richard67
richard67 - comment - 15 Feb 2016

I have real servers, 2 systems, both 1&1 shared hosting, one with database version 5.5.46 (=utf8 capable), other one with 5.1.43 (= not utf8mb4 capable), both with a utf8mb4 capable mysqlnd client. For both systems I can use all 3 drivers mysql, mysqli and pdo, so I can do 6 tests. All 2 systems have the debian crab appended on the server version and the mysqlnd crab on the client version as shown in my results above.

So I think I could help with tests if people still let me do so.

In addition to what I have we would need for further tests

  1. a system with a server being capable of utf8mb4 and a client being not capable - this I could maybe get with changing to a lower PHP version on one of my systems, but this means some work for me, and
  2. systems without any crab appended or prepended to version infos so any regex or explode/implode solution still finds a version number of no crab to strip off.

I agree with you, Nik, that a regex solution is not easy to read and might fail if not correctly handling all cases, e.g. what if the "mysqlnd" is not prepended but appended to the client version and so on.

But I am not familiar enough with explode/implode or PHP in general to easily implement it. I am a c and fortran and such stuff programmer :smile:

So the best is I wait for further feedbacks and commit(s) and then help with testing this is still desired.

Or I will stay completely calm if this is desired, just let me know then.

avatar wilsonge
wilsonge - comment - 15 Feb 2016

OK So i'm actually able to test this at home with @roland-d 's testing instructions i have

$client_version = mysqli_get_client_info();
$server_version = $this->getVersion();

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';
    exit();
    return false;
}
else
{
    if (strpos($client_version, 'mysqlnd') !== false)
    {
        $client_version = preg_replace('/^\D+([\d.]+).*/', '$1', $client_version);
        echo 'Server version lower than 5.5.3';
        exit();

        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 for mysqlnd';
           exit();
        }
    }
}

and then i get on my screen (on my local xampp setup)

Client version: mysqlnd 5.0.11-dev - 20120503 - $Id: bf9ad53b11c9a57efdb1057292d73b928b8c5c77 $
Server version: 5.6.20
Server version lower than 5.5.3

So clearly the currently Pull Request doesn't work. Next step is to try and step through the proposed changes by Richard and Nic and figure out if I can find a version that works on my setup :)

avatar richard67
richard67 - comment - 15 Feb 2016

@wilsonge The 2nd echo in your snippet is wrong, it should be:

echo 'Client version greater than 5.0.9';

according to @roland-d 's description.

Maybe that's why you get this output?


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

avatar richard67
richard67 - comment - 15 Feb 2016

@wilsonge @roland-d Sorry, meant the 4th echo, if you count the 2 at the top, too.


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

avatar wilsonge
wilsonge - comment - 15 Feb 2016

OK So using Nic's code i get

        $client_version = mysqli_get_client_info();
        $server_version = $this->getVersion();

        foreach (array($client_version, $server_version) as &$version)
        {
            $parts = explode(',', $version, 3);
            if (isset($parts[2])) $parts[2] = substr($parts[2], 0, 3);
            if (isset($parts[2])) $parts[2] = (int) $parts[2];
            $version = implode('.', $parts);
        }
        var_dump($client_version);var_dump($server_version);die;

        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', '>=');
            }
        }

The two var_dumps give me

string 'mysqlnd 5.0.11-dev - 20120503 - $Id: bf9ad53b11c9a57efdb1057292d73b928b8c5c77 $' (length=79)
string '5.6.20' (length=6)

so it would appear that Nic's code (as is) doesn't seem to give a fully complient client version either :(

avatar nikosdion
nikosdion - comment - 15 Feb 2016

I told you to add my code INSIDE the mysqlnd if block, NOT before it. The idea is that the reg ex fetches the dirty version and my four lines of code strip it down to integers.

avatar wilsonge
wilsonge - comment - 15 Feb 2016

My bad.

So doing

$client_version = mysqli_get_client_info();
        $server_version = $this->getVersion();

        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);
                $parts = explode(',', $client_version, 3);
                if (isset($parts[2])) $parts[2] = substr($parts[2], 0, 3);
                if (isset($parts[2])) $parts[2] = (int) $parts[2];
                $client_version = implode('.', $parts);
                var_dump($client_version);die;
                return version_compare($client_version, '5.0.9', '>=');
            }
            else
            {
                return version_compare($client_version, '5.5.3', '>=');
            }
        }

the dump of client version before the version compare is:

string 'mysqlnd 5.0.11-dev - 20120503 - $Id: bf9ad53b11c9a57efdb1057292d73b928b8c5c77 $' (length=79)

So unless i'm still doing stupid stuff it would still appear to have no affect?

avatar wilsonge
wilsonge - comment - 15 Feb 2016

In that at least the imploded version is no different to the one before any regex/filtering

avatar nikosdion
nikosdion - comment - 15 Feb 2016

@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)

avatar Kubik-Rubik
Kubik-Rubik - comment - 16 Feb 2016

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. :-)

avatar Kubik-Rubik Kubik-Rubik - change - 16 Feb 2016
Milestone Added:
avatar richard67
richard67 - comment - 16 Feb 2016

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.

avatar Kubik-Rubik
Kubik-Rubik - comment - 16 Feb 2016

@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!

avatar wilsonge
wilsonge - comment - 16 Feb 2016

@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 :)

avatar joomla-cms-bot
joomla-cms-bot - comment - 16 Feb 2016

This PR has received new commits.

CC: @richard67


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

avatar nikosdion
nikosdion - comment - 16 Feb 2016

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.

avatar richard67
richard67 - comment - 16 Feb 2016

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

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.

avatar roland-d
roland-d - comment - 16 Feb 2016

@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 !!

avatar richard67
richard67 - comment - 16 Feb 2016

@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.

avatar roland-d
roland-d - comment - 16 Feb 2016

@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

avatar wilsonge
wilsonge - comment - 16 Feb 2016

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.

avatar richard67
richard67 - comment - 16 Feb 2016

@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.

avatar richard67 richard67 - test_item - 16 Feb 2016 - Tested successfully
avatar richard67
richard67 - comment - 16 Feb 2016

I have tested this item :white_check_mark: 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.

avatar roland-d
roland-d - comment - 16 Feb 2016

@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.

avatar wilsonge
wilsonge - comment - 16 Feb 2016

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

avatar roland-d
roland-d - comment - 16 Feb 2016

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?

avatar richard67
richard67 - comment - 16 Feb 2016

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

avatar roland-d
roland-d - comment - 17 Feb 2016

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.

avatar wilsonge
wilsonge - comment - 18 Feb 2016

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)

avatar wilsonge wilsonge - close - 18 Feb 2016
avatar wilsonge wilsonge - reference | e7aaae9 - 18 Feb 16
avatar wilsonge wilsonge - merge - 18 Feb 2016
avatar wilsonge wilsonge - close - 18 Feb 2016
avatar wilsonge wilsonge - change - 18 Feb 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-02-18 01:05:39
Closed_By wilsonge
avatar richard67
richard67 - comment - 18 Feb 2016

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

avatar wilsonge
wilsonge - comment - 18 Feb 2016

Thanks Richard! Closed it :)

Add a Comment

Login with GitHub to post a comment