SIT-3134: fix/232: Updated data type of keywords field to array. #233
Conversation
| @@ -198,7 +198,11 @@ function get_post_data( $post ) { | |||
| $canonical_url = $yoast_canonical; | |||
| } | |||
|
|
|||
| $keywords = get_post_meta( $post->ID, '_yoast_wpseo_focuskw', true ); | |||
| $yoast_focuskw = get_post_meta( $post->ID, '_yoast_wpseo_focuskw', true ); | |||
| if ( ! empty( $yoast_focuskw ) ) { | |||
There was a problem hiding this comment.
if $yoast_focuskw is empty, then $keywords will be still be an empty string. Do we want to change the initial data type as well?
There was a problem hiding this comment.
Yes, if it's an empty string please have it as an empty array instead, or null
There was a problem hiding this comment.
We have array_filter on $data to remove all empty entries. So, in the case of empty $yoast_focuskw, the keywords field will be removed from $data and it will not be sent in request. As it is not going to send in a request, I think it's fine to keep it as it is. but it may create confusion for other devs too. So, I updated the initial value to [].
Thanks
| $yoast_focuskw = get_post_meta( $post->ID, '_yoast_wpseo_focuskw', true ); | ||
| if ( ! empty( $yoast_focuskw ) ) { | ||
| // Limit focus keyphrase to max length of 128. | ||
| $keywords = [ substr( $yoast_focuskw, 0, 128 ) ]; |
There was a problem hiding this comment.
Although we probably won't be sending anything over 100 strings as the array content, do we want to go ahead and implement that as well? Per requirement:
steps to fix: allow an array of strings with min 0 and max 100 strings.
There was a problem hiding this comment.
@oscarssanchez $yoast_focuskw will be a single string. So, $keywords will be an array with 1 length always. However, the max length of the string can be up to 128, which I already implemented. Please let me know If I missed or misunderstood anything here.
Thanks.
There was a problem hiding this comment.
Sonds good to me.
How will this be populated if they are not using yoast?
There was a problem hiding this comment.
@YMufleh it will be only populated if Yoast is installed. If they are not using Yoast, keywords field will be not sent in the request. (note: keywords field is optional)
There was a problem hiding this comment.
Noting here that WordPress, by default, does not provide keywords functionality and requires a plugin, like Yoast, to add on this SEO feature. There are some other SEO plugins, but by default the plugin only supports Yoast. If there are others that are determined to be in-use by other partners, then support for those SEO plugins can be added.
There was a problem hiding this comment.
@iamdharmesh , yes you are correct at this point we should not have more than 1 string, I was thinking more about future proofing, so something like if (count($array) > 100) limit array , but seems per @YMufleh we can leave this for now
There was a problem hiding this comment.
There was a problem hiding this comment.
I believe tags are sent across in a different field/variable, right? Just like categories?
There was a problem hiding this comment.
See categories: https://github.com/globeandmail/sophi-for-wordpress/blob/47fbb181999890297e3cad5594832c0443d5dd14/includes/functions/content-sync.php#L220
and tags: https://github.com/globeandmail/sophi-for-wordpress/blob/47fbb181999890297e3cad5594832c0443d5dd14/includes/functions/content-sync.php#L222
...right?
|
Kudos, SonarCloud Quality Gate passed!
|
| $yoast_focuskw = get_post_meta( $post->ID, '_yoast_wpseo_focuskw', true ); | ||
| if ( ! empty( $yoast_focuskw ) ) { | ||
| // Limit focus keyphrase to max length of 128. | ||
| $keywords = [ substr( $yoast_focuskw, 0, 128 ) ]; |
There was a problem hiding this comment.
Sonds good to me.
How will this be populated if they are not using yoast?
|
Given both @YMufleh and @oscarssanchez approved, I'm going to merge in so we can get this to our test site for further validation before a plugin minor release. |
Description of the Change
As requested on #232, This PR updates the data type of the
keywordsfield to array from string.Closes #232
Alternate Designs
Possible Drawbacks
Verification Process
Verify that plugin sends an array of strings in the
keywordsfield, further verification can be done on Sophi side logs.Checklist:
Changelog Entry
Changed - The data type of keywords field to an array from string.
Credits
Props @iamdharmesh @jeffpaul