Conversation
…haring Sharing_Service::set_global_options() rebuilds sharing-options['global'] from defaults, so any option missing from the payload is reset. Neither the PHP screen nor the legacy sharing-options handler posts open_links, so saving Settings > Sharing reset a "new window" choice made in Calypso or through the API back to "same". Sharing_Options::update() now keeps the stored values of every global option the payload leaves out, including placement, and both handlers save through it. The other saves move out of Post_Handler and into writers that take values instead of reading $_POST (Likes_Options setters, Placement_Section::update(), Twitter_Site_Tag::update(), Sharing_Resources::update(), Feature_Actions). The REST endpoints that follow reuse them, so the two screens save through the same code.
The React version of the screen needs one API that works on Simple, Atomic and self-hosted sites. Today the settings are split across jetpack/v4 (Jetpack sites only) and the legacy v1.x site settings and sharing-buttons endpoints (Simple). The routes live under wpcom/v2/sharing-likes/: settings, status, services, custom services, and each feature's switch-to-block and activate actions. The namespace is wpcom/v2 even though the data is the site's own. On Simple, wp-admin apiFetch calls go through public-api, which only serves a fixed list of namespaces, so jetpack/v4 or a package namespace would 404 there. The routes expose exactly what the PHP screen shows. A setting whose section is not rendered is left out of reads and refused on writes. An action is only accepted from the section state that offers it. Without that, the API would give back a way to turn the legacy buttons on where the block call-to-action deliberately removes it. The Jetpack plugin registers the routes outside is_admin() in load-jetpack.php, since REST requests are not admin requests. On Simple, sharing.php registers them, because that file loads on public-api through post-flair.php.
Resolve Post_Handler against trunk's Comment Likes section (#52796): keep trunk's per-platform Comment Likes save (the option on Simple, the comment-likes module elsewhere) and its removal from the Likes save, routing the Simple write and the reblogs write through the Likes_Options setters. Restore the Modules import the branch had dropped, which the module toggle needs again.
The endpoints offered comment_likes_enabled on Simple only, and saved it to the jetpack_comment_likes_enabled option. Since #52796, Settings > Sharing shows Comment Likes on every site that can run Likes. On Jetpack and Atomic the switch is the comment-likes module, which never reads that option. The React screen could not have matched the PHP one. - Offer comment_likes_enabled wherever Environment::likes_supported() holds, as the Comment Likes section does. Read it through Environment::comment_likes_enabled() and save it through Comment_Likes_Section::update(), which Post_Handler now uses too. - Switch Comment Likes before any other write. If a host keeps the module the other way, return 409 and leave everything else unsaved, where the PHP screen shows its "could not be switched" notice. - Offer likes_enabled and show where Comment Likes still read them with the Like buttons off, following Environment::comment_likes_follow_likes_settings(), as the PHP screen does. Report that flag, and Comment Likes support, in the status route. Its placement flag now counts Comment Likes too. Saving Comment Likes no longer returns a section state, as first planned: since CM-955, Comment Likes no longer affect the Like buttons section's state. What they do change is reported by the status route.
…them
The switch-to-block and activate routes could act where the screen
shows no button, or act on the wrong feature:
- The `feature` value was read with `get_param()`. Route matching
ignores case, and a body or query `feature` outranks the URL, so on
Simple `POST /SHARING/switch-to-block`, or a `{"feature":"bogus"}`
body, passed the Sharing gate and then ran the Likes switch, turning
off Likes and Reblogs. Non-string values threw a TypeError (500).
Read it from the URL, and validate it against the two features.
- On a site that is neither connected nor in offline mode, the Sharing
section shows no "Turn on" button, because `Modules::activate()`
refuses every module there. The route still accepted the request,
then answered 500. Refuse it with 409, as for Likes.
- `Modules::activate()` saves a module as active even when a host
forces it off, so the route answered 200 while nothing changed.
Report whether the module is active afterwards, and answer 409
rather than 500 when it is not: that is host policy, not a
server error.
…outes A partial save rewrote values it was not sent, and reads returned text in a form the screen never shows: - `Sharing_Options::update()` carried the stored label forward unslashed, and `set_global_options()` unslashes it, so saving only the button style turned `Share \o/` into `Share o/`. Slash it. - `set_global_options()` stores the default label as `false`, so it follows the site language, but only when the incoming label matches the translation exactly, before unslashing. Where the translation holds a quote, the slashed label never matched, and a save froze it in the current language. The form path has always done this; the slashing above made every partial REST save do it too. Pass the default through unslashed when it matches. - The label and custom service names are stored through `wp_kses()`, which encodes a bare `&`. The screen prints them decoded, the routes returned `&`. Decode them in responses. - `open_links` has no field on the screen, and the routes offer only what the screen shows. Drop it from the schema; saves keep the stored value. - The custom service `id` was read with `get_param()`, so a body `id` outranked the URL, and a non-string one threw a TypeError. Read it from the URL.
The Jetpack plugin wired the package up from three places: Settings_Page and Post_Handler inside the is_admin() block of load-jetpack.php, and Endpoints outside it, because REST requests are not admin requests. modules/sharedaddy/sharing.php repeated the same split for WordPress.com Simple, once from sharing_admin_init() and once from a rest_api_init callback. Each caller had to know which parts belong to wp-admin, and adding anything to the package meant finding every caller again. Initializer::init() now owns that split: the REST routes on every request, the screen and its form handler in wp-admin. It runs once, like the SEO and VideoPress initializers. load-jetpack.php calls it once, and sharing_admin_init() calls it on Simple, which replaces sharing_register_rest_routes(). On Simple, sharing_admin_init() runs on init, and public-api only starts the REST server after wp-load.php, so the routes still register in time. PACKAGE_VERSION moves to the Initializer, as it lives on SEO's. The scaffolded Sharing_Likes class held nothing else and nothing referenced it.
Bring in the per-post Likes and Sharing switch classes (#52950) and the 16.3-a.7 backport, which bumped Sharing_Likes::PACKAGE_VERSION to 0.1.1. The branch replaced that scaffold class with Initializer, so keep it deleted and carry the bump to Initializer::PACKAGE_VERSION. The README now says the switch classes are set up apart from Initializer::init() for now, since the plugin modules still register the same fields. AGENTS.md points the follow-up that moves the modules to the Initializer as the place to call them.
Bring in #53039, which has the Likes, Comment Likes and Sharing modules register the per-post switches through Post_Likes_Switch and Post_Sharing_Switch. - sharing.php imports: keep Initializer and trunk's Post_Sharing_Switch. Drop Post_Handler, which the Initializer now starts. - AGENTS.md: take trunk's account of who calls each switch's init(). It replaces this branch's note that the Initializer should call them, which #53039 settled the other way: each field lives with the feature that reads it. - README: say the same, rather than that the switches are left out of the Initializer only for now.
The changelog entry said PACKAGE_VERSION moved, but not that the class holding it is gone. Nothing in the monorepo or on WordPress.com referenced it, but package consumers read the changelog to find out.
jeherve
added
Enhancement
github-actions
Bot
added
[Feature] Sharing
Contributor
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, 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. |
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 9 files. Only the first 5 are listed here.
8 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
Sharing_Admin_Simple_Bridge_Test failed on every PHP job once sharing_admin_init() moved to Initializer::init(). The test bootstrap runs load-jetpack.php, which already called init() once, so the run-once guard made the Simple call a no-op. The test also never made is_admin() true, which the Initializer now checks before hooking up the screen. A real wp-admin request on Simple meets both conditions, since load-jetpack.php never runs there. Reset the guard and set a wp-admin screen for each case, and put both back afterwards. The "elsewhere" case now asserts that nothing is registered, rather than that nothing changed. Before, the guard alone kept it passing even without the Simple check.
tools/project-version.sh matches `const PACKAGE_VERSION = '…';` and nothing else, so the `public` keyword on Initializer::PACKAGE_VERSION hid the constant, and the Changelogger validity check failed with "Did not find version constant". Drop the keyword, as SEO's Initializer does, and note why next to it.
PHPCompatibility's dev branch flags a reserved keyword used as a class constant name, which is deprecated in PHP 8.6, and the "dev branch for PHP 8.0" check fails on it. Nothing outside the package read the constant.
"Whichever modules are active" left readers to guess which ones. Say Sharing, Likes and Comment Likes in load-jetpack.php, the Initializer docblock, and AGENTS.md. Also drop the note on PACKAGE_VERSION's missing visibility keyword: the change it explained is made, and the version tooling flags it if it comes back.
load-jetpack.php calls Initializer::init(), a class new on this branch. Another plugin's autoloader can serve a jetpack-sharing-likes release from before it (0.1.x is on Packagist), and the unguarded call would then fatal on every request. Check the class first, and otherwise register the screen and its form handler in wp-admin as trunk does. The REST routes need the newer package anyway, so the fallback goes without them.
The package is released, so dropping its public Sharing_Likes class can break a consumer that reads Sharing_Likes::PACKAGE_VERSION. The entry folded that into an "Add Initializer" line typed as changed, where nobody scanning for removals would see it. Give the removal an entry of its own, typed removed, and type the Initializer entry as added.
Review turned up text that no longer described the code: - Two docblocks pointed at Post_Handler::activate_module(), which is now activate_feature(), with its rationale in Feature_Actions::activate(). - The README said the screen exists whatever the site is running. It has been limited to Simple, connected and offline-mode sites since the screen stopped registering where its features cannot load. - AGENTS.md said the Simple bridge in sharing.php can go once wpcom registers the screen. It now also registers the REST routes there, so removing it at that point would 404 every route on Simple. - AGENTS.md did not say that a setting which only appears once Comment Likes switch needs a second save, because availability is decided before anything is written. Also cut five docblocks back to the repo's comment budget, and replace a test docblock that repeated the namespace rationale Endpoints owns with a pointer to it.
A POST to settings with `{"likes_enabled": null}` turned Likes off, and
a null Comment Likes, site tag, placement or label likewise saved as
off or empty. A client that serializes an unset field as null would
wipe settings the user never touched.
get_endpoint_args_for_item_schema() gives each argument an explicit
sanitize callback. WP_REST_Request skips the validate callback for a
null value but still sanitizes it, and sanitizing null as a boolean or
string gives false or ''. update_item() then saw a sent value.
Use rest_parse_request_arg as the sanitize callback, which validates
first, so null is refused with a 400 as the services routes already
do through core's default.
…t exist Settings > Sharing is not registered on a self-hosted site that is neither connected nor in offline mode, but its REST routes still answered there. `settings` offered the site tag and saved it, and `status` reported sharing as off, the variant that offers "Turn on", while `sharing/activate` then refused. That breaks the rule that the routes offer what the screen shows and nothing else, and a React screen built on `status` would show a button that always fails. Check Environment::settings_screen_supported() in the permission callback every route shares, after the capability check. It runs per request, since on Simple the routes register before public-api switches to the requested site. Registration stays unconditional for the same reason. That leaves the sharing half of refuse_unless()'s connection check unreachable, so it now only covers Likes, which offline mode keeps off while the screen stays. The tests that described the disconnected cases move to offline mode, and Endpoints_Test now sends every route and method as an editor and on a site without the screen.
The Sharing_Service and Share_Custom stubs differed from the real classes in ways the tests leaned on: - get_global_options() filled in button_style and open_links defaults over a stored global, which the real one does not, so the fallbacks in Sharing_Options::get() were never exercised. - new_service() and update_options() skipped wp_kses() and the 30-character limit, so the decoded-name test had to hand in a name already encoded. - set_blog_services() neither filtered against the available services nor fired sharing_get_services_state, so the Sharing_Service branch of Feature_Actions::remove_all_sharing_services(), which exists for wpcom's hook on that action, could go without anything noticing. - A stored "Share this:" label was not mapped to the translated one. "Saving writes nothing it was not sent" also only passed because the stub never stores defaults on first read, as the real class does; it now starts from a stored global and checks that it is left alone. Add the tests no guard had: writes refused while sharing is off, unknown and duplicated services dropped, the deprecated flag, Reblogs and the site tag offered only where the screen shows them, Simple's switch to the Sharing block going through Sharing_Service, and the open_links fix through the services form save. Drop test docblocks that repeated the source's own.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CM-939
Proposed changes
This adds REST endpoints to the Sharing & Likes package, under
wpcom/v2/sharing-likes/, so the upcoming React version of Settings > Sharing can read and save every setting with one client path, on WordPress.com Simple, Atomic, and self-hosted Jetpack sites:settings(GET, POST): Likes, Reblogs (Simple), Comment Likes, button style, sharing label, placement, the Twitter Site Tag, and "Disable CSS and JS". A POST only writes the keys it sends.status(GET): which variant each section renders, whether Likes and Comment Likes can run, whether placement shows, and the Site Editor link.sharing|likes/switch-to-blockandsharing|likes/activate(POST): the "Switch to the … block" and "Turn on" actions. Both return the new status.services(GET, POST),services/custom(POST),services/custom/<id>(POST, DELETE): enabled services and their order, and custom services.It also fixes a long-standing bug: saving Settings > Sharing reset "Open links in" to "same window" whenever that choice had been made elsewhere (Calypso, the API), because the screen never posts that field and
Sharing_Service::set_global_options()rebuilds its options from defaults.Why
wpcom/v2The data is each site's own, so
wpcom/v2isn't the obvious namespace. I consideredjetpack/v4, a namespace of our own, and core's settings endpoint, butwpcom/v2is the only one that works from wp-admin everywhere without a WordPress.com change. On Simple, wp-adminapiFetchcalls go through public-api, which only serves a fixed list of namespaces (wp/v2,wpcom/v2,wpcom/v3, and a few more):jetpack/v4or a namespace of our own registers fine but 404s there. That's why Newsletter has itsjetpack/v4/v1.4split today. On Jetpack and Atomic,wpcom/v2is served same-origin. Podcast, VideoPress, and PayPal settings use it the same way.Core's
/wp/v2/settingsalso reaches Simple, but only suits flat options: it doesn't fitsharing-options, the services list, or the actions.How it fits with the PHP screen
BLOCK_CALL_TO_ACTIONcloses on purpose.Post_Handlersave through the same writers (Sharing_Options::update(),Placement_Section::update(), theLikes_Optionssetters,Comment_Likes_Section::update(), and so on), so the two screens can't drift while both exist.open_linksisn't exposed, although CM-939 listed it: the screen has no field for it. Saves keep the stored value.One initializer
Initializer::init()now sets the package up: the REST routes on every request, the screen and its form handler in wp-admin.load-jetpack.phpcalls it once, andsharing.phpcalls it on Simple, whereload-jetpack.phpdoesn't run. It replaces the three separate calls, and theSharing_Likesclass, which only heldPACKAGE_VERSION.Usage
On Simple, the wp-admin
apiFetchmiddleware adds/sites/<id>/and routes the call through public-api, so the same path works there.Related product discussion/links
sharing.phpalready loads on public-api throughpost-flair.php.Does this pull request change what data or activity we track or use?
No.
Testing instructions
On a connected Jetpack site running this branch, with the Sharing and Likes modules on and a classic theme:
The
open_linksfixwp eval '$o = get_option( "sharing-options" ); $o["global"]["open_links"] = "new"; update_option( "sharing-options", $o );'wp eval 'var_dump( get_option( "sharing-options" )["global"]["open_links"] );'should still saynew. On trunk it sayssame.The endpoints, from the browser console on any wp-admin page:
await wp.apiFetch( { path: '/wpcom/v2/sharing-likes/settings' } )returns the settings Settings > Sharing shows.await wp.apiFetch( { path: '/wpcom/v2/sharing-likes/settings', method: 'POST', data: { sharing_label: 'Pass it on:' } } ), then reload Settings > Sharing: the label changed, and the button style and placement didn't.await wp.apiFetch( { path: '/wpcom/v2/sharing-likes/status' } )reportsconfigurefor both sections.await wp.apiFetch( { path: '/wpcom/v2/sharing-likes/likes/activate', method: 'POST' } ): Likes come back on. Switch to a block theme with asingletemplate and trysharing/switch-to-block: the Sharing module turns off, andsharing/activateis now refused with a 409.On a WordPress.com Simple sandbox, with this branch synced to both the
jetpack-pluginmirrors: repeat steps 1 to 3 from a Simple site's wp-admin. That's the path I'm least sure about, since it registers the routes fromsharing.phpand reaches them through public-api; the package tests can't cover it.