-
Notifications
You must be signed in to change notification settings - Fork 1
Add support for the new SQLite driver #10
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.
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
Conversation
aa27915
to
464ae33
Compare
464ae33
to
275b382
Compare
@JanJakes, thanks for adding support for AST to the SQLite command. I used those steps to test it in Studio:
I couldn't make it fully work yet. I needed to make a few changes, see inline comments. |
src/SQLiteDriverFactory.php
Outdated
*/ | ||
public static function create_driver() { | ||
$new_driver_supported = class_exists( WP_SQLite_Driver::class ); | ||
$new_driver_enabled = $new_driver_supported && defined( 'WP_SQLITE_AST_DRIVER' ) && WP_SQLITE_AST_DRIVER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests, DB_NAME
and WP_SQLITE_AST_DRIVER
constants are undefined. I needed to hardcode the first one as database_name_here
and $new_driver_enabled
as true
for testing purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojtekn The "correct" value of the database name will be needed with wp sqlite import
because CREATE TABLE
statements need to write it in the information schema. Are we in a context where importing the wp-config.php
of a given site would be conceivable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command runs on before_wp_load
, so wp-config.php
is not loaded. See #7 for some context.
Perhaps we could load it manually, similar to how WP CLI runner already does?
eval( WP_CLI::get_runner()->get_wp_config_code() );
Or alternatively, to limit the amount of loaded code, what if we used the WP CLI parameter instead of the WP_SQLITE_AST_DRIVER
constant to enable the new driver? Like --driver=[ast|legacy]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or alternatively, to limit the amount of loaded code, what if we used the WP CLI parameter instead of the WP_SQLITE_AST_DRIVER constant to enable the new driver? Like --driver=[ast|legacy]?
@wojtekn This would work for the AST/legacy and would be temporary anyway. But what about the DB_NAME
? 🤔
There's this edge-case scenario:
- Someone imports a DB (we use some DB name during that process).
- Then a plugin code runs
SELECT * FROM information_schema.tables WHERE table_schema = 'my-db-name' AND ...
.
In point 2, we'd like the DB name to match the DB_NAME
that was set during the import.
By the way, is Playground booted anywhere before the import command is run? Because we know that site exports without DB_NAME
exist, and the fallback to inject their value in wp-config.php
is done in Playground at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojtekn Looking at wp db import
, it seems to me they're using after_wp_config_load
on the class level, and no other annotation for the import
command. A couple of commands use after_wp_load
.
Also explained here: https://developer.wordpress.org/cli/commands/db/import/
This command runs on the after_wp_config_load hook, after wp-config.php has been loaded into scope. Runs SQL queries using DB_HOST, DB_NAME, DB_USER and DB_PASSWORD database credentials specified in wp-config.php. This does not create database by itself and only performs whatever tasks are defined in the SQL.
Maybe we could use after_wp_config_load
for the import
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JanJakes, it makes sense to try with the after_wp_config_load
for the import
command, especially since it would be consistent with wp db import
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojtekn I pushed that change, let me know if it works for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JanJakes I realized I was testing Export above, so the change doesn't fix the behavior. When I set after_wp_config_load
for export
command, it reads DB_NAME from wp-config.php
correctly.
However, it still doesn't read WP_SQLITE_AST_DRIVER
that is set in Studio through defineWpConfigConsts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WP_SQLITE_AST_DRIVER
constant set in Studio through defineWpConfigConsts
enables the driver for the Studio site running in a browser.
It doesn't work for sqlite export
as it is executed through a non-Playground process. Would it make sense to use --driver=[ast|legacy]
for that part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojtekn Great! Let's use after_wp_config_load
for all the commands, then? As for AST, yes, let's make it configurable on the CLI 👍
@wojtekn I pushed some changes to fix all the issues, except for |
I pushed a few more changes:
|
I reverted |
This ensures the DB_NAME constant from "wp-config.php" is used, and aligns these commands with similar "wp db" commands. With WordPress/sqlite-database-integration#213, the correctness of the database name will also be verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works great with sqlite database integration 2.2.3 and this Studio PR Automattic/studio#1506
This PR adds support for the new SQLite driver.
When the new SQLite driver is available and enabled, it will execute all the MySQL queries using the new driver. Otherwise, the legacy SQLite driver will be used.