Skip to content

Fix RAND() implementation and add plugin compatibility layer #340

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

Open
wp-fuse wants to merge 1 commit into
base: trunk
Choose a base branch
from
+84 −11
Open

Fix RAND() implementation and add plugin compatibility layer #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 56 additions & 0 deletions integrations/plugin-compatibility/boot.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php
/**
* Plugin compatibility layer.
*
* Provides string-level translation fallbacks for complex third-party plugin queries
* that are incompatible with the pure AST SQLite evaluator (e.g. Action Scheduler).
*/

if ( ! defined( 'ABSPATH' ) ) {
exit;
}

/**
* Filter SQL queries early to fix plugin compatibility issues.
*
* @param string $query The SQL query.
* @return string Modified query.
*/
function wp_sqlite_integration_plugin_compat( $query ) {
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

This is an unsafe hack that conflicts with the new driver architecture and goes against what it stands for. The new driver was created to shift from fragile, non-deterministic regex matching to fully analyzing complex database queries. If a specific MySQL construct is not yet supported, it needs to be fixed on the driver level. Don't hesitate to create an issue describing which query doesn't work, and we'll take a look at it.

if ( ! is_string( $query ) ) {
return $query;
}

// 1. Heavy cleaning of unsupported MySQL locking clauses.
// SQLite doesn't support FOR UPDATE, SKIP LOCKED, or NOWAIT anywhere.
// We strip these globally (case-insensitive, multi-line) to prevent syntax errors in subqueries.
if ( stripos( $query, 'FOR UPDATE' ) !== false ) {
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

This is implemented in the driver:

sqlite-database-integration/wp-includes/sqlite-ast/class-wp-pdo-mysql-on-sqlite.php

Line 3810 in bf6a7ac

case 'lockingClause':

Copy link
Copy Markdown
Author

@wp-fuse wp-fuse Mar 30, 2026

Choose a reason for hiding this comment

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

You're absolutely right. I wasn't fully aware it was already cleanly handled by the driver. I will be removing this entire regex file from the PR to stick to the intended architecture.

$query = preg_replace( '/\s+FOR\s+UPDATE(?:\s+(?:SKIP\s+LOCKED|NOWAIT))?\b/is', '', $query );
}

// 2. Action Scheduler specific compatibility fixes.
if ( stripos( $query, 'actionscheduler' ) !== false ) {
// Escape the 'group' keyword safely.
// Action Scheduler sometimes queries an unquoted `group` column.
$query = preg_replace( '/(?<![\'"`])\bgroup\b(?!\s+by)(?![\'"`])/i', '`group`', $query );

// Fix 'INSERT wp_actionscheduler...' syntax to include 'INTO'.
$query = preg_replace( '/INSERT\s+(?!INTO\s+)(wp_actionscheduler_[a-zA-Z0-9_]+)/i', 'INSERT INTO $1', $query );
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

Can you provide an example of a MySQL query that is failing here?

Copy link
Copy Markdown
Author

@wp-fuse wp-fuse Mar 30, 2026

Choose a reason for hiding this comment

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

We noticed Action Scheduler was occasionally firing INSERT table instead of INSERT INTO table and failing, but I will capture the exact raw queries that failed and open a separate Issue for the driver to handle them natively as you recommended. I'll drop this from the PR.


// Fix 'UPDATE ... JOIN' syntax manually.
// Handles variants like "UPDATE table t1 JOIN" and "UPDATE table AS t1 JOIN".
$pattern = '/UPDATE\s+([^\s]+)\s+(?:AS\s+)?t1\s+JOIN\s*\((.*?)\)\s*(?:AS\s+)?t2\s*ON\s*t1\.action_id\s*=\s*t2\.action_id\s*SET\s*(.*)/is';
if ( preg_match( $pattern, $query, $matches ) ) {
$set_clause = str_ireplace( 't1.', '', $matches[3] );
// Extract the SELECT logic from the join to use in an IN clause.
$query = "UPDATE {$matches[1]} SET {$set_clause} WHERE action_id IN ({$matches[2]})";
}
}
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

UPDATE with JOIN is supported by the driver:

sqlite-database-integration/wp-includes/sqlite-ast/class-wp-pdo-mysql-on-sqlite.php

Line 2112 in bf6a7ac

// Compose the FROM clause using all tables except the one being updated.

Copy link
Copy Markdown
Author

@wp-fuse wp-fuse Mar 30, 2026

Choose a reason for hiding this comment

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

Same as above, I’ll remove this entirely. I will test these plugin operations against the current AST driver natively and report any isolated failures we find via a dedicated Issue.


return $query;
}

if ( function_exists( 'add_filter' ) ) {
// Execute the compatibility fixes at priority 0 (before other generic manipulations).
add_filter( 'query', 'wp_sqlite_integration_plugin_compat', 0 );
}
11 changes: 5 additions & 6 deletions tests/WP_SQLite_Driver_Tests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9068,14 +9068,13 @@ public function testColumnInfoForExpressions(): void {
'mysqli:type' => 8,
),
array(
// TODO: Fix custom "RAND()" function to behave like in MySQL.
'native_type' => 'LONGLONG', // DOUBLE in MySQL.
'pdo_type' => PDO::PARAM_INT, // PARAM_STR in MySQL.
'native_type' => 'DOUBLE', // DOUBLE in MySQL.
'pdo_type' => PDO::PARAM_STR, // PARAM_STR in MySQL.
'flags' => array( 'not_null' ),
'table' => '',
'name' => 'col_expr_19',
'len' => 21, // 23 in MySQL.
'precision' => 0, // 31 in MySQL.
'len' => 23, // 23 in MySQL.
'precision' => 31, // 31 in MySQL.
'sqlite:decl_type' => '',

// Additional MySQLi metadata.
Expand All @@ -9084,7 +9083,7 @@ public function testColumnInfoForExpressions(): void {
'mysqli:db' => 'wp',
'mysqli:charsetnr' => 63,
'mysqli:flags' => 0, // 32769 in MySQL.
'mysqli:type' => 8, // 5 in MySQL.
'mysqli:type' => 5, // 5 in MySQL.
),
array(
'native_type' => 'LONGLONG',
Expand Down
14 changes: 12 additions & 2 deletions wp-includes/sqlite-ast/class-wp-pdo-mysql-on-sqlite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4287,10 +4287,14 @@ private function translate_regexp_functions( WP_Parser_Node $node ): string {
* be reasonably safe since PHP does not allow null bytes in
* regular expressions anyway.
*/
$pattern = $this->translate( $node->get_first_child_node() );
// Fix double backslash escaping for REGEXP patterns.
$pattern = str_replace( '\\\\/', '/', $pattern );

if ( true === $is_binary ) {
return 'REGEXP CHAR(0) || ' . $this->translate( $node->get_first_child_node() );
return 'REGEXP CHAR(0) || ' . $pattern;
}
return 'REGEXP ' . $this->translate( $node->get_first_child_node() );
return 'REGEXP ' . $pattern;
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

Could you, please, explain what this change is intended for and provide an example? It doesn't seem to be documented anywhere, and I don't think it's safe (simple str_replace will change all occurrences of a given sequence, even the ones where it's intentional).

Copy link
Copy Markdown
Author

@wp-fuse wp-fuse Mar 30, 2026

Choose a reason for hiding this comment

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

We were temporarily attempting to bypass some complex constraints that were failing on specific regex statements in third-party plugins (FlyingPress), but you are totally right that a blind string replacement is unsafe. I'll revert this undocumented change entirely to keep the AST parser clean!

}

/**
Expand Down Expand Up @@ -4366,6 +4370,12 @@ private function translate_function_call( WP_Parser_Node $node ): string {
}

switch ( $name ) {
case 'RAND':
if ( empty( $args ) ) {
return '(ABS(RANDOM()) / 9223372036854775808.0)';
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

Good idea using an SQLite expression when a seed is not provided!

Looking closer, we need to improve this a little bit:

  1. Calling ABS(-9223372036854775808) will throw an integer overflow error, as there is no corresponding positive number.
  2. Using ABS will cause the value of 0.0 occur half as likely as any other value (-n and n both map to the same value).

I think both of the issues above would be addressed by using (RANDOM() / 18446744073709551616.0 + 0.5). Or maybe even better, we can just clear the sign bit instead of ABS: ((RANDOM() & 0x7FFFFFFFFFFFFFFF) / 9223372036854775808.0). Additionally, we need to add a comment about what this snippet does and what the constant is.

Copy link
Copy Markdown
Author

@wp-fuse wp-fuse Mar 30, 2026

Choose a reason for hiding this comment

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

Excellent catch on the integer overflow mapping to 0.0! I will update it to use ((RANDOM() & 0x7FFFFFFFFFFFFFFF) / 9223372036854775808.0) and add an inline comment explaining the clearing of the sign bit and what the floating point constant represents. I'll include it in the next push.

}
// Seeded RAND() calls should be handled by the PHP UDF.
return $this->translate_sequence( $node->get_children() );
case 'DATE_FORMAT':
list ( $date, $mysql_format ) = $args;

Expand Down
11 changes: 8 additions & 3 deletions wp-includes/sqlite/class-wp-sqlite-pdo-user-defined-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,15 @@ public function md5( $field ) {
* This function uses mt_rand() which is four times faster than rand() and returns
* the random number between 0 and 1.
*
* @return int
* @param int|null $seed The seed value (optional).
*
* @return float
*/
public function rand() {
return mt_rand( 0, 1 );
public function rand( $seed = null ) {
if ( null !== $seed ) {
mt_srand( intval( $seed ) );
}
return mt_rand( 0, mt_getrandmax() ) / mt_getrandmax();
Copy link
Copy Markdown
Member

@JanJakes JanJakes Mar 30, 2026

Choose a reason for hiding this comment

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

I see two potential issues here:

  1. Global state pollution — mt_srand() affects all mt_rand() calls process-wide, potentially breaking other code in WordPress core or plugins.
  2. Wrong sequence — MySQL uses a specific LCG, not Mersenne Twister. Code depending on deterministic output from RAND(42) gets different results.

The UDF should replicate MySQL's actual algorithm with instance-local state. Something like:

private $rand_seed1 = null;
private $rand_seed2 = null;

public function rand( $seed = null ) {
    $max_value = 0x3FFFFFFF;
    if ( null !== $seed ) {
        $this->rand_seed1 = intval( $seed ) % $max_value;
        $this->rand_seed2 = $this->rand_seed1 * 3 + 1;
    }
    if ( null !== $this->rand_seed1 ) {
        $this->rand_seed1 = ( $this->rand_seed1 * 3 + $this->rand_seed2 ) % $max_value;
        $this->rand_seed2 = ( $this->rand_seed1 + $this->rand_seed2 + 33 ) % $max_value;
        return (float) $this->rand_seed1 / (float) $max_value;
    }
    return mt_rand( 0, mt_getrandmax() ) / mt_getrandmax();
}

Copy link
Copy Markdown
Author

@wp-fuse wp-fuse Mar 30, 2026

Choose a reason for hiding this comment

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

I'll use your snippet as a foundation, properly implement the LCG sequence logic to closely match MySQL's output, and run some math verifications against it before updating the PR.

}

/**
Expand Down
3 changes: 3 additions & 0 deletions wp-includes/sqlite/db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,7 @@

// Boot the Query Monitor plugin if it is active.
require_once dirname( __DIR__, 2 ) . '/integrations/query-monitor/boot.php';

// Boot the SQLite Plugin Compatibility Layer.
require_once dirname( __DIR__, 2 ) . '/integrations/plugin-compatibility/boot.php';
}
Loading