-
Notifications
You must be signed in to change notification settings - Fork 798
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.
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
Social | Standardize connections API schema #40589
base: trunk
Are you sure you want to change the base?
Social | Standardize connections API schema #40589
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
a771347
to
22b9c81
Compare
Expanding on #40589 this attempts to refactor the wpcom/v2 endpoints and bring the get_all_connections_for_user method to publicize-base. It means we start relying on the XMLRPC calls entirely, which could be a performance problem with testing the connections. There's probably some more refactoring we could do to proxy the endpoints and use those within Publicize, by overriding some of the methods, like get_publicize_conns_test_results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been expanding on this, and added the implementation of get_all_connections_for_user
to the Publicize_Base
class.
I realise we want to get away from dumping everything in there, but I wonder if this is the first knot to unpick, and then we can start moving things into separate classes or traits.
What do you think of #40604 ?
projects/plugins/wpcomsh/wpcom-features/class-wpcom-features.php
Outdated
Show resolved
Hide resolved
@@ -153,7 +153,7 @@ public static function setup_connections_wpcom() { | |||
|
|||
public static function setup_connections_jetpack() { | |||
set_transient( | |||
'jetpack_social_connections', | |||
'jetpack_social_connection_list', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to be more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned in the PR description
Changed the transient name for connections to ensure to clear old transients to avoid data mismatch
This reverts commit 1cccf43.
Supercedes #40536
Proposed changes:
get_all_connections_for_user
abstract method inPublicize_Base
classget_all_connections_for_user
method in jetpack to return the data as per the new schemaget_all_connections_for_user
method for script data on WPCOMstatus
field towpcom/v2/connection-test-results
endpointOther information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: