Skip to content

Connection: Instantiate the XML-RPC Connector after adding Bootstrap Methods #13139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
zinigor merged 4 commits into from
Jul 30, 2019
Merged

Connection: Instantiate the XML-RPC Connector after adding Bootstrap Methods #13139

zinigor merged 4 commits into from
Jul 30, 2019

Conversation

@mdawaffe
Copy link
Member

@mdawaffe mdawaffe commented Jul 26, 2019

Fixes #13136, #13084.

Changes proposed in this Pull Request:

The bootstrap filter clears out all existing XML-RPC methods, so adding the connector methods first does not work.

With this new order, the connector's methods are appended to the bootstrap methods.

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

Fixes a bug.

Testing instructions:

Confirm that a new site connects via XML-RPC.

Proposed changelog entry for your changes:

Bring back the ability to connect to WordPress.com via XML-RPC or REST API.

@mdawaffe mdawaffe added Connect Flow Connection banners, buttons, ... [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Jul 26, 2019
@mdawaffe mdawaffe added this to the 7.6 milestone Jul 26, 2019
@mdawaffe mdawaffe requested a review from a team July 26, 2019 23:16
@mdawaffe mdawaffe self-assigned this Jul 26, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Jul 26, 2019 •
edited
Loading

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.

Scheduled Jetpack release: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against 67e43b0

oskosk
oskosk previously approved these changes Jul 26, 2019
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

:impressive-quake:

@mdawaffe
Copy link
Member Author

mdawaffe commented Jul 27, 2019

I added some tests in 51d3ec0. They required a refactor.

Do we want this here or in a different PR?

lezama
lezama reviewed Jul 29, 2019
View reviewed changes
class.jetpack.php
}
}

function setup_xmlrpc_handlers( $request_params, $is_active, $is_signed, Jetpack_XMLRPC_Server $xmlrpc_server = null ) {
Copy link
Contributor

@lezama lezama Jul 29, 2019

Choose a reason for hiding this comment

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

should we move this to the connection package?

Copy link
Contributor

@zinigor zinigor Jul 29, 2019

Choose a reason for hiding this comment

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

What do you say we move everything into the connection package in #12699 later? This block depends on a lot of static methods that I don't feel comfortable moving right before the code freeze.

zinigor
zinigor approved these changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Love the tests, thank you for fixing the problem!

zinigor
zinigor suggested changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Ouch, just noticed failing Travis tests :)

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Jul 29, 2019
zinigor
zinigor previously approved these changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

I know it's probably backwards, but I have amended the expectations in the failing tests to include XMLRPC methods that were added by publicize and subscription methods.

mdawaffe and others added 3 commits July 29, 2019 22:19
@mdawaffe @jeherve
Connection: Instantiate the XML-RPC Connector after adding Bootstrap …
9b923d4 
…Methods

The bootstrap filter clears out all existing XML-RPC methods, so adding
the connector methods first does not work.

With this new order, the connector's methods are appended to the bootstrap
methods.

Fixes #13136, #13084.
@mdawaffe @jeherve
Adds tests. Refactor required.
e51d346
@zinigor @jeherve
Fixed expectations to make tests pass.
88eb891
@jeherve jeherve force-pushed the fix/xmlrpc-connector branch from be2b662 to 88eb891 Compare July 29, 2019 20:19
@jeherve
Copy link
Member

jeherve commented Jul 29, 2019

Rebased to fix the tests. Testing now.

@jeherve
Copy link
Member

jeherve commented Jul 29, 2019

Tests are still failing I am afraid :(

@jeherve 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 Jul 29, 2019
@mdawaffe mdawaffe 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 Jul 30, 2019
@mdawaffe
Copy link
Member Author

mdawaffe commented Jul 30, 2019

Tests pass now.

zinigor
zinigor approved these changes Jul 30, 2019
View reviewed changes
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

This is perfect, thank you!

@zinigor zinigor merged commit 63e9744 into master Jul 30, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Jul 30, 2019
@zinigor zinigor deleted the fix/xmlrpc-connector branch July 30, 2019 06:46
@zinigor zinigor mentioned this pull request Jul 30, 2019
Closed
jeherve added a commit that referenced this pull request Jul 30, 2019
@jeherve
changelog: add #13139
2c793d8
jeherve added a commit that referenced this pull request Jul 30, 2019
@jeherve
7.6: running changelog (#12940)
f1d6377 
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
jeherve added a commit that referenced this pull request Jul 30, 2019
@jeherve
7.6: running changelog (#12940)
37aabd8 
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
@jeherve jeherve mentioned this pull request Jul 31, 2019
Closed
iandunn added a commit to WordPress/wordcamp.org that referenced this pull request Aug 1, 2019
@iandunn
Jetpack Tweaks: Restore XMLRPC Connector so sites can register.
2e0d789 
74765c1 whitelisted Jetpack's REST API endpoints, but that wasn't enough to allow sites to connect to WordPress.com without XMLRPC.

Automattic/jetpack#13139 will restore the XMLRPC Connector in Jetpack proper once the upcoming version is released, so it's not worth wasting time investigating why the REST endpoints aren't enough. The hotfix in this commit should restore the ability to connect in the meantime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lezama lezama lezama left review comments

+2 more reviewers

@zinigor zinigor zinigor approved these changes

@oskosk oskosk oskosk left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mdawaffe mdawaffe

Labels

Connect Flow Connection banners, buttons, ...

Projects

None yet

Milestone

7.6

Development

Successfully merging this pull request may close these issues.

Connection: XMLRPC_Connector can never work

8 participants

@mdawaffe @jetpackbot @jeherve @lezama @zinigor @oskosk @matticbot