Skip to content
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

Website Query API: Add import-site and import-content options #610

Merged
merged 12 commits into from
Dec 11, 2023

Conversation

eliot-akira
Copy link
Collaborator

@eliot-akira eliot-akira commented Jul 28, 2023

What is this PR doing?

Adds query parameter import-site and import-content to the Playground website's Query API.

  • import-site - Imports site files and database from a zip file specified by URL.
  • import-content - Imports site content from a WXR or WXZ file specified by URL.

What problem is it solving?

Resolves #608.

Testing Instructions

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

This is looking clean and small 👍

packages/playground/website/src/lib/make-blueprint.tsx Outdated Show resolved Hide resolved
packages/playground/website/src/lib/make-blueprint.tsx Outdated Show resolved Hide resolved
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

can we also add an item to the table of options in the Query API docs?

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

it seems fine to allow import without auto-login, and we can add something better afterwards. your call is good to auto-login or import given our options.

packages/docs/site/docs/08-query-api/01-index.md Outdated Show resolved Hide resolved
@adamziel
Copy link
Collaborator

adamziel commented Dec 1, 2023

I left one minor comment @eliot-akira. The only other thing I'd change is I'd call this option importFile to be consistent with the related Blueprint step AND avoid confusion with the future importWpContentDirectory option (name TBD) I'm planning to add. Other than that, it looks great and I'd love to ship this, thank you! ❤️ It would help with #718, too!

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented Dec 1, 2023

The only other thing I'd change is I'd call this option importFile

OK, will do.

when the login step fails, it fails silently. That's not ideal..but we could actually always try to log the user in ..until the optional steps work lands. Would you please update this PR?

Sure, I will make the change. Just to confirm my understanding: so for now it will import the site and attempt to auto-login with the default user credentials. Even if the login fails, the rest of the steps (like install theme and plugins) should continue to run.

When the "optional steps" feature is ready, then this login step can be made optional so that it doesn't throw an error and stop the install (in case the imported site doesn't have the default admin user).

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented Dec 2, 2023

OK, I made the suggested changes.

I noticed that the query parameter login was documented but not implemented (to disable auto-login with value 0), so I added it - but it conflicted with the trunk branch which had it already. So I reverted that commit.

More importantly.. Recently I was catching up on new developments in the Playground repo, and learned that the importFile step was unintentionally removed in PR #780 (and reported in issue #833).

That means this current PR shouldn't be merged until the importFile step is restored.

But on that topic: importFile was the only step that is not "isomorphic" because it used DOMParser, which is not implemented in Node.js, to interact with the admin screen for WordPress Importer. Ideally, rather than restoring the step as it was, it might be better to write a new function that directly imports such files using PHP.run(). Maybe it could be temporarily restored until the new one is ready.

This also made me wonder, the name importFile is a bit ambiguous what kind of file it accepts. Looking at the last existing state of that step here, I see it expects a WXR/WXZ file. Previously I thought it accepted a zip export of the whole site - hence the title of this PR, "install site from zip file URL". The original issue that prompted this PR also meant this:

In this case I suppose the query parameter would be better named importSite. (Or better yet, import only the contents of wp-content as in the new importWordPressFiles step.)

@adamziel
Copy link
Collaborator

adamziel commented Dec 2, 2023

That means this current PR shouldn't be merged until the importFile step is restored.

Oh you're right! I just started a PR to restore the importFile step #835

But on that topic: importFile was the only step that is not "isomorphic" because it used DOMParser, which is not implemented in Node.js

I wonder what would it take to call the appropriate PHP functions directly, I like that idea. Alternatively, we could use the new WP_HTML_Tag_Processor on PHP side to replace the DOMParser.

In this case I suppose the query parameter would be better named importSite. (Or better yet, import only the contents of wp-content as in the new importWordPressFiles step.)

I like the idea about reorganizing the names of these steps. importSite is also a bit ambiguous but it's better than importFile. How about splitting the importFile step into two distinct steps called importWxr, importWxz, and then renaming importWpFiles to importWpContentDirectory?

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented Dec 4, 2023

I wonder what would it take to call the appropriate PHP functions directly, I like that idea. Alternatively, we could use the new WP_HTML_Tag_Processor on PHP side to replace the DOMParser.

It probably depends on how WordPress Importer works internally, if its import function is written in a way that's callable independently and programmatically, or if it's tightly coupled with the admin screen and form submit flow. In the latter case, maybe the majority of the existing importFile step can be kept while replacing DOMParser with HTML tag processor, which is a great idea.

How about splitting the importFile step into two distinct steps called importWxr, importWxz

That sounds good to me, to include the expected file format in the step name. Or maybe importContent step that accepts both WXR/WXZ formats, distinguished by the file extension.

and then renaming importWpFiles to importWpContentDirectory?

For this one, if there's a single common format to export/import a site (in its entirety or just the wp-content directory), then I kinda still like importSite as a short way to describe what it does. By chance I found the original pull request where the import/export functions were implemented.

So there are ways to:

  • Export and import the whole site (all files and database): zipEntireSite (previously called exportFile) and replaceSite
  • Export and import the site content (partial database): exportWXR, exportWXZ, and submitImporterForm

Looking at this, I can imagine there may be other export formats in the future, like: a "site diff" for only the changed files and database operations; or some kind of JSON format to create content; or importing an SQL file. In that case, it may be better to have step/function names that specify the expected file format, instead of abstract terms like "content" and "site".

I tend to err on the side of user convenience, short and easy to type, like an import step that accepts any supported export format (WXR/WXZ/ZIP/JSON/SQL..) - as they say, "be liberal in what you accept", the so-called Robustness Principle - but sometimes that can cause ambiguity and confusion, or restrict future extensibility, so I understand there's value in being explicit, specific, and even verbose.

@aehlke
Copy link

aehlke commented Dec 4, 2023

For user research you may look at what specific import/export options popular cloud wordpress providers offer. I believing they typically have ways to replace sites (between staging/prod etc) as well as maybe ways to sync content across environments without necessarily syncing full site config but I'm rusty on WP now

@adamziel
Copy link
Collaborator

adamziel commented Dec 6, 2023

@eliot-akira this is unblocked now as #835 is merged.

importContent step that accepts both WXR/WXZ formats, distinguished by the file extension.

importContent sounds great! Let's do that. It also handles both wxr and wxz using the same admin page so maybe it doesn't make as much sense to split that logic in two separate steps.

I kinda still like importSite as a short way to describe what it does

That's fair, and I guess we'll need to go beyond wp-content anyway as, say, wp-config.php lives in WordPress root. Alright, let's settle for importContent and importSite.

I tend to err on the side of user convenience, short and easy to type, like an import step that accepts any supported export format (WXR/WXZ/ZIP/JSON/SQL..)

I like that idea! That could be an entirely new step that's a higher-level abstraction – it knows how to detect the input format, and then call a specific format-specific step.

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented Dec 7, 2023

Alright, let's settle for importContent and importSite.

[The import step] could be an entirely new step that's a higher-level abstraction – it knows how to detect the input format, and then call a specific format-specific step.

That sounds nice for a public interface like the Query API.

OK, I'll work on the above points. It sounds simple enough, as a thin layer to create a blueprint step from the given query parameter. (I guess these steps cannot be used together? Maybe importSite gets priority, and if there's importContent specified, it can run after it.)


For user research you may look at what specific import/export options popular cloud wordpress providers offer.

@aehlke - Thank you for the suggestion. It's a good idea to see what existing import/export solutions there are.

It reminds me of WP-CLI, how it has commands to import/export the database, and particularly its search-replace command which is necessary to update the site URL after importing a new database. Also, having such a blueprint step could help solve an issue like this one, which is caused by hardcoded site URL in the database:

It also makes me wonder whether it's possible in the future to run blueprint steps on regular WP sites outside of Playground. As the Blueprint API evolves, the more it seems like a useful abstraction for setting up a WordPress site in any context, including wp-env and for general-purpose use.

@adamziel
Copy link
Collaborator

adamziel commented Dec 7, 2023

It sounds simple enough, as a thin layer to create a blueprint step from the given query parameter. (I guess these steps cannot be used together? Maybe importSite gets priority, and if there's importContent specified, it can run after it.)

Actually I’m having second thoughts here. It’s easy to pattern match the file extension, but then extending support to, say, plugins or different site export formats could require unzipping the archive and reasoning about its contents — which feels like a big scope creep.

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented Dec 7, 2023

plugins or different site export formats could require unzipping the archive and reasoning about its contents

True, there could be multiple export formats that use the zip file extension. I agree, guessing the export format from zip contents doesn't seem like a good idea.

If we can't rely on file extension to tell us the expected format - that means, instead of importSite and importContent, we should let the user specify the format in the step name (and query parameter name). Maybe..

  • importWxr
  • importWxz
  • importSiteZip
  • importSiteDiffZip
  • importWpContentZip
  • importPluginZip
  • importContentJson

It does feel a bit redundant to have the file extension in the step/query name, if they are the same.

How about, if in the future we have different site export formats, their corresponding import/export step names could specify what they are..?

@adamziel
Copy link
Collaborator

How about, if in the future we have different site export formats, their corresponding import/export step names could specify what they are..?

I like that!

@eliot-akira
Copy link
Collaborator Author

OK, the following options have been added to the website's Query API.

(These example URLs can be visited after running npm run dev.)


Also updated the documentation page to describe these options. Some points to note:

  • import-site (zip) and import-content (wxr/wxz) options can be used together. The steps are run in this order:
    • first it imports the site,
    • then logs in with the default admin user
    • then imports content.
  • import-content option uses the WordPress Importer, so the default admin user must be logged in. It means the login option must be 1 (default) and the login must succeed.
  • Added note about CORS policy

image

From what I understand, this applies to all query parameters that accept a URL, namely: blueprint-url, plugin, theme, import-site, and import-content.

@eliot-akira eliot-akira marked this pull request as ready for review December 11, 2023 15:49
@eliot-akira eliot-akira changed the title Website: Add query parameter "import" to install site from zip file URL Website: Add query parameter import-site (zip) and import-content (wxr/wxz) Dec 11, 2023
@eliot-akira eliot-akira changed the title Website: Add query parameter import-site (zip) and import-content (wxr/wxz) Website: Add query parameter import-site and import-content Dec 11, 2023
@eliot-akira eliot-akira changed the title Website: Add query parameter import-site and import-content Website Query API: Add import-site and import-content options Dec 11, 2023

:::info CORS policy

To import files from a URL, such as a site zip package, they must be served with `Access-Control-Allow-Origin` header set. For reference, see: [Cross-Origin Resource Sharing (CORS)](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#the_http_response_headers).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lovely note, thank you @eliot-akira! ❤️

@@ -33,11 +33,19 @@ You can go ahead and try it out. The Playground will automatically install the t
| `url` | `/wp-admin/` | Load the specified initial page displaying WordPress |
| `mode` | `seamless`, `browser`, or `browser-full-screen` | Displays WordPress on a full-page or wraps it in a browser UI |
| `lazy` | | Defer loading the Playground assets until someone clicks on the "Run" button |
| `login` | `1` | Logs the user in as an admin |
| `login` | `1` | Logs the user in as an admin. Set to `0` to not log in. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, thank you @eliot-akira! I know it's not directly related to this PR, but it's such a small and useful change – let's keep it.

@adamziel
Copy link
Collaborator

adamziel commented Dec 11, 2023

  • The testing instructions work
  • The change works
  • The tests are passing
  • The doc was updated

This is amazing @eliot-akira, thank you so much! ❤️

@adamziel adamziel merged commit 25006aa into WordPress:trunk Dec 11, 2023
5 checks passed
adamziel added a commit that referenced this pull request Dec 11, 2023
…review links (#212)

The preview links attached to Pull Requests exported to GitHub are now
using the new `import-site` Query API param merged in #610. Before this
commit, these preview URLs used Blueprints, which were more verbose and
difficult to modify by hand.
adamziel added a commit that referenced this pull request Dec 11, 2023
…review links (#861)

The preview links attached to Pull Requests exported to GitHub are now
using the new `import-site` Query API param merged in #610. Before this
commit, these preview URLs used Blueprints, which were more verbose and
difficult to modify by hand.

## Testing Instructions

1. Export a PR to GitHub, select "Also export a .zip file"
2. Confirm the preview link in the PR description uses the
`?import-site=` query parameter and not a Blueprint
3. Confirm the preview link works
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query API: accept import GET parameter to grab a whole WordPress zip and load all its content.
4 participants