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.

Sign up for GitHub

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

Edit Site: Add theme exporter. #22922

Merged
merged 2 commits into from
Jun 9, 2020
Merged

Edit Site: Add theme exporter. #22922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c22979
Edit Site: Add theme exporter.
epiqueras Jun 4, 2020
45b9b81
Lib: Make new API experimental and prefix functions.
epiqueras Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions lib/edit-site-export.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php
/**
* REST endpoint for exporting the contents of the Edit Site Page editor.
*
* @package gutenberg
*/

/**
* Output a ZIP file with an export of the current templates
* and template parts from the site editor, and close the connection.
*/
function gutenberg_edit_site_export() {
// Create ZIP file and directories.
$filename = tempnam( get_temp_dir(), 'edit-site-export' );
$zip = new ZipArchive();
$zip->open( $filename, ZipArchive::OVERWRITE );
$zip->addEmptyDir( 'theme' );
Copy link
Member

Choose a reason for hiding this comment

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

Why not put block-templates and block-template-parts at the top-level? (Unless I'm wrong about typical theme .zip files having these directories on the top-level)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because unzipping a zip file yields its contents, not its contents wrapped in an extra directory. Try downloading the file to see what I mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe make a constant for the theme directory name? We might later want to add an option for the user to give a name to the theme 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can change it then. I don't see much value in complicating this very simple code with lots of string joins when it's not needed yet.

$zip->addEmptyDir( 'theme/block-templates' );
$zip->addEmptyDir( 'theme/block-template-parts' );

// Load files into ZIP file.
foreach ( get_template_types() as $template_type ) {
// Skip 'embed' for now because it is not a regular template type.
// Skip 'index' because it's a fallback that we handle differently.
if ( in_array( $template_type, array( 'embed', 'index' ), true ) ) {
continue;
}

$current_template = gutenberg_find_template_post_and_parts( $template_type );
if ( isset( $current_template ) ) {
$zip->addFromString( 'theme/block-templates/' . $current_template['template_post']->post_name . '.html', $current_template['template_post']->post_content );

foreach ( $current_template['template_part_ids'] as $template_part_id ) {
$template_part = get_post( $template_part_id );
$zip->addFromString( 'theme/block-template-parts/' . $template_part->post_name . '.html', $template_part->post_content );
}
}
}

// Send back the ZIP file.
$zip->close();
header( 'Content-Type: application/zip' );
header( 'Content-Disposition: attachment; filename=edit-site-export.zip' );
header( 'Content-Length: ' . filesize( $filename ) );
flush();
echo readfile( $filename );
Comment on lines +41 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we clean up, delete the file? Especially since it's using temporary filenames, so we're not just overwriting the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an issue with keeping it around? Deleting it would technically be more work.

Copy link
Member

Choose a reason for hiding this comment

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

If we do make changes here we should also switch to wp_tempnam which I missed the first time round.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious if that's just for consistency/style or is there some benefit to it?

Copy link
Member

Choose a reason for hiding this comment

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

I think its mostly consistency and style, but it does appear to have some edge case handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue with keeping it around?

I don't think we can make assumptions on how often the location returned by get_temp_dir() is flushed. We must assume that ZIP files would proliferate over time.

Deleting it would technically be more work.

How so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?

You always have to create it/overwrite it. Deleting it is another set of operations we could avoid, although I doubt it makes a measurable difference.

I think we can be safe here by using the same file name every time. That way it just overwrites the same file if it hasn't been cleaned up already.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, @epiqueras and I talked about this over Slack, and my recommendation was to ensure good random filenames (WordPress's wp_tempnam might be better at avoiding naming conflicts) and ensure we delete the file in the moment.

This stands in contrast with adopting a fixed filename so as to overwrite the file over time. This might be enough in simple sites, but is bound to cause race-condition-type problems in any larger installation, and especially in multisite or highly concurrent scenarios.

die();
}
add_action(
'rest_api_init',
function () {
register_rest_route(
'__experimental/edit-site/v1',
'/export',
array(
'methods' => 'GET',
'callback' => 'gutenberg_edit_site_export',
'permission_callback' => function () {
return current_user_can( 'edit_theme_options' );
},
)
);
}
);
1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,5 +100,6 @@ function gutenberg_is_experiment_enabled( $name ) {
require dirname( __FILE__ ) . '/experiments-page.php';
require dirname( __FILE__ ) . '/customizer.php';
require dirname( __FILE__ ) . '/edit-site-page.php';
require dirname( __FILE__ ) . '/edit-site-export.php';
require dirname( __FILE__ ) . '/editor-features.php';
require dirname( __FILE__ ) . '/global-styles.php';
2 changes: 1 addition & 1 deletion lib/template-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ function gutenberg_find_template_post_and_parts( $template_type, $template_hiera

if ( $current_template_post ) {
$template_part_ids = array();
if ( is_admin() ) {
if ( is_admin() || defined( 'REST_REQUEST' ) ) {
noahtallen marked this conversation as resolved.
Show resolved Hide resolved
foreach ( parse_blocks( $current_template_post->post_content ) as $block ) {
$template_part_ids = array_merge( $template_part_ids, create_auto_draft_for_template_part_block( $block ) );
}
Expand Down
6 changes: 6 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/edit-site/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"@wordpress/plugins": "file:../plugins",
"@wordpress/primitives": "file:../primitives",
"@wordpress/url": "file:../url",
"downloadjs": "^1.4.7",
"file-saver": "^2.0.2",
"jszip": "^3.2.2",
"lodash": "^4.17.15",
Expand Down
29 changes: 29 additions & 0 deletions packages/edit-site/src/plugins/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
/**
* External dependencies
*/
import downloadjs from 'downloadjs';

/**
* WordPress dependencies
*/
import { MenuItem } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { registerPlugin } from '@wordpress/plugins';
import { addQueryArgs } from '@wordpress/url';
import apiFetch from '@wordpress/api-fetch';

/**
* Internal dependencies
Expand All @@ -16,6 +22,29 @@ registerPlugin( 'edit-site', {
return (
<>
<ToolsMoreMenuGroup>
<MenuItem
role="menuitem"
icon="download"
onClick={ () =>
apiFetch( {
path: '/__experimental/edit-site/v1/export',
parse: false,
} )
.then( ( res ) => res.blob() )
.then( ( blob ) =>
downloadjs(
Copy link
Member

Choose a reason for hiding this comment

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

Is there not a built-in way to download a file from WordPress? I don't like that we have to add a whole new dependency for it 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of. It's a tiny browser-compat shim.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just "link" to the download URL instead of using apiFetch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to persist the file in a public directory. This seemed cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that's not needed since the request already sends a zip file but maybe there's something I'm missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you mean a link to the URL using an href. I thought you meant having the API return the link to the file.

That won't work either, because we need the nonce middleware and all that for authentication. Unless you think there is another way to authenticate there?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe if you use a link, you might not need the nonces and things like that as it's a "GET" request and it uses the session right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it, but it's still needed.

The API fetch is also a "GET" request.

Copy link
Contributor

@mcsf mcsf Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's definitely weird to bring in downloadjs just for this, even if it's not a huge lib.

The limitation with directly accessing the endpoint lies not in the nonce requirement, since that can be satisfied with the _wpnonce GET parameter, but in the authentication itself.

My reflex here was that we should try to make this work with plain URLs, but after digging a bit I don't think it's very practical.

However, depending on downloadjs is a little unfortunate, and looking at its source it seems like it does a lot of legwork to support older browsers that we don't need to care about anymore, not to mention that it supports different download methods (e.g. URL-based). Could we replace it with something dead-simple? The following works for me in my browser console:

function downloadBlob( data, filename, mimeType ) {
  const blob = new Blob( [ data ], { type: mimeType } );
  const url = URL.createObjectURL( blob );
  const anchor = document.createElement( 'a' );
  anchor.download = filename;
  anchor.href = url;
  document.body.appendChild( anchor );
  anchor.click();
  document.body.removeChild( anchor );
}

downloadBlob( '<html>...</html>', 'home.html', 'text/html' );

If this is worthwhile I can open a PR.

blob,
'edit-site-export.zip',
'application/zip'
)
)
}
info={ __(
'Download your templates and template parts.'
) }
>
{ __( 'Export' ) }
</MenuItem>
<MenuItem
role="menuitem"
href={ addQueryArgs( 'edit.php', {
Expand Down