Add Local backup format importer #436

wojtekn · 2024-08-07T15:20:05Z

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

Proposed Changes

I propose to add support importing site backup exported from the Local app.

Testing Instructions

Create a site in the Local
Install custom theme, activate it, upload media, install the plugin, add post etc.
Export site from Local
Import site to Studio

Confirm that the site looks in the same way as in Local.

Pre-merge Checklist

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

src/lib/import-export/import/importers/importer.ts

wojtekn · 2024-08-07T15:28:03Z

src/lib/import-export/import/importers/importer.ts

@@ -83,7 +79,7 @@ export abstract class BaseImporter extends EventEmitter implements Importer {
 	}
 }

-export class JetpackImporter extends BaseImporter {
+export abstract class BaseBackupImporter extends BaseImporter implements Importer {


It would be better to use composition over inheritance, but currently, we rely on inheritance here, so I followed that pattern and added another level of base class.

wojtekn · 2024-08-07T15:32:05Z

src/lib/import-export/import/validators/local-validator.ts

+import { BackupContents } from '../types';
+import { Validator } from './validator';
+
+export class LocalValidator extends EventEmitter implements Validator {


The only differences between this one and JetpackValidator are path prefixes and meta file names. However, I made a separate class instead of trying to have a generic one that handles both cases.

wojtekn · 2024-08-08T06:31:14Z

@Automattic/yolo it's ready for review.

I'm facing one issue I'm unsure about. When I Import Local backup, the uploads/ are missing on my imported site. I debugged the code to some point, and it seems the validator correctly finds those directories.

I think it may be an issue not related to the change, as @fluiddot reported that for Jetpack Backup import. Maybe something related to file permissions inside the package?

fluiddot

@wojtekn I tested importing a Local site and noticed that the content of wp-content is copied in the wrong path. The files are copied to <SITE_PATH>/app/public/wp-content instead of <SITE_PATH>/wp-content. I think we'd need to adjust the full path defined in the parseBackupContents function of the Local validator.

src/lib/import-export/import/importers/importer.ts

fluiddot · 2024-08-08T09:18:37Z

I also encountered some warnings when importing the database related to LOCK/UNLOCK SQL statements. But I presume they are non-harmful.

Warning: Could not execute statement: LOCK TABLES `wp_commentmeta` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_comments` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_links` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_options` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_postmeta` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_posts` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_term_relationships` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_term_taxonomy` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_termmeta` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_terms` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_usermeta` WRITE
Warning: Could not execute statement: UNLOCK TABLES
Warning: Could not execute statement: LOCK TABLES `wp_users` WRITE
Warning: Could not execute statement: UNLOCK TABLES

wojtekn · 2024-08-08T10:28:30Z

I fixed the issue with the wp-content/ that was unpacked to the incorrect path. It was causing the missing uploads/ as well.

I also encountered some warnings when importing the database related to LOCK/UNLOCK SQL statements. But I presume they are non-harmful.

It's expected - we inform a user about queries that can't be executed on SQLite database.

fluiddot

LGTM 🎊 ! I successfully imported a backup exported from a Local site and confirmed the changes matched the ones from the original site. Awesome job @wojtekn 🏅 !

fluiddot · 2024-08-08T10:48:25Z

src/lib/import-export/import/types.ts

@@ -15,6 +15,7 @@ export interface BackupContents {
 	extractionDirectory: string;
 	sqlFiles: string[];
 	wpContent: WpContent;
+	wpContentDirectory: string;


We could mark this as optional and use wp-content value as default when not defined.

SqlValidator also uses that interface, and it should be empty for this case. In such a case, would it be worth to have a default value of the empty string?

If we set the property as optional, it would be up to the importers to decide when to use a default value. In case it clarifies better my proposal, here's a potential approach of the changes I'm thinking of:

diff --git forkSrcPrefix/src/lib/import-export/import/types.ts forkDstPrefix/src/lib/import-export/import/types.ts index 27e84598288d7aa8509fb3b86c31a3968581a6c4..a7c30ee5f5ca32dd32c1ffd97277831e87aaf985 100644 --- forkSrcPrefix/src/lib/import-export/import/types.ts +++ forkDstPrefix/src/lib/import-export/import/types.ts @@ -15,7 +15,7 @@ export interface BackupContents { extractionDirectory: string; sqlFiles: string[]; wpContent: WpContent; - wpContentDirectory: string; + wpContentDirectory?: string; metaFile?: string; } diff --git forkSrcPrefix/src/lib/import-export/import/validators/sql-validator.ts forkDstPrefix/src/lib/import-export/import/validators/sql-validator.ts index dd81e966e136ffbcc46c8f69804dd07774457f5d..81b230feca3ed4d0154926eb442eeb2a9f7461aa 100644 --- forkSrcPrefix/src/lib/import-export/import/validators/sql-validator.ts +++ forkDstPrefix/src/lib/import-export/import/validators/sql-validator.ts @@ -19,7 +19,6 @@ export class SqlValidator extends EventEmitter implements Validator { plugins: [], themes: [], }, - wpContentDirectory: '', }; for ( const file of fileList ) { diff --git forkSrcPrefix/src/lib/import-export/tests/import/validators/sql-validator.test.ts forkDstPrefix/src/lib/import-export/tests/import/validators/sql-validator.test.ts index 0da86e52faa9faad2a4d6cbf40a6c2c0d41b3a1d..b6f9d9bc5f76476acac33e8afde0c8e9a01db6b6 100644 --- forkSrcPrefix/src/lib/import-export/tests/import/validators/sql-validator.test.ts +++ forkDstPrefix/src/lib/import-export/tests/import/validators/sql-validator.test.ts @@ -40,7 +40,6 @@ describe( 'SqlValidator', () => { plugins: [], themes: [], }, - wpContentDirectory: '', } ); } ); } ); diff --git forkSrcPrefix/src/lib/import-export/import/importers/importer.ts forkDstPrefix/src/lib/import-export/import/importers/importer.ts index d85e306c68a911d3eddfb4e76de18656bc7ef044..35cd8d8676e58b590fcc338cd93b5e459ace2a57 100644 --- forkSrcPrefix/src/lib/import-export/import/importers/importer.ts +++ forkDstPrefix/src/lib/import-export/import/importers/importer.ts @@ -19,6 +19,8 @@ export interface Importer extends Partial< EventEmitter > { import( rootPath: string, siteId: string ): Promise< ImporterResult >; } +const DEFAULT_WP_CONTENT_DIR = 'wp-content'; + abstract class BaseImporter extends EventEmitter implements Importer { constructor( protected backup: BackupContents ) { super(); @@ -128,7 +130,7 @@ abstract class BaseBackupImporter extends BaseImporter { this.emit( ImportEvents.IMPORT_WP_CONTENT_START ); const extractionDirectory = this.backup.extractionDirectory; const wpContent = this.backup.wpContent; - const wpContentSourceDir = this.backup.wpContentDirectory; + const wpContentSourceDir = this.backup.wpContentDirectory ?? DEFAULT_WP_CONTENT_DIR; const wpContentDestDir = path.join( rootPath, 'wp-content' ); for ( const files of Object.values( wpContent ) ) { for ( const file of files ) { diff --git forkSrcPrefix/src/lib/import-export/tests/import/validators/jetpack-validator.test.ts forkDstPrefix/src/lib/import-export/tests/import/validators/jetpack-validator.test.ts index 0155ffc63ce58319e27500fa80f3a5d56dbc647f..538584e91338b835c4fb9409cbdf637bf2307149 100644 --- forkSrcPrefix/src/lib/import-export/tests/import/validators/jetpack-validator.test.ts +++ forkDstPrefix/src/lib/import-export/tests/import/validators/jetpack-validator.test.ts @@ -54,7 +54,6 @@ describe( 'JetpackValidator', () => { plugins: [ '/tmp/extracted/wp-content/plugins/jetpack/jetpack.php' ], themes: [ '/tmp/extracted/wp-content/themes/twentytwentyone/style.css' ], }, - wpContentDirectory: 'wp-content', metaFile: '/tmp/extracted/studio.json', } ); } ); @@ -82,7 +81,6 @@ describe( 'JetpackValidator', () => { plugins: [ '/tmp/extracted/wp-content/plugins/jetpack/jetpack.php' ], themes: [ '/tmp/extracted/wp-content/themes/twentytwentyone/style.css' ], }, - wpContentDirectory: 'wp-content', metaFile: '/tmp/extracted/studio.json', } ); } ); diff --git forkSrcPrefix/src/lib/import-export/import/validators/jetpack-validator.ts forkDstPrefix/src/lib/import-export/import/validators/jetpack-validator.ts index 59b189e7fda6f69e4af1e8749389878cd3f0610e..aada4d5d7d4360fddf10994bb2753f97782b86f8 100644 --- forkSrcPrefix/src/lib/import-export/import/validators/jetpack-validator.ts +++ forkDstPrefix/src/lib/import-export/import/validators/jetpack-validator.ts @@ -23,7 +23,6 @@ export class JetpackValidator extends EventEmitter implements Validator { plugins: [], themes: [], }, - wpContentDirectory: 'wp-content', }; /* File rules: * - Ignore wp-config.php diff --git forkSrcPrefix/src/lib/import-export/tests/import/importer/default-importer.test.ts forkDstPrefix/src/lib/import-export/tests/import/importer/default-importer.test.ts index 935ba152afa3864dc82f0d8bd28fe75500ab706a..b5f639f5dffe6e9771c39b96073e4a9076d86a06 100644 --- forkSrcPrefix/src/lib/import-export/tests/import/importer/default-importer.test.ts +++ forkDstPrefix/src/lib/import-export/tests/import/importer/default-importer.test.ts @@ -18,7 +18,6 @@ describe( 'JetpackImporter', () => { plugins: [ '/tmp/extracted/wp-content/plugins/jetpack/jetpack.php' ], themes: [ '/tmp/extracted/wp-content/themes/twentytwentyone/style.css' ], }, - wpContentDirectory: 'wp-content', metaFile: '/tmp/extracted/studio.json', };

Thanks, I see. I'm still not convinced if the default value is the right choice at this point, as we have three validators, each using a different value:

src/lib/import-export/tests/import/importer/local-importer.test.ts

wojtekn self-assigned this Aug 7, 2024

wojtekn commented Aug 7, 2024

View reviewed changes

wojtekn requested a review from a team August 8, 2024 06:27

wojtekn marked this pull request as ready for review August 8, 2024 06:31

wojtekn changed the title ~~Add Local backup importer~~ Add Local backup format importer Aug 8, 2024

fluiddot suggested changes Aug 8, 2024

View reviewed changes

src/lib/import-export/import/importers/importer.ts Outdated Show resolved Hide resolved

wojtekn force-pushed the add/local-importer branch from 1df6154 to e77de67 Compare August 8, 2024 09:23

wojtekn requested a review from fluiddot August 8, 2024 10:35

fluiddot approved these changes Aug 8, 2024

View reviewed changes

wojtekn added 16 commits August 8, 2024 13:47

Add initial importer for Local file format

cffc710

Refactor to account for another backup file importer

5597bc4

Parse Local meta file PHP version

05443c6

Compose database import method back to base class

f864155

Revert not needed empty line change

ca7c5a1

Fix lint issue

0f656da

Fix final lint issue

fac3316

Revert no longer needed change

1c79dc1

Clean up code

ba2181e

Remove not needed constructor

6f50458

Do not export abstract classes

6dd7e1f

Add tests for Local import validator

9b9b7e3

Add tests for Local importer

075b536

Use semver to manipulate PHP version

838e400

Add support for wp-content that could be placed on different levels

f87df32

Revert to real timers after each test, not after all

89e131f

wojtekn force-pushed the add/local-importer branch from fbb7dc5 to 89e131f Compare August 8, 2024 11:58

wojtekn merged commit 5cde84f into trunk Aug 9, 2024
10 checks passed

wojtekn deleted the add/local-importer branch August 9, 2024 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Local backup format importer #436

Add Local backup format importer #436

wojtekn commented Aug 7, 2024 •

edited

Loading

wojtekn Aug 7, 2024

wojtekn Aug 7, 2024

wojtekn commented Aug 8, 2024

fluiddot left a comment

fluiddot commented Aug 8, 2024 •

edited

Loading

wojtekn commented Aug 8, 2024

fluiddot left a comment

fluiddot Aug 8, 2024

wojtekn Aug 8, 2024

fluiddot Aug 8, 2024

wojtekn Aug 9, 2024

Add Local backup format importer #436

Add Local backup format importer #436

Conversation

wojtekn commented Aug 7, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

wojtekn Aug 7, 2024

Choose a reason for hiding this comment

wojtekn Aug 7, 2024

Choose a reason for hiding this comment

wojtekn commented Aug 8, 2024

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot commented Aug 8, 2024 • edited Loading

wojtekn commented Aug 8, 2024

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot Aug 8, 2024

Choose a reason for hiding this comment

wojtekn Aug 8, 2024

Choose a reason for hiding this comment

fluiddot Aug 8, 2024

Choose a reason for hiding this comment

wojtekn Aug 9, 2024

Choose a reason for hiding this comment

wojtekn commented Aug 7, 2024 •

edited

Loading

fluiddot commented Aug 8, 2024 •

edited

Loading