remove Jetpack dependencies from the Connection package #18952

leogermani · 2021-02-26T21:59:40Z

This PR removes some Jetpack-specific XMLRPC endpoints from the Connection package and places it in the Jetpack plugin.

This should not change anything for sites with the Jetpack plugin active.

Changes proposed in this Pull Request:

Removes Jetpack specific XMLRPC endpoints from the Connection package

Jetpack product discussion

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

1164484600834010-as-1199928370054538

Testing instructions:

Checkout this branch
Connect the site
Run the Jetpack Debugger and make sure Everything looks great
In the debugger, scroll down to "Tests"
Click on "success: test authenticated xmlrpc request for jetpack passes connection test"
Make sure the response is the Jetpack version, and not the success string
Test requests to the json_api endpoint
Go to Jetpack Dashboard and Disconnect. Make sure disconnection still works

Proposed changelog entry for your changes:

Jetpack plugin extends the Connection package and registers its own XMLRPC methods

test-case-reminder

Here are some suggested test cases for this PR.

Connection

In-place connection with free plan
In-place connection with paid plan
In-place connection with product purchase
Classic connection. Use Safari, or set a constant JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME to true
Disconnect/reconnect connection
Secondary user connection
Connection on multisite

Verify that the changes are compatible with the plugins that use the connection package.

WooCommerce Payments
Jetpack Boost
Previous versions of Jetpack

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

github-actions · 2021-02-26T22:00:00Z

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 🤖

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: April 6, 2021.
Scheduled code freeze: March 29, 2021.

leogermani · 2021-03-01T21:52:58Z

projects/plugins/jetpack/class-jetpack-xmlrpc-methods.php

+	 */
+	public function disconnect_blog() {
+
+		/**


We don't need that wp_set_current_user() bit here because the user is already set at Jetpack_XMLRPC_Server::login -> https://github.com/Automattic/jetpack/blob/update%2Fconnection-package-xmlrpc-independence/projects/packages/connection/legacy/class-jetpack-xmlrpc-server.php#L600

sergeymitr

Works well 👍

jeherve

It looks like the tests are failing at the moment because of the removed methods.

leogermani · 2021-03-10T15:20:33Z

@jeherve @sergeymitr PR is good for review again

anomiex

Gave it a once-over. I'd recommend at least fixing the comment on line 147; the rest I don't care so much about.

projects/packages/connection/tests/php/test_jetpack_xmlrpc_server.php

…ver.php Co-authored-by: Brad Jorsch <[email protected]>

anomiex

I node that jetpack.remoteRegister and jetpack.remoteConnect still call methods on the Jetpack class. Maybe that's out of scope for this PR though.

Haven't manually tested, but it looks like there is test coverage.

anomiex · 2021-03-16T13:47:57Z

projects/packages/connection/legacy/class-jetpack-xmlrpc-server.php

-		 * @param array $core_methods    Available core XML-RPC methods.
+		 * @param array         $jetpack_methods XML-RPC methods available to the Jetpack Server.
+		 * @param array         $core_methods    Available core XML-RPC methods.
+		 * @param \WP_User|bool $user            Information about a given WordPress user. False if authenticated with blog token.


Do we document in which version a parameter was added to a filter?

Yes, thanks for the reminder. Done in 21418d1

This reverts commit 24b7b43.

leogermani · 2021-03-16T16:37:02Z

I node that jetpack.remoteRegister and jetpack.remoteConnect still call methods on the Jetpack class. Maybe that's out of scope for this PR though.

Haven't manually tested, but it looks like there is test coverage.

I've checked all the remote_* methods and asked for confirmation. (see p1615909186005600-slack-CDLH4C1UZ). As I suspected, these are features that the Connection package must provide, so it's a job for another PR to remove the Jetpack dependency for those methods.

I reverted the commit in which I moved remote_provision method to the plugin.

leogermani · 2021-03-24T14:26:28Z

@sergeymitr @jeherve could you have a look at this again? I'd love to get it merged. I'm touching some files in another PR that will likely conflict and I'd also want to use the new test files in the Conncetion package to add some tests in the other PR

sergeymitr · 2021-03-24T16:02:26Z

Hi @leogermani.

Methods in the class Jetpack_XMLRPC_Methods are getting called as static, which they are not, so we get errors like this in the debug.log:

PHP Deprecated:  Non-static method Jetpack_XMLRPC_Methods::json_api() should not be called statically in /var/www/html/wp-includes/IXR/class-IXR-server.php on line 121

It's true for methods test_connection(), disconnect_blog(), json_api().
If you switch them to static, please make sure that they aren't called as object methods anywhere else.

leogermani · 2021-03-24T16:27:12Z

Hi @leogermani.

Methods in the class Jetpack_XMLRPC_Methods are getting called as static, which they are not, so we get errors like this in the debug.log:
PHP Deprecated:  Non-static method Jetpack_XMLRPC_Methods::json_api() should not be called statically in /var/www/html/wp-includes/IXR/class-IXR-server.php on line 121
It's true for methods test_connection(), disconnect_blog(), json_api().
If you switch them to static, please make sure that they aren't called as object methods anywhere else.

Thanks for spotting this @sergeymitr . They should be static, yes. I fixed it.

I also removed an old debugging comment that was there.

sergeymitr

👍

changes were made

remove Jetpack dependencies from the Conn package

ca82eb7

leogermani added the [Status] In Progress label Feb 26, 2021

leogermani added this to the 9.6 milestone Feb 26, 2021

leogermani self-assigned this Feb 26, 2021

test-case-reminder bot reviewed Feb 26, 2021

View reviewed changes

github-actions bot added [Package] Connection [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Feb 26, 2021

leogermani added 2 commits March 1, 2021 18:35

move disconnect xmlrpc method to plugin

0601503

fix hook description and arguments

259e199

leogermani commented Mar 1, 2021

View reviewed changes

moves json_api xmlrpc method to plugin

19458c7

leogermani changed the title ~~remove Jetpack dependencies from the Conn package~~ remove Jetpack dependencies from the Connection package Mar 2, 2021

leogermani requested review from fgiannar and sergeymitr March 5, 2021 13:04

leogermani added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 5, 2021

Fixing a typo in a comment.

e808f10

sergeymitr previously approved these changes Mar 5, 2021

View reviewed changes

sergeymitr 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 Mar 5, 2021

jeherve previously requested changes Mar 8, 2021

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 8, 2021

leogermani added 4 commits March 8, 2021 18:12

Merge branch 'master' into update/connection-package-xmlrpc-independence

a0633e6

add deprecated methods

2b60c75

move remote_provision xmlrpc to the plugin

24b7b43

move xmlrpc_server tests

1dbda8b

leogermani dismissed sergeymitr’s stale review via 1dbda8b March 9, 2021 14:36

Merge branch 'master' into update/connection-package-xmlrpc-independence

4af57c5

leogermani added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 10, 2021

anomiex reviewed Mar 10, 2021

View reviewed changes

leogermani and others added 3 commits March 12, 2021 12:18

Update projects/packages/connection/tests/php/test_jetpack_xmlrpc_ser…

0795656

…ver.php Co-authored-by: Brad Jorsch <[email protected]>

Update projects/packages/connection/tests/php/test_jetpack_xmlrpc_ser…

ea7e1fe

…ver.php Co-authored-by: Brad Jorsch <[email protected]>

use Yoas AssertStringContainsString polyfill

8d61a06

anomiex previously approved these changes Mar 16, 2021

View reviewed changes

leogermani added 2 commits March 16, 2021 12:28

improved docs

21418d1

Revert "move remote_provision xmlrpc to the plugin"

a152032

This reverts commit 24b7b43.

leogermani dismissed anomiex’s stale review via a152032 March 16, 2021 16:34

leogermani added 2 commits March 24, 2021 11:08

Merge branch 'master' into update/connection-package-xmlrpc-independence

971c1af

revert changes on tests

baeabf3

leogermani mentioned this pull request Mar 24, 2021

replace is_active to determine available xmlrpc methods #19264

Merged

sergeymitr added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 24, 2021

making methods static and removing old debug

534cd55

make test_connection static

1b409b3

sergeymitr approved these changes Mar 24, 2021

View reviewed changes

sergeymitr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 24, 2021

leogermani merged commit 39de777 into master Mar 24, 2021

leogermani deleted the update/connection-package-xmlrpc-independence branch March 24, 2021 18:52

matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 24, 2021

remove Jetpack dependencies from the Connection package #18952

remove Jetpack dependencies from the Connection package #18952

Uh oh!

Conversation

leogermani commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

test-case-reminder bot left a comment

Choose a reason for hiding this comment

Connection

Uh oh!

github-actions bot commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leogermani Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

sergeymitr left a comment

Choose a reason for hiding this comment

Uh oh!

jeherve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leogermani commented Mar 10, 2021

Uh oh!

anomiex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anomiex left a comment

Choose a reason for hiding this comment

Uh oh!

anomiex Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

leogermani Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

leogermani commented Mar 16, 2021

Uh oh!

leogermani commented Mar 24, 2021

Uh oh!

sergeymitr commented Mar 24, 2021

Uh oh!

leogermani commented Mar 24, 2021

Uh oh!

sergeymitr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

leogermani commented Feb 26, 2021 •

edited

Loading

github-actions bot commented Feb 26, 2021 •

edited

Loading

jeherve left a comment •

edited

Loading