?
avatar nikosdion
nikosdion
12 Jan 2021

Joomla is setting the MySQL query_cache_type server system variable when Site Debug is set to Yes. However, this server system variable is deprecated in MySQL 5.7 and removed as of MySQL 8.0.

When using Joomla with the PDOMySQL driver and the Site Debug feature is enabled and an extension tries to close the system database connection (which forces it to reconnect) you get an uncatchable Fatal Error:

Fatal error: Uncaught PDOException: SQLSTATE[HY000]: General error: 1193 Unknown system variable 'query_cache_type' in /.../libraries/joomla/database/driver/pdomysql.php on line 160

Wrapping the code that recycles the MySQL connection in a try-catch doesn't work, even if the caught exception type is Throwable.

The only solution is to fix Joomla's database driver.

On Joomla 3 this is found in the following code locations:

This has already been fixed on Joomla 4.

I consider this a very high priority issue since there is no viable workaround in third party code (as I said, try/catch fails to catch the exceptions thrown by PDO). Not closing the connection is not an alternative, e.g. when we need to share the PDOMySQL resource with a third party database driver. Sharing the internal resource is desirable instead of using the connection information from JConfig to open a new connection because opening yet another MySQL connection from the same PHP process to the same server is disallowed on many live hosts.

I know that probably nobody will care enough to fix it. I am filing this issue just to have public proof that the Joomla project was made aware of the issue, its nature and its solution.

avatar nikosdion nikosdion - open - 12 Jan 2021
avatar joomla-cms-bot joomla-cms-bot - change - 12 Jan 2021
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 12 Jan 2021
avatar richard67
richard67 - comment - 12 Jan 2021

It was not corrected in J4, it is vice versa: The issue has never been created in J4. It came into J3 with PR #20322 .

Maybe the best would be just to revert the relevant part of that PR? I don't understand anyway what's so bad in having query cache on in debug mode when it is also on in not debug mode.

avatar nikosdion
nikosdion - comment - 12 Jan 2021

So, it's worse than what my cursory glance through the code already uncovered, huh? Joomla never ceases to surprise me, in this case very negatively.

Regarding your comment, I can understand the thought process behind that misguided PR, even though I don't agree with it at all. If you are on MySQL 5 and you have the query cache enabled you could plausibly see some skewed query timings if you are doing a lot of slow selects returning very small result sets from tables which don't change often. Of course if you are actually interested in doing some profiling on your database you should disable query cache on the server, not the client to begin with, like all of us profiling our queries routinely do. As a result this PR should have never been filed and for at least this reason it should have never been accepted.

Let alone the fact that this PR was accepted in 2018. This is FIVE (5) YEARS after MySQL 5.7, which deprecated the query cache, was released and TWO (2) YEARS since MySQL 8.0 which removed the query cache was released. People who supposedly know about how databases work allegedly went over the code and attached their successful tests, despite the fact that they are endorsing a feature which uses a MySQL feature which has been deprecated and subsequently removed from MySQL itself, doing that without even a MySQL version check! Someone else pushed that merge button posthaste with no further scrutiny or accountability.

The project policy of merging after receiving any two allegedly successful tests from any two random GitHub accounts was introduced circa 2012, if memory serves, as a code quality assurance policy. However, it runs counter to the stated goal for anything other than trivial and quick HTML output fixes. If it's anything low level or architectural things get routinely broken as a result of this policy and nobody is ultimately accountable. Not the person submitting the PR, not the people submitting their test results, not the maintainer that pressed that merge button. This is insane! This is a policy with far reaching implications, including security. I've been saying that for years. Anyway, I digress.

I have been able to work around this issue in my code by detecting if it's Joomla 3, PDOMySQL is being used and the MySQL version is 8.0 or later (or completely unknown in which case I assume the worst case scenario of 8.0 or later). In this case I just tell Joomla's DB driver to disable its debug mode which resolves the issue.

avatar zero-24 zero-24 - change - 13 Jan 2021
Status New Closed
Closed_Date 0000-00-00 00:00:00 2021-01-13 21:03:20
Closed_By zero-24
avatar zero-24 zero-24 - close - 13 Jan 2021
avatar zero-24
zero-24 - comment - 13 Jan 2021

Closing here now as there is now an PR: #32028 please comment and test there so we can get this fixed. Thanks!

avatar richard67
richard67 - comment - 13 Jan 2021

@zero-24 My PR is still in draft status because testing instructions need to be written, and for today I am too tired. So I suggest we leave this issue open until the PR is ready.

Update: I also think about modifying the test for availability of the query cache. So my PR is still work in progress.

avatar richard67 richard67 - change - 13 Jan 2021
Status Closed New
Closed_Date 2021-01-13 21:03:20
Closed_By zero-24
avatar richard67 richard67 - reopen - 13 Jan 2021
avatar richard67
richard67 - comment - 13 Jan 2021

Re-opening because PR is not ready yet.

avatar richard67
richard67 - comment - 17 Jan 2021

Closing as having a pull request. Please test #32028 (even if it might not be the solution you like). Thanks in advance.

avatar richard67 richard67 - close - 17 Jan 2021
avatar richard67 richard67 - change - 17 Jan 2021
Status New Closed
Closed_Date 0000-00-00 00:00:00 2021-01-17 15:29:21
Closed_By richard67

Add a Comment

Login with GitHub to post a comment