Conversation
|
This is amazing! are we finally moving to the new driver as default? |
|
+1 to this! Would solve a problem for me when using https://github.com/Automattic/studio for local dev Thanks for your efforts! |
aff0359 to
283fb3d
Compare
283fb3d to
eaecbf3
Compare
eaecbf3 to
68d63c3
Compare
Remove the legacy, token-based MySQL-to-SQLite translator and its supporting files. The new AST-based driver fully replaces it. Removed files: - class-wp-sqlite-translator.php - class-wp-sqlite-lexer.php - class-wp-sqlite-query-rewriter.php - class-wp-sqlite-token.php - class-wp-sqlite-pdo-user-defined-functions.php - php-polyfills.php
Remove test files that tested the legacy translator, along with the test bootstrap, schema helper, and root phpunit.xml.dist. The new driver tests live in packages/mysql-on-sqlite/. Removed files: - tests/WP_SQLite_Translator_Tests.php - tests/WP_SQLite_Query_Tests.php - tests/WP_SQLite_Metadata_Tests.php - tests/WP_SQLite_Query_RewriterTests.php - tests/WP_SQLite_PDO_User_Defined_Functions_Tests.php - tests/bootstrap.php - tests/wp-sqlite-schema.php - phpunit.xml.dist
39c1e6a to
2e30d92
Compare
The AST-based driver is now the only driver. Remove all conditional logic that branched between the new and legacy driver: - db.php: Always load the new driver. - class-wp-sqlite-db.php: Remove WP_SQLite_Translator instanceof checks, simplify all methods to use WP_SQLite_Driver directly. - install-functions.php: Always use WP_SQLite_Driver. - admin-page.php: Remove "(AST)" suffix from admin bar. - constants.php: Remove WP_SQLITE_AST_DRIVER env var handling. - wp-setup.sh: Remove WP_SQLITE_AST_DRIVER env vars from Docker. - wp-mysql-proxy.php: Remove WP_SQLITE_AST_DRIVER define. - phpcs.xml.dist: Remove legacy translator exclusion.
- Remove the "New and old driver" section from AGENTS.md - Remove stale plugin test commands from README.md (the root-level "composer run test" was removed along with the legacy driver) - Remove PHPMyAdmin attribution from readme.txt (the PHPMyAdmin/sql-parser code was only used by the deleted legacy lexer)
Remove the "Run PHPUnit tests for the legacy driver" step that ran root-level phpunit.xml.dist (now deleted). Also remove the unused phpunit-config input parameter and PHPUNIT_CONFIG env var that were only needed to switch between driver configs.
The filter-based fallback to 'database_name_here' was added to ease early adoption of the new AST-based driver in projects that didn't define DB_NAME. With the legacy driver removed, simplify to a plain ternary — matching how WPDB itself handles the constant.
2e30d92 to
1437789
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy token-based MySQL→SQLite translator (and its tests/tooling) and eliminates the WP_SQLITE_AST_DRIVER feature flag so the AST-based driver becomes the only supported implementation across the repo.
Changes:
- Deletes legacy driver implementation + legacy PHPUnit test suite/configuration.
- Removes
WP_SQLITE_AST_DRIVERconditionals across the WordPress plugin and MySQL proxy tooling, defaulting to the AST driver. - Simplifies CI/workflow inputs and updates docs/setup scripts to match the single-driver world.
Reviewed changes
Copilot reviewed 25 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
wp-setup.sh |
Removes WP_SQLITE_AST_DRIVER env injection from local wp-env overrides. |
tests/wp-sqlite-schema.php |
Removes legacy test schema file. |
tests/WP_SQLite_Query_Tests.php |
Removes legacy query tests targeting the translator. |
tests/WP_SQLite_Query_RewriterTests.php |
Removes legacy query rewriter unit tests. |
tests/WP_SQLite_PDO_User_Defined_Functions_Tests.php |
Removes legacy UDF unit tests. |
tests/WP_SQLite_Metadata_Tests.php |
Removes legacy information_schema/metadata tests targeting translator behavior. |
tests/bootstrap.php |
Removes legacy PHPUnit bootstrap and polyfills used by deleted tests. |
README.md |
Updates docs to remove legacy plugin PHPUnit test commands; keeps E2E guidance. |
phpunit.xml.dist |
Removes root PHPUnit config (legacy suite). |
phpcs.xml.dist |
Drops PHPCS exclude for the deleted legacy translator file. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/php-polyfills.php |
Removes legacy PHP 8.0 polyfills (no longer needed by deleted code). |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/install-functions.php |
Uses WP_SQLite_Driver unconditionally during schema creation. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/db.php |
Always loads the new driver loader; removes feature-flag logic and DB_NAME fallback behavior. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-token.php |
Deletes legacy lexer token model. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-query-rewriter.php |
Deletes legacy query rewriter. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-pdo-user-defined-functions.php |
Deletes legacy UDF implementation used by the translator. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-lexer.php |
Deletes legacy lexer implementation. |
packages/plugin-sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-db.php |
Removes translator branches; assumes WP_SQLite_Driver everywhere. |
packages/plugin-sqlite-database-integration/readme.txt |
Removes outdated licensing note for PHPMyAdmin/sql-parser code that’s no longer shipped. |
packages/plugin-sqlite-database-integration/constants.php |
Removes env-var-based enabling of WP_SQLITE_AST_DRIVER. |
packages/plugin-sqlite-database-integration/admin-page.php |
Removes “(AST)” suffix since AST is now the only driver. |
packages/mysql-proxy/bin/wp-mysql-proxy.php |
Removes hard-coded WP_SQLITE_AST_DRIVER define. |
composer.json |
Removes root test script (no root PHPUnit suite anymore). |
AGENTS.md |
Updates contributor docs to remove legacy driver references and plugin PHPUnit test commands. |
.github/workflows/phpunit-tests.yml |
Removes unused workflow parameter wiring for phpunit config. |
.github/workflows/phpunit-tests-run.yml |
Removes unused phpunit-config input/env and legacy-driver CI step. |
Comments suppressed due to low confidence (1)
packages/plugin-sqlite-database-integration/wp-includes/sqlite/install-functions.php:58
- This catch block indexes into
$err->errorInfo, butWP_SQLite_Driver_Exception(thrown by the new driver) extendsPDOExceptionwithout necessarily settingerrorInfo. On such failures (e.g. parse errors), this can trigger warnings and lose the real error context. Consider falling back to$err->getMessage()(and/or$err->getCode()) whenerrorInfois missing, and only indexingerrorInfowhen it’s an array with expected offsets.
} catch ( PDOException $err ) {
$err_data = $err->errorInfo; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
$err_code = $err_data[1];
$translator->rollback();
$message = sprintf(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@joaopedrofrech Yes, coming soon 🙂 |
| if ( defined( 'SQLITE_DB_DROPIN_VERSION' ) && defined( 'DB_ENGINE' ) && 'sqlite' === DB_ENGINE ) { | ||
| $suffix = defined( 'WP_SQLITE_AST_DRIVER' ) && WP_SQLITE_AST_DRIVER ? ' (AST)' : ''; | ||
| $title = '<span style="color:#46B450;">' . __( 'Database: SQLite', 'sqlite-database-integration' ) . $suffix . '</span>'; | ||
| $title = '<span style="color:#46B450;">' . __( 'Database: SQLite', 'sqlite-database-integration' ) . '</span>'; |
There was a problem hiding this comment.
It would be nice for this to still say (AST) as suffix.
There was a problem hiding this comment.
Hmm, the problem is I think most users won't know what that means. It made some sense when it was a feature flag, but otherwise, I'd say it's an implementation detail of the parser.
There was a problem hiding this comment.
I agree, it's an implementation detail. Users who were not using AST would now be using it in future with the newer version of plugin as soon as they update, right? So with that context in mind and personally, as a user I would like to see it.
But feel free to choose what you think is best.
|
|
||
| The SQLite plugin is a community, feature plugin. The intent is to allow testing an SQLite integration with WordPress and gather feedback, with the goal of eventually landing it in WordPress core. | ||
|
|
||
| This feature plugin includes code from the PHPMyAdmin project (specifically parts of the PHPMyAdmin/sql-parser library), licensed under the GPL v2 or later. More info on the PHPMyAdmin/sql-parser library can be found on [GitHub](https://github.com/phpmyadmin/sql-parser). |
There was a problem hiding this comment.
This is a general correction, not related to this PR, right?
There was a problem hiding this comment.
It's related, actually. The legacy driver used a simple lexer/parser extracted from PHPMyAdmin. The new driver doesn't use any of this.
| if ( defined( 'DB_NAME' ) && '' !== DB_NAME ) { | ||
| $db_name = DB_NAME; | ||
| } else { | ||
| $db_name = apply_filters( 'wp_sqlite_default_db_name', 'database_name_here' ); |
There was a problem hiding this comment.
I think we should consider using apply_filters_deprecated() with an empty replacement and a message explaining the "why" instead of silently failing.
apply_filters_deprecated() internally calls _deprecated_hook(), which in turn calls trigger_error() with E_USER_DEPRECATED.
There was a problem hiding this comment.
Oh, this was actually a forgotten temporary fix for Studio. It's not an old filter or anything that would be used originally; it was added only for the new driver as a quick fix until it was superseded by a better solution and forgotten. It had no effect on the old driver.
There was a problem hiding this comment.
I think as a general rule, if something gets added to public api, we should treat the depreciation gracefully. But if you are sure, this couldn't have been used by anyone other than Studio, totally fine to make an exception to that rule.
| require_once __DIR__ . '/class-wp-sqlite-token.php'; | ||
| require_once __DIR__ . '/class-wp-sqlite-pdo-user-defined-functions.php'; | ||
| } | ||
| require_once __DIR__ . '/../database/load.php'; |
There was a problem hiding this comment.
Nit: In /../ cases, the parent directory probably has significance and is usually better to define in a constant and then used in the path.
There was a problem hiding this comment.
Good point, it actually hints at what the directory structure looks like, and I think it will need a follow-up PR. It looks like this:
plugin-sqlite-database-integration/
- wp-includes/ # This mirros WP structure for possible WP Core PR
- database/ # This is a symlink to the new driver package sources
- sqlite/ # This has plugin-specific files that are needed even with the new driver.
# I think a follow-up PR should move out of wp-includes, or place them
# wherever it will make sense. There is also chance for further cleanup.Follow up items for consideration from reviewing #358
Summary
Remove the legacy, token-based MySQL-to-SQLite translator and the `WP_SQLITE_AST_DRIVER` feature flag. The AST-based driver is now the only driver.
Changes
Test plan