fix(system): Database Tables list empty on MySQL 8.0.18#16855
fix(system): Database Tables list empty on MySQL 8.0.18#16855Ibochkarev wants to merge 11 commits intomodxcms:3.xfrom
Conversation
a0c8e85 to
c9d5a99
Compare
- Resolve database name via SELECT DATABASE() with config fallback for MySQL 8 - Cast Data_length, Data_free, Index_length to int to handle NULL in SHOW TABLE STATUS - Compute Effective_size as max(0, dataLength - dataFree) - Extract formatTableRow(), cache permission and manager_log table name before loop - Add type hints for formatTableRow parameters Resolves modxcms#14976
c9d5a99 to
bd6b08c
Compare
Code ReviewSummaryFixes #14976: Database Tables grid returns empty on MySQL 8.0.18. Three key changes: resolve DB name via Issues1. Other processors share the same
2. No null check on Inherited from the original code, but since the file is being refactored and $c = new xPDOCriteria($this->modx,
'SHOW TABLE STATUS FROM ' . $this->modx->escape($dbName));
if ($c->stmt === null) {
return [];
}
$c->stmt->execute();3. Silent empty return without logging if ($dbName === null || $dbName === '') {
return [];
}The user sees an empty grid with no indication of what went wrong — the same symptom as the original bug. Consider adding a log entry: if ($dbName === null || $dbName === '') {
$this->modx->log(modX::LOG_LEVEL_ERROR, '[DatabaseTable/GetList] Could not determine database name');
return [];
}Suggestions1. The docblock promises return (string) $this->modx->getOption('dbname') ?: null;2. MODX codebase conventionally uses AssessmentGood quality PR. The VerdictRequest changes — minor fixes needed (items 2–3 above, item 1 as follow-up issue). |
…processor - Added logging for cases where the database name cannot be determined. - Enhanced error handling by returning early if the statement is null. - Changed access modifiers for formatTableRow() and getDatabaseName() methods to protected for better extensibility.
|
One thing to note here is that the empty list problem will only be able to be verified with this specific version (and perhaps other early 8.x ones) of mysql. I'm running 8.0.44 in my test environment and cannot replicate the issue there. Nonetheless, there are a couple upsides to this PR—some safeguards, optimizations, and logging, as well as only formatting a column value with an optimization link when there is one to be done (e.g., in the overhead col). Will review further later today/tomorrow... |
There was a problem hiding this comment.
Just a few details I'd like to see covered to move this forward. Also, the following Lexicon entries would need to be added into system_info.inc.php in addition to the requested changes below:
$_lang['database_query_err_dbname_empty'] = 'Query for current database name returned an empty value.';
$_lang['database_query_err_table_stat'] = 'Query for TABLE STATUS from [[+db]] returned a null result.';Lastly, if anyone (aside from Ivan) can easily test this against mysql 8.0.18 that'd be ideal. I currently don't have a flexible setup on the mysql side to be able to check multiple minor versions.
Ivan: It's important to note that if you look at the original issue's conversation you'll find the problem may have had more to do with XPDO's parseDSN at the time than mysql itself. That and the issue was reported for MODX 2.x. There needs to be more focus when throwing these issues into the AI “blender” ;-)
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
|
@Ibochkarev Hey - just a reminder you'll need to add the Lexicon entries I mentioned above. |
Done |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #16855 +/- ##
============================================
+ Coverage 21.64% 21.66% +0.01%
- Complexity 10764 10793 +29
============================================
Files 566 566
Lines 33005 33171 +166
============================================
+ Hits 7144 7185 +41
- Misses 25861 25986 +125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
…ist.php Co-authored-by: Jim Graham <jim@pixelsandstrings.com>
What does it do?
SELECT DATABASE()with config fallback (MySQL 8 does not expose it inSHOW TABLE STATUSthe same way).Data_length,Data_free,Index_lengthto int to handle NULL/string values returned bySHOW TABLE STATUSon MySQL 8.Effective_sizeasmax(0, dataLength - dataFree)to avoid negative or invalid values.formatTableRow(), cacheshasPermission('settings')and manager_log table name before the loop; adds type hints for the helper.Why is it needed?
On MySQL 8.0.18, Reports → System Information → Database Tables returns an empty list because the processor relied on database name and numeric fields in ways that are no longer valid (different
SHOW TABLE STATUSbehavior and schema).How to test
core/vendor/bin/phpcs --standard=phpcs.xml core/src/Revolution/Processors/System/DatabaseTable/mysql/GetList.phpto ensure coding standards pass.Related issue(s)/PR(s)
Resolves #14976