-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
Add support for importing .wpress
files
#497
Conversation
Could I get an initial review on this? Also, what sort of tests would you suggest (particularly given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could I get an initial review on this?
The PR looks great, thanks @danielbachhuber for creating it 🙇 . I added some comments to the code but apart from using async file system operations, the approach looks solid.
However, I encountered an issue when testing it. After importing a wpress
file, the active theme points to an empty theme. This makes the site render a blank page.
Also, what sort of tests would you suggest (particularly given .wpress' custom backup format?
You check examples of other tests we made for other formats here. We mock the file reading operations, so I presume the tests for the wpress
format would be similar to the ones we have.
Let me know if there's anything I can help further to move the PR forward 🙂.
} ); | ||
|
||
rl.on( 'line', ( line: string ) => { | ||
writeStream.write( line.replace( /SERVMASK_PREFIX/g, 'wp' ) + '\n' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could expand on why wpress
format needs to modify the SQL code before importing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you refer that we should add a comment explaining why why we replace this fake prefix?
It seems that wpress replaces the original prefix with SERVMASK_PREFIX, probably to avoid prefix collisions when importing to a shared database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you refer that we should add a comment explaining why why we replace this fake prefix? It seems that wpress replaces the original prefix with SERVMASK_PREFIX, probably to avoid prefix collisions when importing to a shared database.
Yep, I was curious why we need this. Thanks @sejas for explaining it 🙇 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa! It's almost working!
Great work and All-in-One WP Migration and Backup
format will be a great addition.
I've experienced the same error, where the theme is not selected. After accessing twice to /wp-admin/themes.php
the 2024 theme was selected automatically instead of the correct theme used in the original site.
At first I thought it was some errors with SQLite import, like not supporting "START TRANSACTION;", but after debugging it, I've found that my .wpress
file has an empty string for the template
and stylesheet
options. It seems that the All in one plugin removes those options from the SQL dump on purpose.
As it turns out, the I guess we need to process that post-import too. |
await fsPromises.rename( tempOutputPath, tmpPath ); | ||
} | ||
|
||
protected async parseWpressPackage(): Promise< { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to treat this package file as the metafile so we could implement this as a part of the parseMetaFile()
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking in something like that.
Currently it's working and we set correctly the theme, but creating a more abstract way to set it can benefit future adaptations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean to create a more abstract way. I meant to use the abstraction that already exists (parseMetaFile
), instead of adding new one (parseWpressPackage
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code to reuse the parseMetaFile function.
Now MetaFileData has WP and PHP versions as optional and template and stylesheet also optionals.
Changed on: 4a08615
@sejas thanks for taking this over. I've just updated the testing steps. It works well for me, allowing me to import a .wpress file. I noticed two issues so far:
|
@wojtekn , thanks for reviewing it.
Good catch! It took me a while find that it was failing only in the add site form. UbxWD5.mp4
I've found that wpress creates a variable with the active plugins on package.json > Plugins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found that wpress creates a variable with the active plugins on package.json > Plugins.
I'm creating a sql file to activate them. 952f8d6
I couldn't make it work - after importing .wpress from the Atomic site, none of the plugins is active.
} ); | ||
|
||
rl.on( 'line', ( line: string ) => { | ||
writeStream.write( line.replace( /SERVMASK_PREFIX/g, 'wp' ) + '\n' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you refer that we should add a comment explaining why why we replace this fake prefix? It seems that wpress replaces the original prefix with SERVMASK_PREFIX, probably to avoid prefix collisions when importing to a shared database.
Yep, I was curious why we need this. Thanks @sejas for explaining it 🙇 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎊 ! I confirmed I can import a .wpress
backup file and themes and plugins match the original site.
Great work @sejas !
I've added some comments but none should block the PR.
@sejas following #497 (comment), it would be great to add unit tests to cover the import of this backup type. WDYT? |
Related #496 and https://github.com/Automattic/dotcom-forge/issues/8380
Proposed Changes
Adds support for importing
.wpress
files from All-in-One WP Migration and Backup.A lot of this code was generated by Cursor + Claude 😊
Props to https://github.com/ofhouse/wpress-extract/blob/main/lib/wpress-extract.js for guidance on how
.wpress
files are structured.Testing Instructions
Pre-merge Checklist