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

Send proper MariaDB database version to the stats server #25281

Merged
merged 1 commit into from Jun 23, 2019

Conversation

Projects
None yet
6 participants
@mbabker
Copy link
Member

commented Jun 20, 2019

Summary of Changes

Right now, when reporting stats for MariaDB users the CMS sends either a version string like "10.1.29-MariaDB" or "5.5.5-10.1.29-MariaDB" to the stats server when reporting data, which through the sanitization routines the latter ends up in an entry being tracked as "5.5.5". In order to better track MariaDB usage, this pull request changes the string sent forward in the "5.5.5-10.1.29-MariaDB" case so that it is the MariaDB version number only (i.e. 10.1.29) without any of the extra cruft (note, the "10.1.29-MariaDB" case does get correctly sanitized down and stored as 10.1.29; the stats server's sanitization step for version strings basically removes everything after a SemVer compliant <major>.<minor>.<patch> matching segment, this is what removes the stability bit from the CMS version, or changes something like "5.5.9-1ubuntu4.11" into "5.5.9", and if you were using a Google Cloud SQL database their "5.7.14-google-log" string would be stored as "5.7.14").

This utilizes the regex check from Doctrine that was referenced in #25245 (comment)

Testing Instructions

Apply the patch to a Joomla installation which uses a MariaDB backend. Then, check the data that the stats plugin will send to the server (you can see this info on the "System - Joomla! Statistics" plugin's edit screen). Pre-patch, the database version should be whatever it is now; post-patch, if it has the "5.5.5-" prefix and "-MariaDB" suffix, it should be only the MariaDB version string (the stuff in the middle of the *fix bits).

@mbabker mbabker requested a review from wilsonge as a code owner Jun 20, 2019

@Quy

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I have tested this item successfully on e31230c

Before:

DB Version 5.5.5

After:

DB Version 10.3.15


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25281.
@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I have tested this item successfully on e31230c


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

@Quy

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Jun 20, 2019

'unique_id' => $this->getUniqueId(),
'php_version' => PHP_VERSION,
'db_type' => $this->db->name,
'db_version' => $this->db->getVersion(),
'cms_version' => JVERSION,
'server_os' => php_uname('s') . ' ' . php_uname('r')
);
// Check if we have a MariaDB version string and extract the proper version from it
if (preg_match('/^(?:5\.5\.5-)?(mariadb-)?(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)/i', $data['db_version'], $versionParts))

This comment has been minimized.

Copy link
@richard67

richard67 Jun 20, 2019

Contributor

This regex and the one from doctrine care for string like "5.5.5-Mariadb-10.0.8-xenial", as the function documentation at doctrine sais. This kind of strings seems to come from a database select for the versio number. But in Joomla framework database package, the mysqli::$server_info is used, which gives string like e.g. "5.5.5-10.1.29-MariaDB", see e.g. here: https://www.php.net/manual/de/mysqli.get-server-info.php#118822. Also in issue #25245 @Quy reported a string like that. So that regex works, because "mariadb-" may appear zero or one time, and the rest after the real version number is ignored, but it might be at least formally not correct and confusing when reading. But this is just my impression, it seems to work so it is ok.

This comment has been minimized.

Copy link
@mbabker

mbabker Jun 20, 2019

Author Member

It's going to be fine. If my regex reading skills are OK, it's going to match if there is a "5.5.5-" prefix, a "mariadb-" prefix, or a "5.5.5-mariadb-" prefix, then the version number that is found in the first group after that is extracted into the $versionParts array. What we want is "10.0.8"; "5.5.5-Mariadb-10.0.8-xenial", "5.5.5-10.0.8-MariaDB", and "5.5.5-10.0.8-xenial" will all end up that way.

The only other way to do it "right" is going to result in a leaky abstraction (either the plugin has to issue a RDBMS specific query to get the version that way, or the $connection resource has to be made available).

This comment has been minimized.

Copy link
@richard67

richard67 Jun 20, 2019

Contributor

Right, it will work and return the right version so or so.
Unfortunately I don't have a MariaDB set up yet, so I could test only with code review. Would that be enough?

This comment has been minimized.

Copy link
@mbabker

mbabker Jun 20, 2019

Author Member

I did it blind taking battle tested code from another framework and adapting it, I have exactly one MariaDB server at my disposal and it's on the one client server whose WHM was built with MariaDB enabled by default instead of MySQL (and they don't use Joomla, so clearly I can't abuse that server for testing).

This comment has been minimized.

Copy link
@richard67

richard67 Jun 20, 2019

Contributor

Let's wait and see, if we don't have a 2nd tester tomorrow I can set up a MariaDB. What keeps me from doing it is that I would ha eto specify a special port so it does not use the same as my MySQL currently used. So or so, code review looks good, I am sure it works asd desired.

This comment has been minimized.

Copy link
@Quy

Quy Jun 20, 2019

Contributor

Live server on Linux 3.10.0-957.10.1.el7.x86_64:

System Information: 5.5.60-MariaDB
Stats plugin: 5.5.60

This comment has been minimized.

Copy link
@Quy

Quy Jun 20, 2019

Contributor

See this comment for possible MariaDB combinations: #25283 (comment)

This comment has been minimized.

Copy link
@richard67

richard67 Jun 20, 2019

Contributor

Until some version 5.5.x, MariaDB used the same numbering scheme as MySQL. Later they changed to 10.x.y. For the J4 installation that should be ok, because "5.5.60-MariaDB" is older than minimum version. For this PR here in staging I am not sure, it might be in fact confusing having 5.w.x and also 10.x.y in the statistics. But assuming that we will not allow installation of staging on MariaDB and not allow such old 5.x.y on J4, it should be practically ok. At the end we will maybe increase requirement to 10.4 anyway, as discussed in the other issue.

This comment has been minimized.

Copy link
@mbabker

mbabker Jun 20, 2019

Author Member

For this PR here in staging I am not sure, it might be in fact confusing having 5.w.x and also 10.x.y in the statistics.

Having MySQL with 10.x (purely MariaDB) and 5.x (mixed MariaDB and MySQL) and 8.x (purely MySQL) version numbers isn't an issue. Actually, the 5.5.5 string is the worst bit of it all (which accounts for a not-so-insignificant number of the total MySQL stats) because we haven't the slightest idea what MariaDB backend is in use (whereas at least the 10.x ones we can tell the difference, there won't be a problem until MySQL gets to 10.x).

In the case of 5.5.60-MariaDB, there isn't a sane way to distinguish that one with the current architecture (neither in the stats plugin here or at the server level, "mariadb" just isn't an allowed thing right now because there isn't a "mariadb" database driver). It's one of those things we just live with.

This comment has been minimized.

Copy link
@richard67

richard67 Jun 20, 2019

Contributor

I fully agree.

@wilsonge
Copy link
Contributor

left a comment

Works on my local mariadb install well

@wilsonge wilsonge merged commit 27ea0dc into joomla:staging Jun 23, 2019

5 checks passed

Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

Thanks!

@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC labels Jun 23, 2019

@wilsonge wilsonge added this to the Joomla 3.9.9 milestone Jun 23, 2019

@mbabker mbabker deleted the mbabker:stats-mariadb-compat branch Jun 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.