Open wojtekn opened 6 months ago
In the SQLite plugin we should either:
@@session.max_allowed_packet
, and then substitute it for something SQLite understand. Upside: It would work for all queries. Downside: Consuming a token could potentially consume three, if @@session
, .
, and max_allowed_packet
are all separate tokens. Because of that, it could be better to...strcspn
as strings should be replaced with :parameters
at that stage, although that requires double checking if @
signs can be legally used outside of strings and variable names.In v1 we could use a hardcoded mapping of variable -> value. In v2 we could create a SQLite database table and source the data from there, also supporting SET
. Would you start a new issue / pr in that repo @brandonpayton? CC @aristath
I created https://github.com/WordPress/sqlite-database-integration/issues/104 for this and added some notes over the weekend. Planning to work on this today.
I ended up fixing the unit tests for sqlite-database-integration today before adding a test to prove these queries are failing.
After adding failing test(s), my general plan is:
After that, we can consider allowing SET to simply modify this mapping. This seems straightforward for @@SESSION
variables because they are scoped to the current connection, but for @@GLOBAL
and @@PERSIST
variables, maybe it would make sense to actually consider some kind of persistence.
NOTE: It turns out @@PERSIST vars are "not permitted in expressions", which I believe includes expressions within SELECT statements based on this doc: https://dev.mysql.com/doc/refman/8.0/en/set-variable.html#variable-references-in-expressions
Started a PR with failing tests for this issue here: https://github.com/WordPress/sqlite-database-integration/pull/109
As a v1, let's start with support for the specific variables used by UpdraftPlus, with an eye to broaden support over time.
The set I found in a recent checkout of UpdraftPlus is:
@@character_set_client
@@character_set_results
@@collation_connection
@@GLOBAL.gtid_purged
@@GLOBAL.log_bin
@@GLOBAL.log_bin_trust_function_creators
@@GLOBAL.sql_mode
@@SESSION.max_allowed_packet
@@SESSION.sql_mode
It turns out that sqlite-database-integration already had a hack to avoid failures for queries for @@SESSION.sql_mode
:
https://github.com/WordPress/sqlite-database-integration/blob/23ed2215dd468a4d9d5f488ecefa66d0118efc50/wp-includes/sqlite/class-wp-sqlite-translator.php#L1455-L1466
} elseif (
strpos( $updated_query, '@@SESSION.sql_mode' ) !== false
|| strpos( $updated_query, 'CONVERT( ' ) !== false
) {
/*
* If the query contains a function that is not supported by SQLite,
* return a dummy select. This check must be done after the query
* has been rewritten to use parameters to avoid false positives
* on queries such as `SELECT * FROM table WHERE field='CONVERT('`.
*/
$updated_query = 'SELECT 1=0';
$params = array();
It would be great to have actual support for working with some of these vars, but as a v0, maybe we should just broaden the condition to match all @@GLOBAL.vars
and @@SESSION.vars
.
I don't yet know how to test dev version of sqlite-database-integration with Playground but plan to find out and test with this change: https://github.com/WordPress/sqlite-database-integration/pull/109/commits/21b0ad2066acf20d927009dbd55abe330fe085dd
I tested with the above workaround, and there are no more query errors. Of course, faking the query is not the same as the real thing, but perhaps avoiding the errors in the meantime is a net gain.
There is also JS errors displayed Uncaught ReferenceError: updraftlion is not defined in the console. Clicking different buttons triggers more errors like Uncaught ReferenceError: updraft_backup_dialog_open is not defined, but I'm not sure if those are related to the Playground environment or the plugin itself.
@wojtekn I tested UpdraftPlus on a WordPress.com site, and am getting the same errors. There are no query errors, but the same JS errors remain so that it is not possible to trigger a backup from the admin page you linked to.
The sqlite-database-integration plugin has been updated to at least tolerate selecting MySQL system vars, and Playground will pick up the changes when rebuilding WordPress. Will see if I can trigger that workflow rather than waiting on a schedule.
@wojtekn with the latest changes to sqlite-database-integration, there are no more SQL var errors here (blueprint with updraft plus and the admin page).
Since the WordPress zips have been updated and deployed with the latest sqlite-database-integration, I believe this means that wp-now and Studio get the fix when doing new WP downloads. Is that correct?
I believe this can be closed now, great work @brandonpayton! @wojtekn feel free to reopen if any other issues come up.
Thanks @brandonpayton !
Since the WordPress zips have been updated and deployed with the latest sqlite-database-integration, I believe this means that wp-now and Studio get the fix when doing new WP downloads. Is that correct?
Currently, we prepackage the SQLite plugin with Studio, so it won't be automatically updated for current installations. The new Studio release will include the new SQLite version. We are going to change it under https://github.com/Automattic/dotcom-forge/issues/6939
I've noted that the ReferenceError: Can't find variable: updraftlion
error remains presents in some circumstances. It appears a different SQL query error occurs during the backup and then leads to the aforementioned reference error in the browser.
0001.896 (0) PHP event: code E_WARNING: Undefined variable $entries (line 3373, wp-content/mu-plugins/sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-translator.php)
0001.897 (0) PHP Fatal error (TypeError) has occurred. Error Message: implode(): Argument #1 ($pieces) must be of type array, string given (Code: 0, line 3373 in /wordpress/wp-content/mu-plugins/sqlite-database-integration/wp-includes/sqlite/class-wp-sqlite-translator.php)
0001.897 (0) An error condition has occurred for the first time during this job
Steps to Reproduce
@adamziel is it appropriate to reopen this issue based on this discovery?
[^1]: If you do not use the tooltip for navigating to the UpdraftPlus Backups page, then the browser immediately loads with the reference error, preventing you from starting a backup and uncovering the SQL error in the log.
Thank you for reporting! And yes, it is appropriate. Cc @brandonpayton
@dcalhoun thanks for reporting the SQLite-database-integration fatal. I added this to my list for investigation.
Regarding the JS error:
I've noted that the ReferenceError: Can't find variable: updraftlion error remains presents in some circumstances.
This is something I observed when running the plugin directly on a WP Cloud site, so I have not considered it a Playground-related issue. Please let me know if you disagree.
This [
updraftlion
error] is something I observed when running the plugin directly on a WP Cloud site, so I have not considered it a Playground-related issue. Please let me know if you disagree.
Yes ā thank you for sharing this! I have experienced similar things on non-Playground sites sporadically but kept doubting myself. š
While the reproduction I shared in https://github.com/WordPress/wordpress-playground/issues/1272#issuecomment-2120820926 is definitely consistent and seemingly related to sqlite-database-integration
, I agree there are likely circumstances where the updraftlion
error occurs and is unrelated to Playground. I'm just not sure what those circumstances are currently. š¤
, I agree there are likely circumstances where the updraftlion errors occurs and is unrelated to Playground. I'm just not sure what those circumstances are, currently. š¤
ah, OK. I'll keep my eye out in case SQLite-database-integration is contributing to the incidence of that JS error.
UpdraftPlus plugin only works in Playground partially.
Steps to reproduce:
/wp-admin/admin.php?page=updraftplus
There is also JS errors displayed
Uncaught ReferenceError: updraftlion is not defined
in the console. Clicking different buttons triggers more errors likeUncaught ReferenceError: updraft_backup_dialog_open is not defined
, but I'm not sure if those are related to the Playground environment or the plugin itself.