Skip to content
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

Heartbeat: Improve xmlrpc request argument handling #10074

Merged
merged 2 commits into from Sep 3, 2018
Merged

Conversation

oskosk
Copy link
Contributor

@oskosk oskosk commented Aug 29, 2018

Fixes #10070

Props to @kraftbj for finding the source of the issue.

The XML-RPC server sets a default param of array() via https://github.com/WordPress/WordPress/blob/4.9.8/wp-includes/IXR/class-IXR-message.php#L17
and it passes that to the method via https://github.com/WordPress/WordPress/blob/4.9.8/wp-includes/IXR/class-IXR-server.php#L65

The proposal here is based on a personal opinion that using generate_stats_array was being overloaded in usage as both a direct xmlrpc method handler and a statically callable method for other purposes.

Changes proposed in this Pull Request:

  • Adds a new static method Jetpack_Heartbeat:: xmlrpc_data_response method as handler for the XML RPC request jetpack.getHeartbeatData.

Testing instructions:

  • Start with a connected Jetpack site, and WP_DEBUG WP_DEBUG_LOG constants set to true.
  • Pass your site through the Jetpack Debugger.
  • Expect not to see PHP notices logged. Before this changeset, you should have expected to see PHP notices logged as described in Heartbeat: Array to string conversion PHP notice #10070 .

Proposed changelog entry for your changes:

@oskosk oskosk added [Type] Bug When a feature is broken and / or not performing as intended [Feature] WPCOM API General [Status] In Progress labels Aug 29, 2018
@oskosk oskosk added this to the 6.5 milestone Aug 29, 2018
@oskosk oskosk requested a review from a team as a code owner August 29, 2018 19:43
@jetpackbot
Copy link

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@oskosk
Copy link
Contributor Author

oskosk commented Aug 29, 2018

not quite sure if introducing the handler method is necessary or even tidier than just checking in generate_stats_array() 🤷‍♂️

@oskosk oskosk added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Aug 29, 2018
@kraftbj
Copy link
Contributor

kraftbj commented Aug 30, 2018

I haven't tested it, but I agree with your approach as it future-proofs us better IMO than keeping it within generate_stats_array. As we develop more via REST, etc, we avoid having to setup special checks to account for each variant within that function. We can have handler functions with each special use all referring to a clean and canonical data source.

Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

LGTM - tested via the debugger and ran a test using the wp jetpack get-heartbeat command in wpcom.

Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

I wonder what all this was breaking on the WP.com side.

Looks good to me.

@oskosk oskosk removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Sep 3, 2018
@oskosk oskosk merged commit c36c5e2 into master Sep 3, 2018
@oskosk oskosk deleted the fix/heartbeat branch September 3, 2018 20:49
oskosk added a commit that referenced this pull request Sep 3, 2018
* Add handler of xmlrpc request for heartbeat data wrapping generate_stats_array call
@oskosk
Copy link
Contributor Author

oskosk commented Sep 3, 2018

Ported to branch-6.5 in 7ec33a9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WPCOM API General [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants