Add API endpoints and Jetpack Backup package for managing Helper Scripts #13830

thingalon · 2019-10-24T12:43:56Z

Project details: pa0RFL-oP-p2

Prototype P2 post: pa0RFL-ra-p2
Related wpcom patch: D34445-code
Related transport patch: 975-gh-jetpack-backups

This PR adds support for Jetpack Backup Helper Scripts. It includes two new API endpoints; one for installing a Jetpack Backup Helper Script, and one for deleting them. It also adds a scheduled action to cleanup any expired Helper Scripts after any get installed.

Changes proposed in this Pull Request:

New module; Jetpack Backup - for helper methods for the backup system. For now, it only contains one class; a class for managing Helper Scripts.
New API endpoints; one for installing a Jetpack Backup Helper Script, and one for deleting one.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This adds support for Zero-Config Backups to Jetpack.

Testing instructions:

See instructions on the transport patch: 975-gh-jetpack-backups

Proposed changelog entry for your changes:

Support for Jetpack Backups with simpler configuration.

matticbot · 2019-10-24T12:44:03Z

Caution: This PR has changes that must be merged to WordPress.com
Hello thingalon! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D34452-code before merging this PR. Thank you!

jetpackbot · 2019-10-24T12:46:08Z

	Warnings
⚠️	`pre-commit` hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 50c7922

matticbot · 2019-10-24T15:05:12Z

thingalon, Your synced wpcom patch D34452-code has been updated.

seear · 2019-10-24T15:05:19Z

Rebased to master.

packages/backup/composer.json

packages/backup/actions.php

jeherve · 2019-10-25T08:21:05Z

Related discussion: p9ue0y-81-p2

jeherve

I can't really test this without a VP sandbox, so my comments will mostly be nitpicking.

What do you think about doing some cleanup in uninstall.php to make sure we always clean after ourselves if someone deletes the plugin from their site?

There are quite a few PHPCS errors and warning that will stop you from committing changes to those files. Could you fix those errors?

json-endpoints/jetpack/class.jetpack-json-api-delete-backup-helper-script.php

json-endpoints/jetpack/class.jetpack-json-api-install-backup-helper-script.php

json-endpoints/jetpack/json-api-jetpack-endpoints.php

packages/backup/src/Helper_Script_Manager.php

json-endpoints/jetpack/class.jetpack-json-api-install-backup-helper-script.php

packages/backup/src/Helper_Script_Manager.php

tyxla

I love how readable and both self-documented and well-documented this code is. Thanks for the great work!

I haven't tested it, but I've left some drive-by comments and questions in case they can be useful.

tyxla · 2019-10-28T09:21:57Z

json-endpoints/jetpack/class-jetpack-json-api-install-backup-helper-script-endpoint.php

+
+use Automattic\Jetpack\Backup\Helper_Script_Manager;
+
+class Jetpack_JSON_API_Install_Backup_Helper_Script_Endpoint extends Jetpack_JSON_API_Endpoint { 


Have you considered creating those endpoints as v2 endpoints (like this one)? I thought we try to create new endpoints with this mechanism, rather than the legacy v1 one.

Just in case it's helpful, this post is a good place to read more about the different types of endpoints: PCYsg-aqU-p2

packages/backup/composer.json

tyxla · 2019-10-28T09:26:33Z

packages/backup/src/Helper_Script_Manager.php

+		for ( $attempt = 0; $attempt < $max_attempts; $attempt++ ) {
+			$file_key           = wp_generate_password( 10, false );
+			$relative_file_path = trailingslashit( $relative_temp_dir ) . 'jp-helper-' . $file_key . '.php';
+			$absolute_file_path = trailingslashit( ABSPATH ) . $relative_file_path;


Should this be \ABSPATH because we're in a different namespace from the global one here?

Yes, probably better to be explicit. This instance of ABSPATH was refactored out based on previous feedback just as your comments were added -- but I've added \ to the new instance of ABSPATH (and similar constants) in the latest version.

Thanks for the feedback! :)

Thank you! Also, noting that we should be able to use the constants package to safely check and retrieve constants.

packages/backup/src/Helper_Script_Manager.php

thingalon · 2019-10-28T09:39:43Z

Thanks for the review @jeherve

What do you think about doing some cleanup in uninstall.php to make sure we always clean after ourselves if someone deletes the plugin from their site?

Good idea; added. Thanks! :)

There are quite a few PHPCS errors and warning that will stop you from committing changes to those files. Could you fix those errors?

Cleaned up! Thanks :)

…lated to Jetpack Backup

…content and upload locations without making assumptions

…ace, remove two unused variables

seear · 2019-10-28T18:51:08Z

Rebased to master.

seear · 2019-10-28T19:28:06Z

Is there some extra step necessary to use the latest automattic/jetpack-backup package on the Jetpack site?

Failing Travis build because of the wrong filenames in whitelist was the problem.

… check for it in the global namespace.

seear · 2019-10-28T20:39:46Z

I've re-tested using latest patches D34445-code and 975-gh-jetpack-backups. The helper script uploads and a backup runs.

jeherve

I can't test all of this since I don't have a VP sandbox, but the Jetpack side looks good. I'm merging this now to make testing easier.

Conversation at p9ue0y-81-p2 is still ongoing so keeping an eye on that too.

D34452-code, D34445-code, and 975-gh-jetpack-backups will also need to be merged.

* 7.9: Changelog * Update version number * Update stable tag and tested up to * Changelog: add #13530 * changelog: add #13578 * Changelog: add #13598 * Changelog: add entry for numerous block preview changes * Changelog: add #13599 * changelog: add #13541 * Changelog: add #13542 * Changelog: add #13331 * Changelog: add #13558 * Changelog: add #13409 * Changelog: add #13582 * Changelog: add #13600 * Changelog: add #13601 * Changelog: add #13595 * Changelog: add #12695 * Changelog: add #13009 * Changelog: add #13649 * Changelog: add #13450 * Changelog: add #13507 * Changelog: add #13658 * Changelog: add #13687 * changelog: add #13683 * Changelog: add #9323 * Changelog: add #13681 * Fix typos in readme * Add link to WordPress Beta Tester plugin * Changelog: add #13630 * Changelog: add #13695 * Changelog: add #13659 * Changelog: add #13716 * Changelog: add #13664 * Changelog: add #13682 * Changelog: add #13362 * Changelog: add #13563 * Add testing list for #13563 * Changelog: add #13735 * Changelog: add #13752 * Changelog: add #13624 * Changelog: add #13756 * Changelog: add #13745 * Changelog: add #13728 * Changelog: add #13779 * Changelog: add #13699 * Changelog: add #13804 * Changelog: add #13761 * Changelog: add #13637 * Changelog: add #13517 * Changelog: add #13521 * Changelog: add #13729 * Testing list: add testing instructions for #13729 * Changelog: add sync changes * Changelog: add #13807 * Changelog: add #13654 * Changelog: add #13795 * Changelog: add #13801 * Changelog: add #13818 * Changelog: add #13725 * Changelog: add #13831 * Changelog: add #13516 * Testing list: add Twenty Twenty instructions * Changelog: add #13799 * Changelog: add #13805 * Changelog: add #13688 * Changelog: add #13830

- Project: https://[private link] - P2 post about this prototype: https://[private link] - Related Jetpack PR: #13830 - Related Rewind PR: https://github.com/Automattic/jetpack-backups/pull/975 This adds two new API endpoints for calling out to a Jetpack site, to install or delete a Helper Script. It also locks these new API endpoints down to require access through a Rewind API token, ensuring this is not called from any other source. This commit was generated from D34445-code.

* Add endpoints for Jetpack Backup Helper Script methods - Project: https://[private link] - P2 post about this prototype: https://[private link] - Related Jetpack PR: #13830 - Related Rewind PR: Automattic/jetpack-backups#975 This adds two new API endpoints for calling out to a Jetpack site, to install or delete a Helper Script. It also locks these new API endpoints down to require access through a Rewind API token, ensuring this is not called from any other source. This commit was generated from D34445-code. * Make the require_rewind_auth parameter optional

…13922) * Add endpoints for Jetpack Backup Helper Script methods - Project: https://[private link] - P2 post about this prototype: https://[private link] - Related Jetpack PR: #13830 - Related Rewind PR: Automattic/jetpack-backups#975 This adds two new API endpoints for calling out to a Jetpack site, to install or delete a Helper Script. It also locks these new API endpoints down to require access through a Rewind API token, ensuring this is not called from any other source. This commit was generated from D34445-code. * Make the require_rewind_auth parameter optional

thingalon added [Status] In Progress [Feature] Backup & Scan labels Oct 24, 2019

thingalon self-assigned this Oct 24, 2019

matticbot added the Touches WP.com Files label Oct 24, 2019

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 24, 2019

seear marked this pull request as ready for review October 24, 2019 14:33

seear requested a review from a team October 24, 2019 14:33

seear force-pushed the add/jetpack-backups-helper-apis branch from be288b5 to 3578e5d Compare October 24, 2019 15:05

seear added this to the 7.9 milestone Oct 24, 2019

seear added the [Pri] High label Oct 24, 2019

enejb reviewed Oct 24, 2019

View reviewed changes

packages/backup/composer.json Show resolved Hide resolved

enejb reviewed Oct 24, 2019

View reviewed changes

packages/backup/actions.php Outdated Show resolved Hide resolved

thingalon added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Oct 25, 2019

thingalon requested a review from seear October 25, 2019 03:07

jeherve removed the [Status] In Progress label Oct 25, 2019

jeherve requested changes Oct 25, 2019

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 25, 2019

thingalon requested a review from a team as a code owner October 28, 2019 02:41

thingalon force-pushed the add/jetpack-backups-helper-apis branch from 55a2e0a to 2b2eb7e Compare October 28, 2019 02:44

tyxla reviewed Oct 28, 2019

View reviewed changes

thingalon added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 28, 2019

jeherve force-pushed the add/jetpack-backups-helper-apis branch from a622cb4 to f82df05 Compare October 28, 2019 17:01

Mark George added 5 commits October 28, 2019 18:46

Change Jetpack Backups -> Jetpack Backup, also clean up formatting

0d28467

Tidy up alignment of => markers

15c5b72

Use a more explicit header comment; making it clearer that this is re…

44db4a1

…lated to Jetpack Backup

Use WP_CONTENT_DIR, WP_CONTENT_URL and wp_upload_dir to determine wp-…

c6d322e

…content and upload locations without making assumptions

Explicitly use constants and WordPress methods from the global namesp…

e089413

…ace, remove two unused variables

seear force-pushed the add/jetpack-backups-helper-apis branch from f82df05 to 67039aa Compare October 28, 2019 18:46

Mark George and others added 2 commits October 28, 2019 19:43

Escape the backslash in function_exists for WP_Filesystem to actually…

cf2b048

… check for it in the global namespace.

Correct endpoint filenames in phpcs whitelist

34b187e

seear force-pushed the add/jetpack-backups-helper-apis branch from 7e46b32 to 34b187e Compare October 28, 2019 19:44

Missing comma

50c7922

jeherve approved these changes Oct 29, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 29, 2019

jeherve merged commit 07d0f36 into master Oct 29, 2019

jeherve deleted the add/jetpack-backups-helper-apis branch October 29, 2019 12:17

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 29, 2019

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Oct 29, 2019

jeherve added a commit that referenced this pull request Oct 29, 2019

Changelog: add #13830

b5751ee

jeherve removed the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 29, 2019

thingalon mentioned this pull request Nov 1, 2019

Add security settings for Helper Script API methods #13922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API endpoints and Jetpack Backup package for managing Helper Scripts #13830

Add API endpoints and Jetpack Backup package for managing Helper Scripts #13830

thingalon commented Oct 24, 2019 •

edited

Loading

matticbot commented Oct 24, 2019

jetpackbot commented Oct 24, 2019 •

edited

Loading

matticbot commented Oct 24, 2019

seear commented Oct 24, 2019

jeherve commented Oct 25, 2019

jeherve left a comment

tyxla left a comment

tyxla Oct 28, 2019

tyxla Oct 28, 2019

thingalon Oct 28, 2019

tyxla Oct 28, 2019

thingalon commented Oct 28, 2019

seear commented Oct 28, 2019

seear commented Oct 28, 2019

seear commented Oct 28, 2019

jeherve left a comment •

edited

Loading


		use Automattic\Jetpack\Backup\Helper_Script_Manager;

		class Jetpack_JSON_API_Install_Backup_Helper_Script_Endpoint extends Jetpack_JSON_API_Endpoint {

Add API endpoints and Jetpack Backup package for managing Helper Scripts #13830

Add API endpoints and Jetpack Backup package for managing Helper Scripts #13830

Conversation

thingalon commented Oct 24, 2019 • edited Loading

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Oct 24, 2019

jetpackbot commented Oct 24, 2019 • edited Loading

matticbot commented Oct 24, 2019

seear commented Oct 24, 2019

jeherve commented Oct 25, 2019

jeherve left a comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

tyxla Oct 28, 2019

Choose a reason for hiding this comment

tyxla Oct 28, 2019

Choose a reason for hiding this comment

thingalon Oct 28, 2019

Choose a reason for hiding this comment

tyxla Oct 28, 2019

Choose a reason for hiding this comment

thingalon commented Oct 28, 2019

seear commented Oct 28, 2019

seear commented Oct 28, 2019

seear commented Oct 28, 2019

jeherve left a comment • edited Loading

Choose a reason for hiding this comment

thingalon commented Oct 24, 2019 •

edited

Loading

jetpackbot commented Oct 24, 2019 •

edited

Loading

jeherve left a comment •

edited

Loading