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

Add import and export logic #110

Conversation

artemiomorales
Copy link
Collaborator

@artemiomorales artemiomorales commented Jan 12, 2023

This pull request contains functionality for exporting and importing changes from a Playground instance. It renders some debug input fields to allow you to play with it.

This PR addresses issue #88. Note: Currently, this only exports and imports the filesystem changes, not the database.

How to Test

  1. This pull request contains two new packages, so run npm ci
  2. Use the input fields to specify a target path and content.
  3. Download the changes using the Export button. You will receive a ZIP file.
  4. Refresh the page.
  5. Import the ZIP file.
import-and-export.mp4

Notes

This currently makes use of the JSZip and FileSaver.js. How do we feel about adding those dependencies?

@artemiomorales
Copy link
Collaborator Author

artemiomorales commented Jan 12, 2023

As a next step, I'd like to incorporate the database changes into the import / export functionality. The default WordPress export works; however, I'm running into an issue with the WordPress Importer, namely that it doesn't install.

I'm not sure what the best approach here is. Maybe the WordPress Importer should be installed by default with Playground. I haven't looked into this in depth yet, but I imagine I'd need to hook into that when doing the import functionality on my end, at least code-wise even if the UI doesn't work.

I could also take a look at this myself as part of this issue and pull request if it would be helpful, though would appreciate any pointers to set me off in the right direction 🙏

wordpress-importer.mp4

@artemiomorales
Copy link
Collaborator Author

Here's the issue I'm running into the WordPress Importer, namely that it doesn't install.

It was pointed out to me that it's possible to install the WordPress Importer and other plugins using a query string as follows:

https://wasm.wordpress.net/wordpress.html?step=playground&plugin=wordpress-importer&url=/wp-admin/index.php

With that in mind, will continue working on this issue and incorporate the database changes as well.

@adamziel
Copy link
Collaborator

adamziel commented Jan 12, 2023

This is awesome @artemiomorales, thank you so much for looking into this! I'll leave some more notes tomorrow, but for now:

JSZip and FileSaver.js. How do we feel about adding those dependencies?

As a rule of thumb, tiny libraries are mostly fine. FileSaver.js is tiny, although I saw it claims support for browsers like IE > 10 – I wonder if the major browsers have caught up and introduced a native function that achieves the same goal without a library?

JsZip is 100kb minified. It may not be much compared to 5MB of WebAssembly code in php.wasm file, but I would still like to add as little on top of that as possible. If it can be replaced by PHP's ZipArchive then great. If not, let's explore loading it async with import() later on (but let's not block this PR on that).

@adamziel
Copy link
Collaborator

adamziel commented Jan 13, 2023

I took it for a spin @artemiomorales and it's lovely!

I tried commenting out the !relativePath.includes('wp-content/database') && check to bring over the .sqlite file as well, but of course the homepage URL wasn't right. Even more interestingly, I saw the following error:

One or more database tables are unavailable. The database may need to be repaired.

I think your intuition behind the importer plugin is right – it should take care of all the necessary adjustments 🤞

I'm not sure what the best approach here is. Maybe the WordPress Importer should be installed by default with Playground.

Yeah it would make sense to bundle it with the default WordPress installation. It would mean adjusting the WordPress build script and committing new bundles to this repo. As a side note, eventually it would be nice if GitHub handled the build pipeline automatically. Until then, we'll need to periodically re-run npm run build:wp:6.1, npm run build:wp:6.0 , etc.

I haven't looked into this in depth yet, but I imagine I'd need to hook into that when doing the import functionality on my end, at least code-wise even if the UI doesn't work.

Take a look at wp-macros.ts where Playground handles logging the user in, installing the plugins etc. I imagine the importer workflow could leverage the same idea of basically controlling the website like a Puppeteer test would.

All in all, splendid work so far!

@artemiomorales artemiomorales force-pushed the try/add-import-export-functionality branch 2 times, most recently from 21d299d to 7bc02d5 Compare January 20, 2023 20:29
@artemiomorales
Copy link
Collaborator Author

@adamziel Here's an updated screen capture showing the current state of this feature.

We now have exporting and importing of the database alongside the file system changes, and all of the ZIP processing is now happening in the worker thread on the PHP side.

import-and-export-update.mp4

Key considerations:

  1. This flow requires the WordPress Importer plugin. That means that one needs to pass in the URL arguments to boot the Playground instance with the plugin installed: http://127.0.0.1:8777/wordpress.html?step=playground&plugin=wordpress-importer
  2. We should start thinking about the user experience. Where does this functionality live? Also, a user needs to be logged in for the import to work. Should we start all users as signed in by default in order to accommodate this?
  3. When updating the theme styles, it looks like the old styles are cached by the browser. I still need to look into invalidating the old styles so we can see theme changes right away.
  4. We'll likely want to ship this feature with tests; I'd like to start looking at adding those this week.

Please let me know your thoughts 🙏

If we can think of a targeted, primary use case of this feature, maybe that can help us nail down the user experience. I'll think on this more.

@adamziel
Copy link
Collaborator

adamziel commented Jan 24, 2023

This is awesome @artemiomorales! Thank you so much for working on this!

Some notes:

That means that one needs to pass in the URL arguments to boot the Playground instance with the plugin installed: http://127.0.0.1:8777/wordpress.html?step=playground&plugin=wordpress-importer

I haven't given the UX part of it much thought yet. Maybe it would make sense to always ship that plugin preinstalled, or at least install it when the import/export feature is enabled? Just like you get logged in as an admin when you request any plugin= in the URL even though there's a separate ?login=1 flag available.

We should start thinking about the user experience. Where does this functionality live? Also, a user needs to be logged in for the import to work. Should we start all users as signed in by default in order to accommodate this?

There's a larger question about the project structure here.

Playground as an isolated site on https://wasm.wordpress.net/wordpress.html has limited usefulness. Its real power is the ability to embed it in other apps.

Some importing/exporting use-cases I came across are:

  • Live plugins and themes previews in WordPress.org directory
  • Bug reports with reproduction attached
  • An app for translators where each translation can be one-click reproduced in the right context
  • Building your own site and then saving it, perhaps even directly to a web host
  • Showcasing preconfigured sites, plugins, themes (e.g. import as a starter content delivery mechanism)

None of these would involve an upload form directly in the playground – they all need a dedicated user interface.

Consider the "settings button" in the "browser window" on https://developer.wordpress.org/playground/. It's not implemented in this repository but is a part of the Wasm Demo Gutenberg block which lives in a dedicated wp.org theme.

All this tells me that an import/export needs to be exposed to developers via some form of API, be it a window.postMessage() or via a query parameters like the ?plugin= we have now.

The playground itself only needs a basic UI to demo the feature. There could be a set of export / import buttons somewhere in the browser UI and all they would do is trigger a file download and bring up a file upload form. They could be visible by default unless disabled with ?export_ui=0 or something to that effect. Ideally they would also be visible in the ?seamless=1 mode where there's no browser chrome visible – I'm not sure how to approach it so we may need to through a few mockups.

What do you think? I'm happy to be wrong about any of these.

When updating the theme styles, it looks like the old styles are cached by the browser. I still need to look into invalidating the old styles so we can see theme changes right away.

WordPress assets are often sourced a version string like ?ver=c24330f635f5cb9d5e0e or ?ver=6.1.1 – there must be a way to update them somehow. Perhaps there's some prior art in some other importing plugins? @dd32 may have some insights

We'll likely want to ship this feature with tests; I'd like to start looking at adding those this week.

That sounds awesome! See the existing tests in src/php-wasm/__tests__. You'll likely need to extract the tested functions to a separate file as loading src/wordpress-playground/index.tsx triggers browser-specific side-effects.

@adamziel
Copy link
Collaborator

adamziel commented Jan 24, 2023

If we can think of a targeted, primary use case of this feature, maybe that can help us nail down the user experience. I'll think on this more.

CC @bengreeley as this is highly relevant for live previews in the plugins and themes directories on WordPress.org

@ironprogrammer
Copy link
Contributor

Great progress, @artemiomorales!

For the caching delay, global styles are currently set to a 1-minute transient. The final fix to core is still being deliberated, but this PR shows promise if you're looking for a patch before 6.2 drops: WordPress/wordpress-develop#3712.

@ironprogrammer
Copy link
Contributor

As follow-up:

For the caching delay, global styles are currently set to a 1-minute transient...

Note that WordPress/wordpress-develop#3712 has been merged: Use a non-persistent object cache instead of transient in wp_get_global_stylesheet().

@artemiomorales
Copy link
Collaborator Author

@adamziel I took a look at the new CLI functionality, but I think it makes more sense to install the WordPress Importer via the Dockerfile.

The reason for this is that, if we use WP-CLI to manage installation and activation of the WordPress Importer, the way that WP-CLI parses the wp-config file conflicts with the removal of whitespace from the PHP files. Since the stripped version is what exists in the WordPress bundle, opting to get the importer with WP-CLI during the build process seems to be the most straightforward way of doing things.

With that in mind, I moved the stripping of the whitespace until after WordPress Importer is installed, making sure to avoid stripping the importer files, as modifying them prevents the plugin from working properly.

I also went ahead and rebuilt all of the WordPress bundles, and the export / import seems to be working in all cases. One issue though is that full site editing in 6.0 and 6.1 appears to have broken during the build process. I'm planning to look further into what's causing that issue.

Another limitation is that we can't go between different versions of WordPress — for example, if you export a WordPress 5.9 instance, you can't import that into a 6.0 instance.

Exporting also only works with PHP versions 7.4 and up, and I'm planning to look into ensuring compatibility with other versions of PHP as well.

I'm considering how far to take this and whether we should cover going between different versions of WordPress right now. I think it may be worth it to document that as a known limitation, and just fix full site editing, PHP compatibility, and the CSS cache issue, along with adding the UI, to ship this and get more eyeballs on it.

Would be happy to hear your thoughts!

@adamziel
Copy link
Collaborator

adamziel commented Jan 31, 2023

@adamziel I took a look at the new CLI functionality, but I think it makes more sense to install the WordPress Importer via the Dockerfile.

That sounds great @artemiomorales! 👍

I also went ahead and rebuilt all of the WordPress bundles, and the export / import seems to be working in all cases. One issue though is that full site editing in 6.0 and 6.1 appears to have broken during the build process. I'm planning to look further into what's causing that issue.

Could that be related to this?

# Pair the site editor's nested iframe to the Service Worker.
#
# Without the patch below, the site editor initiates network requests that
# aren't routed through the service worker. That's a known browser issue:
#
# * https://bugs.chromium.org/p/chromium/issues/detail?id=880768
# * https://bugzilla.mozilla.org/show_bug.cgi?id=1293277
# * https://github.com/w3c/ServiceWorker/issues/765
#
# The problem with iframes using srcDoc and src="about:blank" as they
# fail to inherit the root site's service worker.
#
# Gutenberg loads the site editor using <iframe srcDoc="<!doctype html">
# to force the standards mode and not the quirks mode:
#
# https://github.com/WordPress/gutenberg/pull/38855
#
# This commit patches the site editor to achieve the same result via
# <iframe src="/doctype.html"> and a doctype.html file containing just
# `<!doctype html>`. This allows the iframe to inherit the service worker
# and correctly load all the css, js, fonts, images, and other assets.
#
# Ideally this issue would be fixed directly in Gutenberg and the patch
# below would be removed.
#
# See https://github.com/WordPress/wordpress-playground/issues/42 for more details
RUN echo '<!doctype html>' > wordpress-static/wp-includes/empty.html && \
sed -E 's#srcDoc:"[^"]+"#src:"/wp-includes/empty.html"#g' -i wordpress-static/wp-includes/js/dist/block-editor.min.js && \
sed -E 's#srcDoc:"[^"]+"#src:"/wp-includes/empty.html"#g' -i wordpress-static/wp-includes/js/dist/block-editor.js

Another limitation is that we can't go between different versions of WordPress — for example, if you export a WordPress 5.9 instance, you can't import that into a 6.0 instance.

That's a good point! Is this how WordPress works in general? Or is that specific to Playground? If it's the latter – what makes Playground different?

Exporting also only works with PHP versions 7.4 and up, and I'm planning to look into ensuring compatibility with other versions of PHP as well.

I wonder if that's because of the following part of the PHP build process:

# PHP <= 7.3 is not very good at detecting the presence of the POSIX readdir_r function
# so we need to force it to be enabled.
RUN if [[ "${PHP_VERSION:0:1}" -le "7" && "${PHP_VERSION:2:1}" -le "3" ]] || [ "${PHP_VERSION:0:1}" -le "5" ]; then \
echo '#define HAVE_POSIX_READDIR_R 1' >> /root/php-src/main/php_config.h; \
fi;

Does the readdir function work properly? If not, we may have to implement one for PHP <= 7.3 (or google for an existing implementation – perhaps someone already solved that).

I'm considering how far to take this and whether we should cover going between different versions of WordPress right now. I think it may be worth it to document that as a known limitation, and just fix full site editing, PHP compatibility, and the CSS cache issue, along with adding the UI, to ship this and get more eyeballs on it.

A known limitation sounds great 👍

@adamziel
Copy link
Collaborator

adamziel commented Feb 2, 2023

I just checked the readdir function on PHP <= 7.3 and it works correctly:

		console.log(
			new TextDecoder().decode(this.run({
				code: `<?php
			$handle = opendir('/');
			while (false !== ($entry = readdir($handle))) { echo "$entry\n"; }`,
			}).body)
		);

The problem you're running into @artemiomorales must be caused by something else then.

@artemiomorales
Copy link
Collaborator Author

One issue though is that full site editing in 6.0 and 6.1 appears to have broken during the build process.

For anyone following along, @adamziel and I took a look at this together — it was related to the build process deleting a file needed for full site editing to work. That's now been fixed on trunk.

That's a good point! Is this how WordPress works in general? Or is that specific to Playground? If it's the latter – what makes Playground different?

Not sure! We've determined though that this can be a known limitation for now. I've made a note to create an issue for this.

Does the readdir function work properly? If not, we may have to implement one for PHP <= 7.3 (or google for an existing implementation – perhaps someone already solved that).
...
I just checked the readdir function on PHP <= 7.3 and it works correctly:

This is something I'll also look into later — will make an issue for it.

@artemiomorales
Copy link
Collaborator Author

I just pushed up the UI for this migration logic. Here's a demo. Would be happy to hear feedback 🙏

Note: The cache issue still exists. I tried patching it using the fix on WordPress trunk, but it seems getting that bug cleared may be beyond the scope of this feature; likely we can ship with the cache issue as a known limitation until the next WP version is released.

cc @adamziel

migration-logic-ui-1080.mp4

@adamziel
Copy link
Collaborator

This is lovely @artemiomorales! A few notes from me:

  • Let's add a note in the modal that you can upload the previously exported files and that cache will auto-refresh after one minut
  • Does it work with lower PHP versions? If not, what's the error? We could look into that together.

Otherwise it looks great and I'm excited to get it in! CC @fabiankaegy

@adamziel
Copy link
Collaborator

adamziel commented Feb 10, 2023

Ah, and could this PR not affect the rebuilt assets in wp-content etc? That's a lot of files and I don't think any of them matters – the only thing that matters is shipping the PHP importer plugin in the .data bundle. I see most of the changes are in file permissions from 0755 to 0644 – that could be safely ignored.

…cludes/' and 'wp-content/database/' directories
Added back the empty.html file that is needed for full site
editing to work.

Removed an erroneous command in the package.json that was being
used to build WordPress bundles, as our intent is for specific
versions of WordPress to be specified when building WP bundles.
Export functionality was not working due to
PHP not being able to read a file after it had been
written. This was due to insufficient file permissions;
that has now been fixed.

I also added the PHP version to the name of the
export file.
In order to test the behavior of the migration feature's PHP code,
I moved that code to separate files so that it can be run by Jest.
This required modifying the generateZipFile() function a bit to accept
parameters.
Moved part of the import logic to separate file, modified it to
use parameters, and added many constants in the migration test file
to prevent code duplication.
It turns out the code was not importing files if their parent
directories didn't exist. That has now been fixed, and the code
has also been moved to a separate file to allow for testing.
@artemiomorales artemiomorales force-pushed the try/add-import-export-functionality branch from 19bfb46 to 0badcd2 Compare February 24, 2023 09:25
@artemiomorales
Copy link
Collaborator Author

@adamziel Tests have been added; I even caught a bug while adding them 🙌

Let me know what you think. If all looks good, I think this feature is ready to ship.

@adamziel adamziel marked this pull request as ready for review February 24, 2023 09:35
) as HTMLButtonElement;
const importCloseModalButton = document.querySelector(
'#import-close-modal--btn'
) as HTMLButtonElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's about time this entire file gets migrated to (p)react – that's definitely out of scope of this PR, though

@adamziel
Copy link
Collaborator

Thank you @artemiomorales ! I left a few nitpicks but it looks good overall. I'd like to merge as soon as these are addressed.

For posterity, here's a few tasks I'd like to look into next:

  • Migrate the Playground app to React – the custom HTML is getting unwieldy
  • Introduce a general createFiles() function that accepts an object and creates a file tree
  • Fix this TypeScript problem: error TS2307: Cannot find module './migration.php' or its corresponding type declarations.
  • Add a PHP linter

All of these are out of scope for this PR but would greatly benefit the repository.

@artemiomorales artemiomorales merged commit 6ef8e8d into WordPress:trunk Feb 24, 2023
@artemiomorales
Copy link
Collaborator Author

For posterity, here's a few tasks I'd like to look into next:

  • Migrate the Playground app to React – the custom HTML is getting unwieldy
  • Introduce a general createFiles() function that accepts an object and creates a file tree
  • Fix this TypeScript problem: error TS2307: Cannot find module './migration.php' or its corresponding type declarations.
  • Add a PHP linter

@adamziel Great, this PR has been merged and I created issues for all of the above.

@adamziel
Copy link
Collaborator

adamziel commented Mar 8, 2023

@artemiomorales This is great!

As a side note, please use the squash&merge feature when merging future PRs to avoid merging the entire PR history to trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants