Site Editor: Add missing export endpoint #1936

Mamaduka · 2021-11-24T07:42:02Z

Trac ticket: https://core.trac.wordpress.org/ticket/54448

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Mamaduka · 2021-11-24T08:07:30Z

@TimothyBJacobs I'm not sure how to fix the fixtures/wp-api-generated.js error. Can you point me to developer documentation on how to fix this?

desrosj · 2021-11-24T14:55:59Z

@Mamaduka You need to run the REST API unit tests in PHP, and then a new version of that file will get generated!

We should add an npm script to make that easier and more clear how to accomplish that.

Mamaduka · 2021-11-24T16:00:53Z

Thanks, @desrosj.

Fixtures are updated now.

src/wp-includes/block-template-utils.php

youknowriad · 2021-11-26T11:04:36Z

src/wp-includes/block-template-utils.php

+ *
+ * @return string Updated wp_template content.
+ */
+function _remove_theme_attribute_in_block_template_content( $template_content ) {


I don't know how we organize things in general in Core but for me, I don't think we should change this file as the added functions are specific to the export behavior and could be moved to the export controller directly (private methods there) or their own file. WDYT?

I see this file as the API to fetch block templates and template parts.

We can't test the private method in PHP, but we can make it public.

Also, make sense to have a remove method when we have an inject one.

I don't know how we organize things in general in Core

I'm also not sure about this. I talked with @noisysocks, and we decided to keep these methods in the utils file for now.

Mmm, I don't like that we're kind of changing the role of the "utils" file but I can live with it. We should also do the same in the plugin if we go that road to keep the files similar to easy backports...

Done - WordPress/gutenberg#36908

Initially, I was planning to have these methods private, but then I got reminded that public API could be helpful for WP CLI and similar tools - WordPress/gutenberg#36559 (comment).

Maybe we can have a separate file for site export methods once API is more mature in the future.

youknowriad

Do you think there's a chance of things breaking in the Gutenberg plugin if we commit this? I guess we should potentially follow-up and move this code in the Gutenberg plugin into compat/wordpress-5.9

Mamaduka · 2021-11-26T11:11:35Z

Thanks for the reminder. I need to rename the method in Gutenberg, and we should be good. All methods and files are in the compat folder and have the function_exists guard clause.

walbo

A few gutenberg texdomains that shouldn't be in core. Other than that it looks good to me 👍

src/wp-includes/block-template-utils.php

src/wp-includes/rest-api/endpoints/class-wp-rest-edit-site-export-controller.php

Co-authored-by: Petter Walbø Johnsgård <[email protected]>

…ort-controller.php Co-authored-by: Petter Walbø Johnsgård <[email protected]>

Mamaduka · 2021-11-29T05:02:44Z

Thanks, @walbo.

peterwilsoncc

I've added a few notes inline.

peterwilsoncc · 2021-11-30T02:48:12Z

src/wp-includes/rest-api/endpoints/class-wp-rest-edit-site-export-controller.php

+	 * @return WP_Error|bool True if the request has access, or WP_Error object.
+	 */
+	public function permissions_check() {
+		if ( current_user_can( 'edit_theme_options' ) ) {


There is a capability export that is usually used for exporting site contents but I am wondering if it should be checked here too? If so, possibly in addition to edit_theme_options to make sure the has permission to view what they are exporting.

I'll need to consider this, I don't have an answer as to whether it's needed or not as yet.

I'm also not sure, so I kept the cap that was used in the original code. I think we might need new caps for block theme exports in the long run.

Yeah I don't have a solid answer either. As you said, I think export is more on the side of site data, whereas this is less about site content but appearance. It feels like it may be a candidate for a dedicated meta capability that maps to both by default?

Thanks both. Let's consider this but not treat is as a blocker for getting this patch in. As is, I think the permission check is fit for purpose.

tests/phpunit/tests/block-template-utils.php

src/wp-includes/block-template-utils.php

peterwilsoncc · 2021-11-30T03:19:43Z

src/wp-includes/block-template-utils.php

+	if ( ! class_exists( 'ZipArchive' ) ) {
+		return new WP_Error( __( 'Zip Export not supported.' ) );
+	}


Is the UI hidden if ZipArchive is unavailable?

This line will be needed even if it is but I just want to make sure it is.

Currently, UI is always visible since we will have to make a new request to check if ZipArchive is available.

In my testing, the export feature did not fail gracefully.

Screen.Recording.2021-11-30.at.16.33.44.mov

# Conflicts: # src/wp-includes/rest-api.php # src/wp-settings.php # tests/qunit/fixtures/wp-api-generated.js

peterwilsoncc · 2021-11-30T03:56:26Z

@Mamaduka I've merged trunk in to your branch to resolve some merge conflicts

Co-authored-by: Peter Wilson <[email protected]>

peterwilsoncc

Thanks for those changes, functional code changes all look good.

I missed a couple of docblocks that need a short summary in my review yesterday, sorry, have added some suggestions inline.

peterwilsoncc · 2021-11-30T03:58:42Z

src/wp-includes/block-template-utils.php

+/**
+ * Creates an export of the current templates and
+ * template parts from the site editor at the
+ * specified path in a ZIP file.
+ *
+ * @since 5.9.0
+ *
+ * @return WP_Error|string Path of the ZIP file or error on failure.
+ */


Suggested change

/**

* Creates an export of the current templates and

* template parts from the site editor at the

* specified path in a ZIP file.

*

* @since 5.9.0

*

* @return WP_Error|string Path of the ZIP file or error on failure.

*/

/**

* Generate export file for block templates.

*

* Creates an export of the current templates and

* template parts from the site editor at the

* specified path in a ZIP file.

*

* @since 5.9.0

*

* @return WP_Error|string Path of the ZIP file or error on failure.

*/

peterwilsoncc · 2021-12-01T02:52:08Z

tests/phpunit/tests/block-template-utils.php

+	 * @dataProvider data_remove_theme_attribute_in_block_template_content
+	 */
+	function test_remove_theme_attribute_in_block_template_content( $template_content, $expected ) {
+		$this->assertEquals( $expected, _remove_theme_attribute_in_block_template_content( $template_content ) );


Suggested change

$this->assertEquals( $expected, _remove_theme_attribute_in_block_template_content( $template_content ) );

$this->assertSame( $expected, _remove_theme_attribute_in_block_template_content( $template_content ) );

peterwilsoncc · 2021-12-01T03:46:03Z

src/wp-includes/rest-api/endpoints/class-wp-rest-edit-site-export-controller.php

+	/**
+	 * Output a ZIP file with an export of the current templates


Suggested change

/**

* Output a ZIP file with an export of the current templates

/**

* Output ZIP archive of export.

*

* Output a ZIP file with an export of the current templates

Another one that really needs a short summary ahead of the description.

The first line is pulled out as a short summary in the developer docs, for example https://developer.wordpress.org/reference/functions/wp_remote_get/

Mamaduka · 2021-12-05T11:05:11Z

Thanks for the fantastic feedback, @peterwilsoncc.

I always struggle with DocBlock comments. Now that API got merged, should I create separate PR for those.

Mamaduka added 3 commits November 24, 2021 11:40

Site Editor: Add missing export endpoint

191d85d

Don't use RDONLY constant in tests

e74a881

Use numFiles

275656f

Update WP API fixtures

f441c4d

youknowriad reviewed Nov 26, 2021

View reviewed changes

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

Use new folder names

55cab98

youknowriad reviewed Nov 26, 2021

View reviewed changes

Mamaduka mentioned this pull request Nov 26, 2021

Site Editor: Sync export API WordPress/gutenberg#36908

Merged

7 tasks

youknowriad approved these changes Nov 26, 2021

View reviewed changes

walbo suggested changes Nov 28, 2021

View reviewed changes

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

src/wp-includes/rest-api/endpoints/class-wp-rest-edit-site-export-controller.php Outdated Show resolved Hide resolved

Mamaduka and others added 3 commits November 29, 2021 05:02

Remove texdomain

b02f8f3

Co-authored-by: Petter Walbø Johnsgård <[email protected]>

Update src/wp-includes/block-template-utils.php

0d5916e

Co-authored-by: Petter Walbø Johnsgård <[email protected]>

Update src/wp-includes/rest-api/endpoints/class-wp-rest-edit-site-exp…

2b40531

…ort-controller.php Co-authored-by: Petter Walbø Johnsgård <[email protected]>

peterwilsoncc reviewed Nov 30, 2021

View reviewed changes

Merge branch 'trunk' into add/edit-site-export

f7fe3c4

# Conflicts: # src/wp-includes/rest-api.php # src/wp-settings.php # tests/qunit/fixtures/wp-api-generated.js

Mamaduka and others added 4 commits November 30, 2021 05:20

Ticket DocBlock

48dd608

Co-authored-by: Peter Wilson <[email protected]>

Use plain language

0473b9c

Co-authored-by: Peter Wilson <[email protected]>

Improve nesting

8388d27

Co-authored-by: Peter Wilson <[email protected]>

Use data provider

80bbb6d

spacedmonkey self-requested a review November 30, 2021 13:44

spacedmonkey self-assigned this Nov 30, 2021

spacedmonkey added 2 commits November 30, 2021 16:07

Merge branch 'trunk' into add/edit-site-export

8aec83d

Tweaks and add comments.

7c78e4f

spacedmonkey requested review from peterwilsoncc and youknowriad November 30, 2021 16:34

peterwilsoncc reviewed Dec 1, 2021

View reviewed changes

spacedmonkey closed this Dec 14, 2021

Mamaduka deleted the add/edit-site-export branch April 7, 2022 10:12

	$this->assertEquals( $expected, _remove_theme_attribute_in_block_template_content( $template_content ) );
	$this->assertSame( $expected, _remove_theme_attribute_in_block_template_content( $template_content ) );

		/**
		* Output a ZIP file with an export of the current templates

Site Editor: Add missing export endpoint #1936

Site Editor: Add missing export endpoint #1936

Uh oh!

Conversation

Mamaduka commented Nov 24, 2021

Uh oh!

Mamaduka commented Nov 24, 2021

Uh oh!

desrosj commented Nov 24, 2021

Uh oh!

Mamaduka commented Nov 24, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Mamaduka commented Nov 26, 2021

Uh oh!

walbo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mamaduka commented Nov 29, 2021

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwilsoncc commented Nov 30, 2021

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mamaduka commented Dec 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants