Correctly validate on/off values for the boolean type in REST API

[htdat]

When working and testing #14239, I notice that it's not possible to really switch off Someone likes one of my posts from /wp-admin/options-discussion.php. This PR is to fix this issue.

Steps to replicate this issue

• Go to /wp-admin/options-discussion.php
• Disable Someone likes one of my posts option.
• Check https://wordpress.com/settings/discussion/site.com, Someone likes one of my posts is always enabled when it should NOT.
• Try to disable and enable a few times to notice the issue.

Debugging and changes proposed in this Pull Request:

With \Jetpack_Likes::admin_discussion_likes_settings_validate(), social_notifications_like can be only on or off if trying to update this option in /wp-admin/options-discussion.php

\Jetpack_Core_Json_Api_Endpoints::cast_value() is responsible to check and return boolean types. However, it does not check two strings on and off values at all.

That's why even with off, it will be return true with this assessment:

return (bool) $value;

This PR will help check on and off strings to return expected values (false / true) for REST API.

Proposed changelog entry for your changes:

• General: Correctly validate on/off values for the boolean type in REST API

[jetpackbot]

	Warnings
⚠️	"Testing instructions" are missing for this PR. Please add some

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 5f60fe1

[jeherve]

With \Jetpack_Likes::admin_discussion_likes_settings_validate(), social_notifications_like can be only on or off if trying to update this option in /wp-admin/options-discussion.php

Part of me wonders if it wouldn't be best to have that always return boolean values instead of on or off. This may be a good opportunity to update that.

@zinigor What's your take on this?

[kraftbj]

I agree with @jeherve that I'd rather not use on and off, using a true boolean instead.

Off the top of my head, we could probably filter on a pre_option filter to change on/off to true/false. If is_admin, then also change the setting to the proper bool. I'd want to only do that is_admin to prevent a site with a lot of front-end requests causing some sort of race condition or multiple attempts to save the option, etc.

[htdat]

I agree with @jeherve that I'd rather not use on and off, using a true boolean instead.

I also agree that it's better to use real boolean values. However, in my mind, there are at least two things to consider if we switch to real boolean values:

1. Backward compatibility for on and off values. If so, we may still need to apply this patch?
2. Double check WP.com codebase and verify that this option still functions with the real boolean values.

Off the top of my head, we could probably filter on a pre_option filter to change on/off to true/false.

I think we can update this method \Jetpack_Likes::admin_discussion_likes_settings_validate() directly:

https://github.com/Automattic/jetpack/blob/master/modules/likes.php#L237-L244

[kraftbj]

I think we can update this method
\Jetpack_Likes::admin_discussion_likes_settings_validate() directly:

I agree, plus the on references above it (~L220), at L106, etc.

Backward compatibility for on and off values. If so, we may still need to apply this patch?

Psuedocode, but I was thinking of something like...

add_filter( 'option_social_notifications_like', 'example' );
function example( $value ) {

if ( 'on' === $value ) {
$value = true;
if ( is_admin() ) {
update_option( 'social_notifications_like', true );
}
} else if ( 'off' === $value ) {
$value = false;
if ( is_admin() ) {
update_option( 'social_notification_like', false );
}
}
return $value;

or if it is just a sitewide-notification, we can update it when Jetpack is updated a la https://github.com/Automattic/jetpack/blob/master/class.jetpack.php#L433

I haven't reviewed the existing code well enough to have an opinion if true / false is better or worse than 1 / 0, but I think either of those is better than on/off. I may be missing things above too, but to give you a sense of what I'm thinking.

[kraftbj]

As a note, though, if this ends up being a rabbit hole, I am okay with accepting this PR after testing as is to restore expected functionality and opening a new issue to track the option migration in a future release.

[zinigor]

I agree with @kraftbj and @jeherve here about it being weird to use on and off for boolean values. I couldn't find any more examples of this anywhere in Jetpack except the other setting for "Email me when someone follows my site". That's a good catch, @htdat ! Let's merge this one and weed out all the cases afterwards, so that we can migrate to real booleans seamlessly.