Skip to content

Conversation

@fredrikekelund
Copy link
Contributor

@fredrikekelund fredrikekelund commented Dec 17, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/10054

In https://github.com/Automattic/dotcom-forge/issues/10054, we discovered that exports are currently broken in Studio when running PHP <8. This happens because str_ends_with is used in the sqlite-database-integration codebase, but that function only became part of PHP's stdlib in version 8. The crux is that sqlite-database-integration actually has a polyfill for this function, but that polyfill isn't loaded. This happens because we use the before_wp_load hook for the WP-CLI commands in this project, short-circuiting WP's regular logic for loading mu-plugins. Instead, we manually require files from sqlite-database-integration, but we hadn't previously included php-polyfills.php. This PR fixes that.

Testing instructions

  1. Clone this PR locally
  2. Run composer install --no-dev --optimize-autoloader --ignore-platform-reqs in this repo
  3. Replace wp-files/sqlite-command in the Studio repo with a symlink to your local wp-cli-sqlite-command repo
  4. Apply 3658f-pb to the Studio repo
  5. Run npm start
  6. In the Settings tab, change the PHP version to 7.4
  7. In the Import/Export tab, click "Export database"
  8. Ensure a SQL dump is created successfully

@fredrikekelund fredrikekelund self-assigned this Dec 17, 2024
@fredrikekelund fredrikekelund requested review from a team and removed request for sejas and wojtekn December 18, 2024 10:26
@wojtekn
Copy link
Contributor

wojtekn commented Dec 18, 2024

@fredrikekelund, the fix by loading polyfills explicitly looks valid, and it fixes the issue for me.

Are src/SQLite_Command.php changes leftovers from the previous implementation that should be removed?

Comment on lines -32 to -34
if ( ! Import::get_sqlite_plugin_version() ) {
WP_CLI::error( 'The SQLite integration plugin is not installed or activated.' );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized the SQLiteDatabaseIntegrationLoader::load_plugin() method already performs an equivalent check. No need to duplicate that logic.

* @throws WP_CLI\ExitException
*/
protected function load_dependencies() {
public static function load_plugin() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing the initial changes in this PR with @wojtekn, we concluded that this method could also be static, and there's really no reason for the command classes to extend this class. Hence, I took the opportunity to make this into a standalone class with only static methods.

Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected. Thanks for the cleanup!

@wojtekn wojtekn merged commit 45f05f5 into main Dec 18, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants