Skip to content

Conversation

@jeherve
Copy link
Member

@jeherve jeherve commented Aug 4, 2020

Changes proposed in this Pull Request:

  • New Plugins management endpoints to be used by both blocks and the admin page.
  • Rework of the Akismet plugin activation endpoint to use that.
  • Shared utility so blocks can leverage the new endpoints.

Background

In WordPress 5.5, new plugin management endpoints will be added: 50321-core. Those will be used in the block editor by the block directory features. Those endpoints are useful, and could be useful in Jetpack as well to further integrations between our blocks and other plugins for example. #16519 is a good example of an integration that uses the existing v4/plugins endpoint, but could benefit from an endpoint allowing you to install the plugin.

That said, those endpoints will not be available for us in Jetpack for a while, since we will be supporting folks using WordPress 5.4 until WordPress 5.6 comes out.

Until then, let's extend our current Jetpack endpoints to replicate some of what Core has added. I'd suggest trying to stick to the core implementation in our parameters and responses so we can deprecate our endpoints and switch to Core's once we drop support for WordPress 5.4.

Caveat

I had to add a new optional request parameter to the endpoints so we can pass the source of the request see pbtFPC-H5-p2 for more details.
That makes us diverge from the Core implementation, so it would make deprecating our endpoints once WP 5.5 is the minimum required version a bit more difficult. I could not think of a better alternative though.

Jetpack product discussion

  • Related discussion: p1HpG7-9NB-p2

Does this pull request change what data or activity we track or use?

  • No

Testing instructions:

  • Start from a free site with both the Akismet plugin and the Jetpack CRM plugin installed but not active, and with the Contact form module active.
  • Go to Posts > Add New and add the Form block.
  • In the sidebar, you should see the CRM integration setting. It will do a query to get the list of plugins on your site when you open the field. That query should succeed and you should see an invitation to activate the plugin.
  • In Jetpack > Dashboard, you should be able to activate the Akismet plugin (and be redirected to the Akismet configuration page upon activation).
  • Testing the block management tools is a bit more difficult. I would recommend checking out Jetpack CRM: auto-install and activate plugin from form settings #16825 instead, and following the instructions there.

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Pri] Normal [Feature] WP REST API labels Aug 4, 2020
@jeherve jeherve self-assigned this Aug 4, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Aug 4, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16713

Scheduled Jetpack release: September 1, 2020.
Scheduled code freeze: August 25, 2020

Generated by 🚫 dangerJS against 0b7f8c6

@kbrown9
Copy link
Member

kbrown9 commented Aug 5, 2020

I received a fatal error when I sent a POST request to the new jetpack/v4/plugins endpoint. Have you encountered this?

[05-Aug-2020 03:52:59 UTC] PHP Fatal error:  Uncaught Error: Call to undefined function Automattic\Jetpack\Sync\Modules\get_plugins() in /var/www/html/wp-content/plugins/jetpack/packages/sync/src/modules/class-plugins.php:98
Stack trace:
#0 /var/www/html/wp-includes/class-wp-hook.php(289): Automattic\Jetpack\Sync\Modules\Plugins->populate_plugins()
#1 /var/www/html/wp-includes/plugin.php(206): WP_Hook->apply_filters()
#2 /var/www/html/wp-admin/includes/class-wp-upgrader.php(487): apply_filters()
#3 /var/www/html/wp-admin/includes/class-wp-upgrader.php(794): WP_Upgrader->install_package()
#4 /var/www/html/wp-admin/includes/class-plugin-upgrader.php(108): WP_Upgrader->run()
#5 /var/www/html/wp-content/plugins/jetpack/_inc/lib/plugins.php(71): Plugin_Upgrader->install()
#6 /var/www/html/wp-content/plugins/jetpack/_inc/lib/class.core-rest-api-endpoints.php(3486): Jetpack_Plugins::install_plugin()
#7 /var/www/html/wp-includes/rest-api/class-wp-rest-server.php(1015): Jetpack_Core_Json_Api_Endpoints::install_plugin()
#8 /var/www/html/wp-includes/rest-api/class in /var/www/html/wp-content/plugins/jetpack/packages/sync/src/modules/class-plugins.php on line 98

It looks like Automattic\Jetpack\Sync\Modules\Plugins::populate_plugins might need to check for the get_plugins():

if ( ! function_exists( 'get_plugins' ) ) {
	require_once ABSPATH . 'wp-admin/includes/plugin.php';
}

@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Aug 11, 2020
@jeherve
Copy link
Member Author

jeherve commented Aug 11, 2020

I received a fatal error when I sent a POST request to the new jetpack/v4/plugins endpoint. Have you encountered this?

Good catch. This should be solved in 6d3aa98

@jeherve jeherve force-pushed the add/plugin-management-endpoints branch from 0190009 to 16e9cb2 Compare August 13, 2020 14:15
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Aug 13, 2020
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Tested following the instructions and the other PR and everything worked as expected.

The only thing I miss are... tests! Would be nice to have tests for those new endpoints.

Another thing not directly related to this PR is the message that appears in the block configuration when Jetpack CRM is active:

"Contacts from this form will be stored in Jetpack CRM if the CRM Jetpack Forms extension is active."

It feels ambiguous to me. We know the plugin is active, so we should not have the "if" there. What do you think?

@leogermani leogermani added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 14, 2020
Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

I tested using the provided testing instructions, and the new endpoints worked well for me. I also tested the endpoints directly with valid and invalid args, and everything looked good.

@kbrown9
Copy link
Member

kbrown9 commented Aug 17, 2020

Another thing not directly related to this PR is the message that appears in the block configuration when Jetpack CRM is active:

"Contacts from this form will be stored in Jetpack CRM if the CRM Jetpack Forms extension is active."

It feels ambiguous to me. We know the plugin is active, so we should not have the "if" there. What do you think?

To integrate form submission into the Jetpack CRM contacts, two things need to be active: the Jetpack CRM plugin and the CRM's Jetpack Forms extension:

Annotation on 2020-08-16 at 21-42-10

I think your point is valid. New users that just installed Jetpack CRM probably won't know that Jetpack CRM has extensions, so the current text is confusing and should be changed. We can handle that in a different PR since it's not related to the new endpoints.

@jeherve
Copy link
Member Author

jeherve commented Aug 17, 2020

The only thing I miss are... tests! Would be nice to have tests for those new endpoints.

That's a good point! I can take care of that in a follow-up PR. How do you think I should go about mocking plugin lists and plugin installation / activation?

@jeherve jeherve merged commit de74386 into master Aug 17, 2020
@jeherve jeherve deleted the add/plugin-management-endpoints branch August 17, 2020 12:06
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 17, 2020
@jeherve jeherve added the [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. label Aug 17, 2020
@jeherve
Copy link
Member Author

jeherve commented Aug 18, 2020

r212300-wpcom

@leogermani
Copy link
Contributor

The only thing I miss are... tests! Would be nice to have tests for those new endpoints.

That's a good point! I can take care of that in a follow-up PR. How do you think I should go about mocking plugin lists and plugin installation / activation?

I think there's a good chance WorDBless can handle it, because it basically involves writing/reading options.

Another thing I forgot to mention: our debug helper plugin now has an API tester. So for PRs like this you can rely on it for testing new endpoints without the need of writing new code:

Captura de Tela 2020-08-18 às 15 03 19

props to @sergeymitr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Admin Page React-powered dashboard under the Jetpack menu [Feature] WP REST API [Plugin] CRM Issues about the Jetpack CRM plugin [Pri] Normal [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants