Skip to content

Conversation

@JanJakes
Copy link
Member

@JanJakes JanJakes commented Dec 5, 2025

This PR implements foundational PDO APIs and uses them in the existing WP_SQLite_Driver class.

The changes are best understood commit-by-commit, and they include the following:

  1. Make WP_PDO_MySQL_On_SQLite extend PDO.
  2. Implement PDO::exec() API.
  3. Implement PDO::query() API.
  4. Implement PDOStatement API for in-memory data (operating on PHP arrays) as WP_PDO_Synthetic_Statement. This is required for PDO::query() that must return a PDOStatement instance.
  5. Implement PDOStatement::fetch() and PDOStatement::fetchAll() with the most common PDO fetch modes.
  6. Implement PDO and PDOStatement getAttribute() and setAttribute() methods.
  7. Implement PDOStatement::setFetchMode().
  8. Fix some PDO compatibility issues across all supported PHP versions.

Synthetic vs. proxy PDO statement

This initial work implements only a WP_PDO_Synthetic_Statement that requires the full PDO statement result set to be loaded in memory. This makes it easier to implement a gradual transition to the PDO API (the current driver loads all result sets in memory as well) and it can support all statement types, including those that are composed on the PHP side.

In a follow-up work, it should be possible to transition most statement types to a proxy-like PDO statement that would internally use PDO SQLite statements directly, without eagerly fetching all data. The proxy will be required to address incompatibilities between SQLite and MySQL statements.

Next steps

Subsequent PRs will focus on the following:

  • Implement more of the PDO and PDOStatement APIs.
  • Implement a WP_PDO_Proxy_Statement as described above.
  • Simplify the SQLite driver state to only store a "last result statement" instead of the current list of properties.
  • Use the PDO API directly and remove the temporary proxy to the "old" driver API.

@JanJakes JanJakes force-pushed the pdo-api branch 8 times, most recently from fd4cc59 to c02c987 Compare December 10, 2025 19:48
@JanJakes JanJakes force-pushed the pdo-api branch 9 times, most recently from 12df654 to 9e8e453 Compare December 17, 2025 12:54
adamziel added a commit that referenced this pull request Dec 18, 2025
Extracting smaller chunks from
#291 so we
can review and merge them independently.

This PR includes the following changes:
- Removed support for nested transactions. MySQL doesn't support them.
- Implemented PDO-like APIs for start transaction, commit, and rollback.
- The PDO-like APIs differ from an equivalent SQL command in that they
always throw an exception in scenarios when a transaction was already
started or not yet started (depending on the method), irrespective of
the `ATTR_ERRMODE` setting.
- Polyfill `PDO::inTransaction()` on PHP < 8.4, where it is not reliable
([issue](https://bugs.php.net/bug.php?id=81227),
[PR](php/php-src#14268)).
adamziel added a commit that referenced this pull request Dec 18, 2025
Extracting smaller chunks from
#291 so we
can review and merge them independently.

This PR adds `Table` column to `SHOW CREATE TABLE` statements, [as per
MySQL behavior](https://onecompiler.com/mysql/447x6gea7).
adamziel added a commit that referenced this pull request Dec 18, 2025
Extracting smaller chunks from
#291 so we
can review and merge them independently.

This PR renames the current `WP_SQLite_Driver` to
`WP_PDO_MySQL_On_SQLite` and reintroduces the `WP_SQLite_Driver` as a
simple proxy over the new renamed class. It's done in these two steps
(first rename, then reintroduce) so that Git understands the rename and
presents history (hopefully) accurately.

The changes are better understood in a commit-by-commit view.

#### `WP_PDO_MySQL_On_SQLite` vs `WP_SQLite_Driver`
The "reintroduced" `WP_SQLite_Driver` is not meant to be permanent. It
is a temporary proxy so we can gradually modify `WP_PDO_MySQL_On_SQLite`
to support PDO APIs while not touching the driver API just yet. Once the
basics of PDO API are in place, we can make all dependencies use the new
class directly and then remove the `WP_SQLite_Driver`.

That is, in the future, the `WP_SQLite_DB extends wpdb` will use
directly the new `WP_PDO_MySQL_On_SQLite` class.
@JanJakes JanJakes force-pushed the pdo-api branch 2 times, most recently from 5fe6619 to 4aadd4e Compare December 18, 2025 16:05

// Create a new SQLite connection.
if ( isset( $options['pdo'] ) ) {
$this->connection = new WP_SQLite_Connection( array( 'pdo' => $options['pdo'] ) );
Copy link
Collaborator

@adamziel adamziel Jan 17, 2026

Choose a reason for hiding this comment

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

Hm, so we can pass both the dsn and $options['pdo']? And the dsn is still validated and parsed in that case? Shouldn't we throw an error when both are passed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel This allows passing the underlying PDO SQLite instance, not the one we're parsing the options for.

While in theory, it can make some sense to support it (new WP_PDO_MySQL_On_SQLite( 'mysql-on-sqlite:dbname=test', [ 'pdo' => $my_pdo ] )), I'm not sure if we should keep it, and if we do, a better name for that option would probably make sense.

*
* From MySQL documentation:
*
* In the absence of the SQL_CALC_FOUND_ROWS option in the most
Copy link
Collaborator

@adamziel adamziel Jan 17, 2026

Choose a reason for hiding this comment

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

I hope noone out there actually relies on that behavior, but some plugins probably do. Obligatory xkcd: xkcd

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel True true 😬😂 I took a stab at it here: 3c8543b

I think that implementation does the > 99% case.

It's not 100%, because calling FOUND_ROWS() multiple times in a row will not necessarily give correct results. And then I also discovered this:

I also found this strange nuance:
Screenshot 2026-01-19 at 11 24 55

But we never supported these edge cases anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wow. Good find!

Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

I only have nitpicks. Really good work here, it's exciting to see how this is coming together. If you asked me a few years ago, I would have never guessed we'd be doing anything like this and yet here we are ❤️

JanJakes and others added 2 commits January 19, 2026 16:19
@JanJakes
Copy link
Member Author

@adamziel Thanks for the great review, as always. Another round should be ready now ✅

@JanJakes JanJakes requested a review from adamziel January 19, 2026 15:35
Comment on lines 2899 to 2900
$this->quote_sqlite_identifier( $statistics_table ),
$this->connection->quote( $this->get_saved_db_name( $database ) ),
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting, why do we need two quoting functions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

aha, one is for identifiers and another is for string values, and also this is no longer a prepared statement? Why ditch prepared statements here?

Copy link
Collaborator

@adamziel adamziel Jan 19, 2026

Choose a reason for hiding this comment

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

This would communicate the data flow right away:

$this->quote_sqlite_identifier( $statistics_table ),
$this->quote_sqlite_value( $this->get_saved_db_name( $database ) );

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel I guess it evolved into this state over time, and it's a bit confusing indeed:

  • Identifier: Private method on the driver class.
  • Value: Public method on the connection class.

What you suggest is much clearer 👍, and it has additional advantages:

  • It will clearly differentiate the methods from WP_PDO_MySQL_On_SQLite::quote(), which is required by the PDO API, but it's intended to be MySQL-like quoting, not SQLite.
  • We may consider getting rid of WP_SQLite_Connection in the future. I'm not sure if such a small wrapper around the PDO SQLite connection is necessary, probably not anymore.

So I can add a commit with these changes 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Why ditch prepared statements here?

@adamziel This was a result of a quick attempt to solve the FOUND_ROWS() without SQL_CALC_FOUND_ROWS functionality. I needed to store the last SQLite query that returns some rows in order to be able to wrap it with SELECT COUNT ... in case FOUND_ROWS() is used. For SELECT it was straightforward, but then I realized we also need SHOW and DESCRIBE and the quick attempt was to make all the queries just strings that I can then use in a subquery.

That said, it's not strictly necessary to ditch prepared statements—I could simply store both the query string and the parameters. Do you think I should revert those changes and do it this way instead?

Copy link
Collaborator

@adamziel adamziel Jan 21, 2026

Choose a reason for hiding this comment

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

I'm always nervous about formatting strings instead of using prepared statements, considering how different encodings represent bytes. We may have a watertight implementation here, but it still stands out to me as a risky spot in the system. I wouldn't say it's blocking this PR, but if it's an easy change I'd go for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel I already did (last two commits), but forgot to note it here.

The test failures are unrelated, I hotfixed that in #306.

@adamziel
Copy link
Collaborator

Lovely work @JanJakes! This one looks great to me, feel free to merge anytime.

@JanJakes JanJakes merged commit a2fcb8e into develop Jan 21, 2026
14 of 17 checks passed
@JanJakes JanJakes deleted the pdo-api branch January 21, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants