Address WordPress PHPUnit test fails: charset detection, length validation etc#331
Address WordPress PHPUnit test fails: charset detection, length validation etc#331chloe-pomegranate wants to merge 3 commits intoWordPress:trunkfrom
Conversation
…ation, capabilities
|
Hi @chloe-pomegranate, thanks for your contribution! It demonstrates that we're only a few small fixes away from improving the WP unit test compatibility. I left some feedback, mostly with ideas of better aligning with WPDB and what makes sense for SQLite. Feel free to address them, or I can take it over later on. Thanks again for bringing these to our attention! |
|
hey @JanJakes no prob, would be happy to! I can't see the review at the moment though, am I missing something? |
JanJakes
left a comment
JanJakes
left a comment
There was a problem hiding this comment.
@chloe-pomegranate Ups, sorry, missed the submit review button.
| * @return bool Whether the database feature is supported, false otherwise. | ||
| */ | ||
| public function has_cap( $db_cap ) { | ||
| return 'subqueries' === strtolower( $db_cap ); |
There was a problem hiding this comment.
Do we still need this method override at all? The capabilities are derived from the MySQL version, so they depend on the db_version() implementation (currently hardcoded to 8.0).
| case 'utf8mb4': | ||
| case 'utf8mb4_520': | ||
| case 'identifier_placeholders': | ||
| return true; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Added a version_compare for this one
| * To do so, mb_convert_encoding can be used with an optional | ||
| * fallback to a htmlspecialchars method. E.g.: | ||
| * https://github.com/nette/utils/blob/be534713c227aeef57ce1883fc17bc9f9e29eca2/src/Utils/Strings.php#L42 | ||
| */ |
There was a problem hiding this comment.
The TODO explains what should be done for SQLite. However, the implementation doesn't seem to follow it, and it removes all of that context. The TODO is there for a reason, and we should follow it.
| * Determines the best charset and collation for the database connection. | ||
| * | ||
| * This overrides wpdb::determine_charset() to handle SQLite's lack of mysqli. | ||
| * WordPress expects utf8 to be upgraded to utf8mb4 when supported. |
There was a problem hiding this comment.
Let's just state that this closely mirrors the implementation of the parent method, like we do for other methods, keeping the parent method docs otherwise.
| * @since 2.3.0 | ||
| * | ||
| * @return bool True if connection was closed successfully. | ||
| */ |
There was a problem hiding this comment.
Let's just state that this closely mirrors the implementation of the parent method, like we do for other methods, keeping the parent method docs otherwise.
There was a problem hiding this comment.
Added this
| $table_key = strtolower( $table ); | ||
| $column_key = strtolower( $column ); | ||
|
|
||
| // Check cached column metadata first. |
There was a problem hiding this comment.
The parent method ensures $this->col_meta is populated, but here it's missing.
Alternatively, we could also consider just calling the parent:
try {
$this->is_mysql = true; // We'd need to explain this hack.
return parent::get_col_length( $table, $column );
} finally {
$this->is_mysql = false;
}There was a problem hiding this comment.
Amended to go with this method
|
|
||
| // Use SHOW FULL COLUMNS to get column metadata with collation info. | ||
| // This works because the driver translates this to query the information schema. | ||
| $results = $this->get_results( "SHOW FULL COLUMNS FROM `{$table_key}`" ); |
There was a problem hiding this comment.
Is there a reason to implement this differently than WPDB does?
Alternatively, we could also consider just calling the parent:
try {
$this->is_mysql = true; // We'd need to explain this hack.
return parent::get_col_charset( $table, $column );
} finally {
$this->is_mysql = false;
}There was a problem hiding this comment.
Amended to go with this method
…apabilities | Amends from review
|
@JanJakes Made a few updates from your review! |
2 participants
Summary
Fixes (some of) #320 - WordPress PHPUnit test failures related to charset/collation handling, column length validation, and db capabilities
Changes Made
New Methods
get_col_charset($table, $column)- Returns charset for a table column by parsing the Collation field from SHOW FULL COLUMNS outputget_col_length($table, $column)- Returns column length information for validation, supporting char/varchar/binary/varbinary and TEXT/BLOBhas_cap($db_cap)- Reports supported capabilities: collation, group_concat, subqueries, set_charset, utf8mb4, utf8mb4_520, identifier_placeholdersdetermine_charset($charset, $collate)- Upgrades utf8 to utf8mb4 and handles collation mappingExisting Methods Changed
query()- Added strip_invalid_text_from_query() validation to reject queries with invalid data for the column charsetclose()- Properly sets ready=false and has_connected=false when closing the connectionTests
Fixes from the issue:
Other
Haven't touched these ones as they're generally all MySQL specific or SQLite handles them differently:
test_replace- MySQL REPLACE returns affected rows differently than SQLitetest_close- Similar to abovetest_mysqli_flush_sync- MySQL-specific, don't think we can do anything heretest_spatial_indices- SQLite doesn't support SPATIAL KEYtest_wp_update_commenttest_strip_invalid_texttest_set_charset_changes_the_connection_collationNotes
@since 2.3.0- can change to whatever :)