File sync: JSON API: Add new wrapper function on WordPress.com to match Jetpack-only funct… #12133
Conversation
…ion. Summary: When syncing Jetpack files with WordPress.com, we sometimes run into issues because Jetpack and WordPress.com have 2 different functions to check if a module is active: `is_active_module` on WordPress.com, `is_module_active` in Jetpack. This diff fixes that by adding a new `is_module_active` function on WordPress.com. Test Plan: Once this is merged, we should be able to sync PRs like this one with no issues. #10120 Reviewers: dereksmart, kraftbj, mdawaffe, migueluy, jeherve Reviewed By: dereksmart, kraftbj, mdawaffe Subscribers: gibrown, mdawaffe, kraftbj Tags: #touches_jetpack_files Differential Revision: https://[private link] Merges r190254-wpcom.
| $jetpack_search_active = is_jetpack_module_active( 'search', $blog_id ); | ||
| } | ||
| $jetpack_search_active = | ||
| $jetpack_search_supported |
There was a problem hiding this comment.
This looks like it may be changing the behavior? Before, $jetpack_search_supported is set to false on Jetpack, but it isn't used except in the response of the API. After, it is used in $jetpack_search_active which used to just check if the module was active for a Jetpack site (is_jetpack_module_active is wpcom only).
There was a problem hiding this comment.
Right. tl;dr with these changes, it's impossible for a Jetpack site to ever be true for $jetpack_search_active
@zinigor can you take a look?
There was a problem hiding this comment.
Yes, we'll need to implement something using SAL here, good catch. I'll try to work on that today.
kraftbj
added
[Status] Needs Author Reply
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: May 7, 2019. |
|
Just wanted to comment here to illustrate the point. @jeherve and I have discussed putting this PR on hold because for now we either have to put a temporary fix in, or have a complicated refactor that will touch both sides. So waiting until we figure out the DNA could actually help us understand the direction that we need to take this code in. |
jeherve
removed
the
[Status] Needs Author Reply
|
This PR has been marked as stale. This happened because:
No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation. |
|
Too old. Will re-open if necessary |
dereksmart
deleted the
sync/class.wpcom-json-api-site-settings-endpoint.phpp
branch
Add new wrapper function on WordPress.com to match Jetpack-only funtion
Summary:
When syncing Jetpack files with WordPress.com, we sometimes run into issues
because Jetpack and WordPress.com have 2 different functions to check
if a module is active:
is_active_moduleon WordPress.com,is_module_activein Jetpack.This diff fixes that by adding a new
is_module_activefunction on WordPress.com.Test Plan:
Once this is merged, we should be able to sync PRs like this one with no issues.
#10120
Differential Revision: https://[private link]
Merges r190254-wpcom.
Testing instructions:
Can you modify your Jetpack settings from Calypso without errors? Especially search/Related Posts.
Proposed changelog entry for your changes:
*none