Disable import/export for sites not using SQLite integration #463

fluiddot · 2024-08-14T16:38:57Z

Related to https://github.com/Automattic/dotcom-forge/issues/9395

Proposed Changes

Add a function to determine if SQLite integration is installed on a site.
Add an IPC handler that determines if the Import/Export feature is supported for a site. For now, the condition we check is if SQLite integration is installed for that site as the import/export commands used only support SQLite databases. In the future, we could add more.
Render a warning notice in the Import/Export tab if the site doesn't have the SQLite integration.

Captura de pantalla 2024-09-26 a las 14 13 47

Testing Instructions

Create a site.
Navigate to the site folder.
Remove the file wp-content/db.php.
Navigate to the Import/Export tab.
Observe a warning notice is displayed and that the Import/Export functionality is disabled.
Select another site.
Observe the Import/Export functionality is enabled and working.

Pre-merge Checklist

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

fluiddot · 2024-08-14T16:42:24Z

@matt-west let me know if you could take a look at the UI and share thoughts/feedback. I decided to use the WordPress Notice component as an initial iteration, but we could follow a better approach. Thanks 🙇 !

jeroenpf · 2024-08-15T12:07:06Z

src/site-server.ts

+		);
+		const wpConfigPath = nodePath.join( this.details.path, 'wp-config.php' );
+		const databaseIndexPath = nodePath.join( this.details.path, 'wp-content', 'db.php' );
+		const databasePath = nodePath.join( this.details.path, 'wp-content', 'database', '.ht.sqlite' );


I don't think we need to check this because even if you remove this file, the site will still work and use SQLite. It will just create a new .ht.sqlite file automatically.

Ah, thanks for pointing this out. I've tested removing the file and confirmed it's recreated. Hence, I'll remove this check.

I've addressed this in 3314f63.

jeroenpf · 2024-08-15T12:08:15Z

src/site-server.ts

 		}
 	}
+
+	async isSQLiteInstalled(): Promise< boolean > {


Is this function checking if SQLite plugin is installed or if it is active? I think, based on the code inside the function, that it is to check if SQLite is active. If that is the case, for clarity maybe this can be renamed to isSQLitePluginActivated or something similar.

Yep, good point. The function checks both that is installed and active. My rationale is that since the SQLite plugin is a must-use plugin if it's installed we also consider it active. However, I agree that it's more accurate to reference in the function's name that the plugin is active. I'll rename it.

I've addressed this in 566e657.

matt-west · 2024-09-20T10:14:03Z

@fluiddot Sorry for the slow response on this!

Using the Notice component here makes sense. We’ll have an updated version of this in the new design system, but we can use this one for now.

I think we could tweak the copy to something like:

Import / Export is not available for this site
This feature is only available for sites using the default SQLite integration.

fluiddot · 2024-09-23T08:01:46Z

@fluiddot Sorry for the slow response on this!

Using the Notice component here makes sense. We’ll have an updated version of this in the new design system, but we can use this one for now.

I think we could tweak the copy to something like:

Import / Export is not available for this site
This feature is only available for sites using the default SQLite integration.

Great, thanks @matt-west for taking a look 🙇 ! I'll update the message and wrap up the PR with the current design 👍 .

fluiddot · 2024-09-26T12:15:40Z

@jeroenpf @matt-west I've addressed the latest comments and updated the message in the notice. The PR is now ready to review. Thanks 🙇 !

wojtekn · 2024-09-26T12:43:30Z

@fluiddot it works for most of my sites. However, it disables Import / Export for some older sites that had -main appended to the sqlite plugin name:

Could we handle those, too?

fluiddot · 2024-09-26T13:21:21Z

@fluiddot it works for most of my sites. However, it disables Import / Export for some older sites that had -main appended to the sqlite plugin name:

Could we handle those, too?

Sure, I'll cover this case when we use -main for the folder's name. I feel this case can be easily overlooked when testing, so I'll consider adding some unit tests to cover it.

sejas · 2024-10-04T13:06:57Z

@wojtekn, I wonder how is possible to have a site with the old integration 🤔 .

From what I can see the old/main SQLite folder gets renamed when the site starts.

studio/src/lib/sqlite-versions.ts

Line 173 in 792b39e

await removeLegacySqliteIntegrationPlugin( sqlitePluginPath );

studio/src/lib/sqlite-versions.ts

Lines 139 to 142 in 792b39e

    
           const sqliteInstalled = await isSqlLiteInstalled( sqlitePath ); 
        
           const sqliteOutdated = sqliteInstalled && ( await isSqliteInstallationOutdated( sqlitePath ) ); 
        
           if ( ( ! sqliteInstalled && ! hasWpConfig ) || sqliteOutdated ) {

Could you share your zip? Maybe your site is using MySQL?

wojtekn · 2024-10-04T15:19:33Z

@sejas I checked two sites, and one has a plugin in wp-content/mu-plugins/sqlite-database-integration-main and another in wp-content/plugins/sqlite-database-integration.

The sites that show the warning, have the plugin in wp-content/mu-plugins/sqlite-database-integration. Stopping and starting site doesn't change anything.

…-import-export-mysql-sites

sejas · 2024-11-18T18:39:03Z

@wojtekn , can you try again?

I tried using a different approach, by checking if the $wpdb uses the SQLite PHP class, but I had to go back to use the paths check because my approach only worked for started sites. Making it work for stopped sites is possible but we need a big refactor, and I think that is currently out of scope of this issue.

Here is the code for reference to check if SQLite is used in a WordPress site:

		const isSQLitePhp = `<?php
		require_once('${ this.server?.php?.documentRoot }/wp-load.php');
		global $wpdb;
		echo get_class($wpdb) === 'WP_SQLite_DB' ? 'true' : 'false';
		`;
		let isSQLiteString = null;
		try {
			isSQLiteString = await this.server?.runPhp( {
				code: isSQLitePhp,
			} );
		} catch ( error ) {
			Sentry.captureException( error );
		}
		return isSQLiteString === 'true';

So in the end I just added the alternative paths and added a new test. Let me know if it works for your old sites.

wojtekn

Works well for all three cases: plugin in wp-content/, plugin in mu-plugins/, and plugin in mu-plugins/ with -main appended to the name.

wojtekn · 2024-11-19T09:19:32Z

src/site-server.ts

 		}
 	}
+
+	async isSQLitePluginActivated(): Promise< boolean > {


As we only check if files and directories exist, should we find a more accurate name for the function like hasSQLitePlugin?

Great suggestion. Done! 9e54f4a

fluiddot requested a review from a team August 14, 2024 16:38

fluiddot self-assigned this Aug 14, 2024

jeroenpf reviewed Aug 15, 2024

View reviewed changes

matt-west self-requested a review September 20, 2024 08:59

matt-west removed their request for review September 20, 2024 10:14

fluiddot added 6 commits September 26, 2024 13:59

Add IPC handler isSQLiteInstalled

928e517

Disable Import/Export for sites not using SQLite integration

14af84f

Fix import/export unit tests

d161db0

Remove checking ht.sqlite file

3314f63

Rename isSQLiteInstalled function

566e657

Expand message when import/export is not supported

792b39e

fluiddot force-pushed the fix/disable-import-export-mysql-sites branch from f0a508c to 792b39e Compare September 26, 2024 12:14

fluiddot marked this pull request as ready for review September 26, 2024 12:15

fluiddot requested a review from a team September 26, 2024 12:15

fluiddot added [Type] Enhancement Import & Export Related to the Import & Export functionality in Studio. labels Sep 26, 2024

fluiddot removed their assignment Oct 3, 2024

wojtekn removed the [Status] Needs Review label Oct 4, 2024

sejas self-assigned this Oct 4, 2024

Merge branch 'trunk' of github.com:Automattic/studio into fix/disable…

bbafff9

…-import-export-mysql-sites

sejas added 4 commits November 18, 2024 17:06

Add alternative SQLite mu-plugins paths

ff7d57d

display nothing instead of enable window while loading

b789dbf

Update the hook when the site starts or stops

aa71c1c

update test

6156172

sejas requested a review from wojtekn November 18, 2024 18:35

sejas added the Reviewer can merge label Nov 19, 2024

wojtekn approved these changes Nov 19, 2024

View reviewed changes

rename isSQLitePluginActivated to hasSQLitePlugin

9e54f4a

sejas merged commit e163432 into trunk Nov 20, 2024
15 checks passed

sejas deleted the fix/disable-import-export-mysql-sites branch November 20, 2024 14:13

Disable import/export for sites not using SQLite integration #463

Disable import/export for sites not using SQLite integration #463

Uh oh!

Conversation

fluiddot commented Aug 14, 2024 • edited by wojtekn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Testing Instructions

Pre-merge Checklist

Uh oh!

fluiddot commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matt-west commented Sep 20, 2024

Uh oh!

fluiddot commented Sep 23, 2024

Uh oh!

fluiddot commented Sep 26, 2024

Uh oh!

wojtekn commented Sep 26, 2024

Uh oh!

fluiddot commented Sep 26, 2024

Uh oh!

sejas commented Oct 4, 2024

Uh oh!

wojtekn commented Oct 4, 2024

Uh oh!

sejas commented Nov 18, 2024

Uh oh!

wojtekn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fluiddot commented Aug 14, 2024 •

edited by wojtekn

Loading