Load polyfills correctly for the new sqlite version (v2.2.22)#24
Load polyfills correctly for the new sqlite version (v2.2.22)#24katinthehatsite merged 12 commits intomainfrom
Conversation
b098ba7 to
100d0a4
Compare
fredrikekelund
left a comment
There was a problem hiding this comment.
Rather than supporting two different file structures, I think we should consider updating the version check for the sqlite-database-integration plugin.
|
The PR tests well, though. I can confirm that it fixes the problem for me locally 👍 |
|
OK, hold on a minute… Studio checks for remote updates to this dependency, but not for sqlite-database-integration. That means we ship a variable version of this dependency but a fixed version of sqlite-database-integration. Zooming out for a moment, this seems like a less-than-ideal strategy because these two dependencies are actually relatively tightly coupled. Nonetheless, that's how it works. So my suggestion to support a single file structure in sqlite-database-integration wouldn't really work, because we can't be sure which bundled version of sqlite-database-integration the user has locally. |
|
It looks like the failed tests are due to some recent update in https://github.com/wp-cli/wp-cli-tests. AFAICT, we didn't run those tests in other recent PRs. |
fredrikekelund
left a comment
There was a problem hiding this comment.
This PR works well – it does what it says on the tin 👍
However, I still can't push a site using PHP 8.5, so it doesn't fix STU-1529 for me. Here are my error logs for when I try to push a site that uses PHP 8.5.
["wp_commentmeta","wp_comments","wp_links","wp_options","wp_postmeta","wp_posts","wp_term_relationships","wp_term_taxonomy","wp_termmeta","wp_terms","wp_usermeta","wp_users"]
Error occurred in handler for 'exportSiteForPush': Error: Database export failed: PHP Deprecated: Case statements followed by a semicolon (;) are deprecated, use a colon (:) instead in phar:///tmp/wp-cli.phar/vendor/react/promise/src/functions.php on line 369
at exportDatabaseToMultipleFiles (/Users/fredrik/Code/studio/apps/studio/dist/main/index.js:28293:11)
at async DefaultExporter.addDatabase (/Users/fredrik/Code/studio/apps/studio/dist/main/index.js:28553:24)
at async DefaultExporter.export (/Users/fredrik/Code/studio/apps/studio/dist/main/index.js:28450:7)
at async exportBackup (/Users/fredrik/Code/studio/apps/studio/dist/main/index.js:28691:9)
at async exportSiteForPush (/Users/fredrik/Code/studio/apps/studio/dist/main/index.js:44236:5)
at async Session.<anonymous> (node:electron/js2c/browser_init:2:116791)
| $db = $plugin_directory . '/wp-includes/database'; | ||
| return [ | ||
| "$db/version.php", | ||
| "$db/parser/class-wp-parser-grammar.php", | ||
| "$db/parser/class-wp-parser.php", | ||
| "$db/parser/class-wp-parser-node.php", | ||
| "$db/parser/class-wp-parser-token.php", | ||
| "$db/mysql/class-wp-mysql-token.php", | ||
| "$db/mysql/class-wp-mysql-lexer.php", | ||
| "$db/mysql/class-wp-mysql-parser.php", | ||
| "$db/sqlite/class-wp-sqlite-pdo-user-defined-functions.php", | ||
| "$db/sqlite/class-wp-sqlite-connection.php", | ||
| "$db/sqlite/class-wp-sqlite-configurator.php", | ||
| "$db/sqlite/class-wp-sqlite-driver.php", | ||
| "$db/sqlite/class-wp-sqlite-driver-exception.php", | ||
| "$db/sqlite/class-wp-sqlite-information-schema-builder.php", | ||
| "$db/sqlite/class-wp-sqlite-information-schema-exception.php", | ||
| "$db/sqlite/class-wp-sqlite-information-schema-reconstructor.php", | ||
| ]; | ||
| } | ||
|
|
||
| $wp = $plugin_directory . '/wp-includes'; | ||
| return [ | ||
| $plugin_directory . '/version.php', | ||
| "$wp/parser/class-wp-parser-grammar.php", | ||
| "$wp/parser/class-wp-parser.php", | ||
| "$wp/parser/class-wp-parser-node.php", | ||
| "$wp/parser/class-wp-parser-token.php", | ||
| "$wp/mysql/class-wp-mysql-token.php", | ||
| "$wp/mysql/class-wp-mysql-lexer.php", | ||
| "$wp/mysql/class-wp-mysql-parser.php", | ||
| "$wp/sqlite/class-wp-sqlite-pdo-user-defined-functions.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-connection.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-configurator.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-driver.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-driver-exception.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-information-schema-builder.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-information-schema-exception.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php", | ||
| ]; |
There was a problem hiding this comment.
Since we're not using the same array to load the files, the $db and $wp prefixing seems kind of arbitrary to me. I'd spell out the full $plugin_directory . $path
There was a problem hiding this comment.
I think these are not needed as per https://github.com/Automattic/wp-cli-sqlite-command/pull/24/changes#r3057297293.
| require_once "$sqlite/class-wp-sqlite-lexer.php"; | ||
| require_once "$sqlite/class-wp-sqlite-query-rewriter.php"; | ||
| require_once "$sqlite/class-wp-sqlite-translator.php"; | ||
| require_once "$sqlite/class-wp-sqlite-token.php"; | ||
| require_once "$sqlite/class-wp-sqlite-pdo-user-defined-functions.php"; |
There was a problem hiding this comment.
Nitpicking, but still, I'd definitely spell out the full $plugin_directory . $path here.
JanJakes
left a comment
There was a problem hiding this comment.
Thanks for working on this! I totally missed the wp-pdo-mysql-on-sqlite.php back-compatibility angle. Sorry about that. I shared a few ideas for improvements and simplifications.
| */ | ||
| private static function load_ast_driver( $plugin_directory, $new_structure ) { | ||
| if ( file_exists( $plugin_directory . '/wp-pdo-mysql-on-sqlite.php' ) ) { | ||
| require_once $plugin_directory . '/wp-pdo-mysql-on-sqlite.php'; |
There was a problem hiding this comment.
Not keeping require_once $plugin_directory . '/wp-pdo-mysql-on-sqlite.php'; in 2.2.21 and 2.2.22 was an oversight, but rather than listing the plugin files, we can just add this:
} elseif ( file_exists( $plugin_directory . '/wp-includes/database/load.php' ) ) {
require_once '$plugin_directory . '/wp-includes/database/load.php';
}The rest of the diff is not needed.
| $db = $plugin_directory . '/wp-includes/database'; | ||
| return [ | ||
| "$db/version.php", | ||
| "$db/parser/class-wp-parser-grammar.php", | ||
| "$db/parser/class-wp-parser.php", | ||
| "$db/parser/class-wp-parser-node.php", | ||
| "$db/parser/class-wp-parser-token.php", | ||
| "$db/mysql/class-wp-mysql-token.php", | ||
| "$db/mysql/class-wp-mysql-lexer.php", | ||
| "$db/mysql/class-wp-mysql-parser.php", | ||
| "$db/sqlite/class-wp-sqlite-pdo-user-defined-functions.php", | ||
| "$db/sqlite/class-wp-sqlite-connection.php", | ||
| "$db/sqlite/class-wp-sqlite-configurator.php", | ||
| "$db/sqlite/class-wp-sqlite-driver.php", | ||
| "$db/sqlite/class-wp-sqlite-driver-exception.php", | ||
| "$db/sqlite/class-wp-sqlite-information-schema-builder.php", | ||
| "$db/sqlite/class-wp-sqlite-information-schema-exception.php", | ||
| "$db/sqlite/class-wp-sqlite-information-schema-reconstructor.php", | ||
| ]; | ||
| } | ||
|
|
||
| $wp = $plugin_directory . '/wp-includes'; | ||
| return [ | ||
| $plugin_directory . '/version.php', | ||
| "$wp/parser/class-wp-parser-grammar.php", | ||
| "$wp/parser/class-wp-parser.php", | ||
| "$wp/parser/class-wp-parser-node.php", | ||
| "$wp/parser/class-wp-parser-token.php", | ||
| "$wp/mysql/class-wp-mysql-token.php", | ||
| "$wp/mysql/class-wp-mysql-lexer.php", | ||
| "$wp/mysql/class-wp-mysql-parser.php", | ||
| "$wp/sqlite/class-wp-sqlite-pdo-user-defined-functions.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-connection.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-configurator.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-driver.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-driver-exception.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-information-schema-builder.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-information-schema-exception.php", | ||
| "$wp/sqlite-ast/class-wp-sqlite-information-schema-reconstructor.php", | ||
| ]; |
There was a problem hiding this comment.
I think these are not needed as per https://github.com/Automattic/wp-cli-sqlite-command/pull/24/changes#r3057297293.
|
|
||
| require_once $new_structure | ||
| ? $plugin_directory . '/wp-includes/database/php-polyfills.php' | ||
| : $plugin_directory . '/php-polyfills.php'; |
There was a problem hiding this comment.
These php-polyfills preloads are only relevant for the old structure. The new structure includes them in $plugin_directory . '/wp-includes/database/load.php'. See also https://github.com/Automattic/wp-cli-sqlite-command/pull/24/changes#r3057297293.
In other words, it's safe to include this file only if it exists:
if ( file_exists( $plugin_directory . '/php-polyfills.php' ) {
require_once $plugin_directory . '/php-polyfills.php';
}| // We also need to selectively load the necessary classes from the plugin. | ||
| require_once $plugin_directory . '/php-polyfills.php'; | ||
| // In v2.2.22+, files moved into wp-includes/database/ subdirectories. | ||
| $new_structure = file_exists( $plugin_directory . '/wp-includes/database/php-polyfills.php' ); |
There was a problem hiding this comment.
Maybe it's safer to check for the old structure, as in:
$old_structure = file_exists( $plugin_directory . '/php-polyfills.php' );Or for the new structure using $plugin_directory . '/wp-includes/database/load.php'. It will be safer than relying on the existence of the polyfill file.
There was a problem hiding this comment.
Applied the suggestion 👍
|
|
||
| if ( $new_driver_enabled && file_exists( $plugin_directory . '/wp-pdo-mysql-on-sqlite.php' ) ) { | ||
| require_once $plugin_directory . '/wp-pdo-mysql-on-sqlite.php'; | ||
| } elseif ( $new_driver_enabled ) { |
There was a problem hiding this comment.
@katinthehatsite This branch actually also needs to stay in the code for now. I guess I should've expressed it clearer, as in, let's only add the new case to the existing ones:
} elseif ( file_exists( $plugin_directory . '/wp-includes/database/load.php' ) ) {
require_once $plugin_directory . '/wp-includes/database/load.php';
}There was a problem hiding this comment.
Added them back now 👍
| "email": "jeroen.pfeil@automattic.com" | ||
| } | ||
| ], | ||
| "minimum-stability": "dev", |
There was a problem hiding this comment.
I tried without these but it did not work for using wp-cli/wp-cli": "^2.13 so Claude recommended adding this
Closes STU-1529
In
v2.2.22+for thesqlite-database-integration,php-polyfills.phpmoved towp-includes/database/php-polyfills.php. If you try to push a newly created site in Studio with that version, it will immediately fail due not being able to loadphp-polyfills.php. This PR adds a backwards compatible fix for it.Testing instructions
~/.studio/server-files/sqlite-command/src/SQLiteDatabaseIntegrationLoader.php~/.studio/server-files/sqlite-command/versiontov99.99.99Synctab