Skip to content

Studio: Sites with old SQLite database integration are not updated # #966

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.

Sign up for GitHub

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

Jump to bottom
Merged
katinthehatsite merged 6 commits into from
Feb 24, 2025
Merged

Studio: Sites with old SQLite database integration are not updated # #966

katinthehatsite merged 6 commits into from
Feb 24, 2025

Conversation

@katinthehatsite
Copy link
Contributor

@katinthehatsite katinthehatsite commented Feb 21, 2025 •
edited
Loading

Related issues

Closes https://github.com/Automattic/dotcom-forge/issues/8638
Related to https://github.com/Automattic/dotcom-forge/issues/10305

Proposed Changes

This PR ensures that if an older site has wp-content/mu-plugins/sqlite-database-integration-main installed on a site, the plugin will be updated to wp-content/mu-plugins/sqlite-database-integration on app restart.

Testing Instructions

Firstly, you would need to prepare a site to reproduce the issue:

  • Create a Studio site
  • Open it in Finder and rename mu-plugins/sqlite-database-integration to mu-plugins/sqlite-database-integration-main
  • Next, change the following line in db.php:
$sqlite_plugin_implementation_folder_path = realpath( __DIR__ . '/mu-plugins/sqlite-database-integration-main' );
  • Now, restart the app and check the mu-plugins folder
  • Observe that it does not update the mu-plugins/sqlite-database-integration-main to mu-plugins/sqlite-database-integration
  • Pull the change from this branch
  • Restart the app in the terminal where you started it with rs
  • Check the mu-plugins folder
  • Observe that the old integration was updated to mu-plugins/sqlite-database-integration and main was removed at the end

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@katinthehatsite katinthehatsite self-assigned this Feb 21, 2025
katinthehatsite
katinthehatsite commented Feb 21, 2025
View reviewed changes
src/lib/sqlite-versions.ts
const sqlitePath = path.join( sitePath, 'wp-content', 'mu-plugins', SQLITE_FILENAME );
const hasWpConfig = await fs.pathExists( path.join( sitePath, 'wp-config.php' ) );
const sqliteInstalled = await isSqlLiteInstalled( sqlitePath );
const sqliteOutdated = sqliteInstalled && ( await isSqliteInstallationOutdated( sqlitePath ) );
Copy link
Contributor Author

@katinthehatsite katinthehatsite Feb 21, 2025

Choose a reason for hiding this comment

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

This current logic is problematic because is checks if the sqlite is installed at export const SQLITE_FILENAME = 'sqlite-database-integration' file path. When it does not find it (which it won't because the directory has main at the end), it evaluates to false so the update fails on the condition sqliteOutdated below: if ( ( ! sqliteInstalled && ! hasWpConfig ) || sqliteOutdated ). By removing the check, we are only checking if SqLite is outdated resulting in true and the installation of the update gets executed correctly.

@katinthehatsite katinthehatsite requested a review from a team February 21, 2025 15:47
@katinthehatsite
Copy link
Contributor Author

katinthehatsite commented Feb 21, 2025

FYI, I will fix the unit test later if we agree on the approach!

bcotrim
bcotrim reviewed Feb 24, 2025
View reviewed changes
Copy link
Contributor

@bcotrim bcotrim left a comment

Choose a reason for hiding this comment

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

I think removing the sqliteInstalled check from the sqliteOutdated calculation might make the hasWpConfig protection ineffective. Since isSqliteInstallationOutdated() returns true when no version is found (i.e., when SQLite isn't installed), wouldn't this cause SQLite to be installed on MySQL sites (with wp-config.php) when it shouldn't?

I might be missing something, what do you think?

@katinthehatsite
Copy link
Contributor Author

katinthehatsite commented Feb 24, 2025

Thanks for the feedback @bcotrim - I thought this might be a problem so I have another possible solution and that is to keep the check for sqliteInstalled as a part of sqliteOutdated and instead solve the issue as part of sqliteInstalled check where we also check for the legacy path. I made some changes here: https://github.com/Automattic/studio/pull/966/files

Let me know what you think about this approach 🙇

@katinthehatsite katinthehatsite requested a review from a team February 24, 2025 09:56
bcotrim
bcotrim approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@bcotrim bcotrim left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification and adjustments!
I tested the changes and they work as expected.
The code looks good, and I think this more conservative approach is better.
LGTM 👍

@katinthehatsite katinthehatsite merged commit 6de7117 into trunk Feb 24, 2025
7 checks passed
@katinthehatsite katinthehatsite deleted the fix/old-sql-sites-not-updated branch February 24, 2025 15:09
@wojtekn
Copy link
Contributor

wojtekn commented Feb 25, 2025

@katinthehatsite it's great that we have a simple solution than the initial one.

It fixed the issue for my older sites, and now, all of them have the plugin in the correct location, and all can be exported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bcotrim bcotrim bcotrim approved these changes

Assignees

@katinthehatsite katinthehatsite

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@katinthehatsite @wojtekn @bcotrim