WIP PDO API #291
WIP PDO API #291
Conversation
JanJakes
force-pushed
the
pdo-api
branch
8 times, most recently
from
fd4cc59 to
c02c987
Compare
JanJakes
force-pushed
the
pdo-api
branch
9 times, most recently
from
12df654 to
9e8e453
Compare
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)).
| } | ||
|
|
||
| public function test_connection(): void { | ||
| $driver = new WP_PDO_MySQL_On_SQLite( 'mysql-on-sqlite:path=:memory:;dbname=WordPress;' ); |
| * | ||
| * The driver requires PDO with the SQLite driver, and the PCRE engine. | ||
| */ | ||
| class WP_PDO_MySQL_On_SQLite extends PDO { |
There was a problem hiding this comment.
Oh, so the connection driver is just this now and the other class only wraps it and translates PDO output into WordPress information. Really nice!
There was a problem hiding this comment.
Can we intentionally split this into two PRs where one would make this change directly in the class-wp-sqlite-connection.php and move the actual WP SQLite connection class to another file? Just so that we get to review what changes in the driver logic. Right now this file is 6,5k new lines and I'm not sure which ones overlap exactly with the previous driver and which ones were modified or added. Once such a PR is merged, we could swap the file names in a follow-up PR to end up with the useful naming scheme this PR proposes. That's a bit of GitHub hacking, but it should support us here.
There was a problem hiding this comment.
@adamziel Good idea. Now it can be viewed in a commit-by-commit fashion:
- Rename WP_SQLite_Driver to WP_PDO_MySQL_On_SQLite
- Reintroduce WP_SQLite_Driver as a temporary proxy exposing legacy API
But making an extra PR is even better. I already started extracting this PR into multiple smaller ones:
- Improve transaction behavior compatibility, add PDO-like APIs #294
- Fix missing table name column in
SHOW CREATE TABLEstatement #295 - Fix incorrect column names for string literals without alias #296
So I'll prepare even more.
There was a problem hiding this comment.
I'm still getting some reasonably recent search results when I google for "PHP Fatal error: Class 'PDO' not found". We might want to polyfill the core PDO class when it's missing.
There was a problem hiding this comment.
It's not the blocker here by the way, we could follow up with this. I don't think we'll get any problem reports early on but let's still take this into consideration
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).
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.
| */ | ||
|
|
||
| /** | ||
| * Some PDOStatement methods are not compatible across different PHP versions. |
| ) { | ||
| $this->pdo = $pdo; | ||
| $this->sqlite_column_meta = $sqlite_column_metadata; | ||
| $this->rows = $rows; |
There was a problem hiding this comment.
This implies we're always loading the full result set into memory. Do we have to do that? I worry we could blow up memory on some large queries.
There was a problem hiding this comment.
Do we have to do that?
@adamziel In most cases, fortunately not. But in some, when we construct the result in PHP (things like SHOW GRANTS, etc.), we need to have a "synthetic" statement that is implementing the PDO API on top of PHP arrays.
For all statements where a MySQL statement corresponds to an SQLite statement, it will be better to use a "proxy" statement. We can't use the SQLite statement directly because some things like column metadata formats differ, but we can use a proxy to avoid loading all results into memory. I actually have a class-wp-pdo-proxy-statement.php in a WIP commit, but I'll probably extract it to a follow-up PR. Here's why I think it's fine:
In the first step, we can use the synthetic statement for everything because 1) the current driver already stores everything in memory ($this->last_result, etc.), and 2) $wpdb loads everything in memory as well. So I think I can complete this PR with the "synthetic" statement, and then in the next PR, I would replace all the $this->last_something props with a single $this->last_result_statement that we would use as the proxy target.
WIP