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.

Sign up for GitHub

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

Fix some WP unit test errors #32

Merged
merged 2 commits into from
Mar 25, 2025
Merged

Fix some WP unit test errors #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91ca58f
Fix ReflectionException in PHPUnit tests
JanJakes Mar 21, 2025
ad3beb8
Pass $dbname into WP_SQLite_DB
JanJakes Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 2 additions & 18 deletions .github/workflows/wp-tests-phpunit-run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,11 @@ const fs = require( 'fs' );
const path = require( 'path' );

const expectedErrors = [
'Tests_Admin_wpSiteHealth::test_object_cache_default_thresholds_non_multisite',
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #0',
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #1',
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #2',
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #3',
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #4',
'Tests_Comment_WpComment::test_get_instance_should_succeed_for_float_that_is_equal_to_post_id',
'Tests_Cron_getCronArray::test_get_cron_array_output_validation with data set "null"',
'Tests_DB_Charset::test_strip_invalid_text',
'Tests_DB::test_db_reconnect',
'Tests_DB::test_get_col_info',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "escaped-false-1"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "escaped-false-2"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "escaped-true-1"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "escaped-true-2"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "format-false-1"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "format-false-2"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "format-true-1"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "format-true-2"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "numbered-false-1"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "numbered-false-2"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "numbered-true-1"',
'Tests_DB::test_prepare_should_respect_the_allow_unsafe_unquoted_parameters_property with data set "numbered-true-2"',
'Tests_DB::test_process_fields_value_too_long_for_field with data set "invalid chars"',
'Tests_DB::test_process_fields_value_too_long_for_field with data set "too long"',
'Tests_DB::test_process_fields',
Expand All @@ -45,6 +27,8 @@ const expectedErrors = [
];

const expectedFailures = [
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #2',
'Tests_Admin_wpSiteHealth::test_object_cache_thresholds with data set #3',
'Tests_Comment::test_wp_new_comment_respects_comment_field_lengths',
'Tests_Comment::test_wp_update_comment',
'Tests_DB_dbDelta::test_spatial_indices',
Expand Down
4 changes: 2 additions & 2 deletions wp-includes/sqlite/class-wp-sqlite-crosscheck-db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

class WP_SQLite_Crosscheck_DB_ extends WP_SQLite_DB {

public function __construct() {
parent::__construct();
public function __construct( string $dbname ) {
parent::__construct( $dbname );
$GLOBALS['sqlite'] = $this;
$GLOBALS['mysql'] = new wpdb(
DB_USER,
Expand Down
47 changes: 43 additions & 4 deletions wp-includes/sqlite/class-wp-sqlite-db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,24 @@ class WP_SQLite_DB extends wpdb {
protected $dbh;

/**
* Constructor
* Backward compatibility, see wpdb::$allow_unsafe_unquoted_parameters.
*
* Unlike wpdb, no credentials are needed.
* This property is mirroring "wpdb::$allow_unsafe_unquoted_parameters",
* because some tests are accessing it externally using PHP reflection.
*
* @var
*/
private $allow_unsafe_unquoted_parameters = true;

/**
* Connects to the SQLite database.
*
* Unlike for MySQL, no credentials and host are needed.
*
* @param string $dbname Database name.
*/
public function __construct() {
parent::__construct( '', '', '', '' );
public function __construct( $dbname ) {
parent::__construct( '', '', $dbname, '' );
$this->charset = 'utf8mb4';
}

Expand Down Expand Up @@ -289,6 +301,33 @@ public function check_connection( $allow_bail = true ) {
return true;
}

/**
* Prepares a SQL query for safe execution.
*
* See "wpdb::prepare()". This override only fixes a WPDB test issue.
*
* @param string $query Query statement with `sprintf()`-like placeholders.
* @param array|mixed $args The array of variables or the first variable to substitute.
* @param mixed ...$args Further variables to substitute when using individual arguments.
* @return string|void Sanitized query string, if there is a query to prepare.
*/
public function prepare( $query, ...$args ) {
/*
* Sync "$allow_unsafe_unquoted_parameters" with the WPDB parent property.
* This is only needed because some WPDB tests are accessing the private
* property externally via PHP reflection. This should be fixed WP tests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh boy 🙈

As a side note, I found myself not trusting $wpdb->prepare() at all in my code. It seems weird to the point where I just stitch strings myself. I'm curious if that was your experience, too. I wonder if we have an opportunity here to make it better for SQLite and, e.g., actually use prepared statements. Probably not, because $wpdb is supposed to return a string here, but it would be cool if that was possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamziel I haven't explored $wpdb->prepare() in depth yet, but I do think it's potentially problematic — not only possible performance problems (roundtrips to MySQL server for escaping), but also the fact that it will consume any input, no matter whether it's a valid query or not, which may lead to misuse. And on top of that, you have things like this, which make me even less confident about that approach.

Considering this, I think WordPress deserves better, and maybe we could apply our learnings from implementing the SQLite driver, prototype a Postgres driver, and then see what improvements we could propose upstream.

As for what we can do with the SQLite driver itself, we only control the _real_escape WPDB method. I can see that historically, it seems to call addslashes, which I think we should replace with PDO->quote(), which should have no roundtrip cost since it's just a method call with SQLite. I'll check if using that breaks any tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I realized the addslashes part is correct because in that place, we emulate MySQL escaping, while escaping for SQLite is done later, when translating the literals.

Copy link
Contributor

@adamziel adamziel Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping with slashes is difficult when we don't know the encoding of the input string. Is the byte 34 a double quote? Or is it a part of a multibyte character that we didn't recognize? Maybe we could call the method bind_utf8_string() and drop support for other encodings, then it's easy. I've been playing with escaping via bin2hex($string) in php and UNHEX() in the query. It's more computations, but it may still be faster than relying on real escape.

*/
$wpdb_allow_unsafe_unquoted_parameters = $this->__get( 'allow_unsafe_unquoted_parameters' );
if ( $wpdb_allow_unsafe_unquoted_parameters !== $this->allow_unsafe_unquoted_parameters ) {
$property = new ReflectionProperty( 'wpdb', 'allow_unsafe_unquoted_parameters' );
$property->setAccessible( true );
$property->setValue( $this, $this->allow_unsafe_unquoted_parameters );
$property->setAccessible( false );
}

return parent::prepare( $query, ...$args );
}

/**
* Performs a database query.
*
Expand Down
4 changes: 2 additions & 2 deletions wp-includes/sqlite/db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
$crosscheck_tests_file_path = dirname( __DIR__, 2 ) . '/tests/class-wp-sqlite-crosscheck-db.php';
if ( defined( 'SQLITE_DEBUG_CROSSCHECK' ) && SQLITE_DEBUG_CROSSCHECK && file_exists( $crosscheck_tests_file_path ) ) {
require_once $crosscheck_tests_file_path;
$GLOBALS['wpdb'] = new WP_SQLite_Crosscheck_DB();
$GLOBALS['wpdb'] = new WP_SQLite_Crosscheck_DB( DB_NAME );
} else {
$GLOBALS['wpdb'] = new WP_SQLite_DB();
$GLOBALS['wpdb'] = new WP_SQLite_DB( DB_NAME );
}