Plugins Installer: Extract Jetpack plugin installer and create package

[leogermani]

This PR extracts the Jetpack_Plugins class into its own package.

In the future we might want to extract the REST API endpoints that use it to the package as well.

The approach was to keep the same class names and add the new package as a dependency to the Jetpack plugin. Doing that, the autoloader will take care to include the class whenever it's needed.

Changes proposed in this Pull Request:

• extracts the Jetpack_Plugins class into its own package.

Jetpack product discussion

p1643119059163700-slack-CBG1CP4EN

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

No

Testing instructions:

• Set up a new Jurassic Ninja site with this branch
• Activate and connect Jetpack
• Add a new post
• Add a Contact Form block
• On the block options panel, go to "Newsletter Integration"
• Click to install the Creative Email plugin
• Make sure the plugin is properly installed and activated

The above tests the sideloading of plugins. Now let's test the REST endpoint that allows user to upload a zip file to add plugins to their site

• Go to wordpress.com
• Switch site to your testing Jetpack site
• Go to Plugins
• Click "Upload" on the top of the screen
• Upload a zip file with any valid plugin
• Go back to your site's wp-admin and verify that the plugin was installed and activated.

Now let's test the backward compatibility of the changes. Let's open a terminal and do things the old way.

Open a terminal with wp shell

• jetpack_require_lib( 'plugins' ); should return null
• Install a plugin using the old class, for example woocommerce:
• Jetpack_Plugins::install_plugin( 'woocommerce' );
• Verify that the plugin is installed

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ⚠️ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Jetpack plugin:

• Next scheduled release: February 1, 2022.
• Scheduled code freeze: January 24, 2022.

[anomiex]

Gave it a once-over from a monorepo perspective.

It'd be nice if we could rename the class to something like Automattic\Jetpack\PluginsInstaller if there aren't any external uses (as suggested by p1643119495165100/1643119059.163700-slack-CBG1CP4EN).

[anomiex]

Should this be something like this instead?

Suggested change

	$this->main_error_message = ( 'process_failed' === $code && $this->main_error_code ? $this->main_error_code : $message );
	$this->main_error_message = ( 'process_failed' === $code && $this->main_error_message ? $this->main_error_message : $message );

[leogermani]

I think the code is right. It checks for the error code but sets the error message.

[anomiex]

I haven't tried it, but it looks to me that with the current code you'd get the following behavior:

$instance->set_main_error_code( "foo" );
$instance->set_main_error_message( "Message for foo", "foo" );
echo $instance->get_main_error_message() . "\n"; // Prints "Message for foo".
$instance->set_main_error_message( "Unused message", "process_failed" );
echo $instance->get_main_error_message() . "\n"; // Now prints "foo", not "Message for foo".

Whether we should check the code or message in the first part is debatable (personally I'd probably merge set_main_error_code and set_main_error_message into one method to avoid the ambiguity), but the second should certainly not be the code.

[leogermani]

Yeah I get your point now. I'm not sure if we should change the second check against the existence of the code or the message...

I went ahead and merged the PRto unblock the work and will prepare a new one to address this.

Worth saying that this has been there for many many years, I just moved the code around.

[leogermani]

Gave it a once-over from a monorepo perspective.

It'd be nice if we could rename the class to something like Automattic\Jetpack\PluginsInstaller if there aren't any external uses (as suggested by p1643119495165100/1643119059.163700-slack-CBG1CP4EN).

Yes, it definitely would. But I don't see a way of doing this in a fully backward compatible manner, unless we also kept the deprecated classes in the Jetpack plugin, leaving more garbage behind... what do you think?

I just re-added the removed files, leaving them empty with just a comment. We could leave the full classes and add deprecation warnings to all methods - and then replace all usages of it to the new classes...

[anomiex]

The stub files could do like class_alias( Automattic\Jetpack\PluginsInstaller::class, Jetpack_Plugins::class ); to maintain compat.

[leogermani]

Great idea @anomiex

I've done that and updated the test instructions to test the backward compatibility of the changes.

[retrofox]

minor: typo

[retrofox]

Testing the functionality of the implementation according to instructions.

I've added some error_log lines to follow the installing plugin process:

Getting the plugins list

Installing a plugin

Screen.Recording.2022-01-26.at.12.28.56.PM.mov

Activating plugin

It works as expected, too.

[retrofox]

wpcom side

Screen.Recording.2022-01-26.at.12.42.25.PM.mov

I didn't see any activity in the testing site backend, though :-/ Maybe doing something wrong?

[retrofox]

Accepting ✅ but I suggest another couple of 👀 here before merging it.

[leogermani]

I didn't see any activity in the testing site backend, though :-/ Maybe doing something wrong?

@retrofox Did you see the plugin showing up in your site? Calypso won't make a request to the site directly. The access is routed from a request to the public API. On your site you will only see some requests coming in to the XMLRPC endpoint