-
Notifications
You must be signed in to change notification settings - Fork 817
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
WPcom: adjust filter for comment counts used in Calypso #24512
Conversation
…pack_api_include_comment_types_count
Caution: This PR has changes that must be merged to WordPress.com |
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just have a couple thoughts. On the wpcom side, there may need some tests that need to be updated as well, but we can look at that when the time comes.
projects/plugins/jetpack/changelog/add-jetpack-api-include-comment-types-count
Outdated
Show resolved
Hide resolved
…ment-types-count Co-authored-by: Samiff <samiff@users.noreply.github.com>
…o apply_filters_deprecated
Thanks, Sami! I've gone ahead and committed the suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few tiny suggestions 👍
Co-authored-by: Samiff <samiff@users.noreply.github.com>
Co-authored-by: Samiff <samiff@users.noreply.github.com>
Co-authored-by: Samiff <samiff@users.noreply.github.com>
5ab6ab2
to
33f69e1
Compare
I'm a bit confused on the failing TC tests for the wpcom diff. We are touching the |
* | ||
* @param array Array of comment types to exclude (default: 'order_note', 'webhook_delivery', 'review', 'action_log') | ||
*/ | ||
$exclude = apply_filters_deprecated( 'jetpack_api_exclude_comment_types_count', array( 'order_note', 'webhook_delivery', 'review', 'action_log' ), 'jetpack-11.0', 'jetpack_api_include_comment_types_count' ); // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$exclude = apply_filters_deprecated( 'jetpack_api_exclude_comment_types_count', array( 'order_note', 'webhook_delivery', 'review', 'action_log' ), 'jetpack-11.0', 'jetpack_api_include_comment_types_count' ); // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable | |
$exclude = apply_filters_deprecated( 'jetpack_api_exclude_comment_types_count', array( 'order_note', 'webhook_delivery', 'review', 'action_log' ), 'jetpack-$$next-version$$', 'jetpack_api_include_comment_types_count' ); // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anomiex Should the replace-next-version-tag
script be catching this already, or would it need to be adjusted?
[plugins/jetpack 4:17.704] === Updating $$next-version$$ ===
[plugins/jetpack 4:17.704]
[plugins/jetpack 4:19.232] Processing projects/plugins/jetpack/class.json-api.php
[plugins/jetpack 4:19.243] ::error file=projects/plugins/jetpack/class.json-api.php,line=1032::Unexpected `$$next-version$$` token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I expected the script to catch this indeed, my bad.
52f4c59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should, but currently doesn't. It only handles _deprecated_function
, _deprecated_constructor
, _deprecated_file
, _deprecated_argument
, and _deprecated_hook
.
Handling apply_filters_deprecated
may not be entirely straightforward, since it seems likely enough that phpcs might wind up wrapping that onto multiple lines. Maybe it's time to just replace $$next-version$$
everywhere and hope that never manages to collide with anything unexpected.
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
…lude-comment-types-count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well for me now, I think it should be good to go. 🚢
Great news! One last step: head over to your WordPress.com diff, D81537-code, and deploy it. Thank you! |
r246959-wpcom |
* Deprecates jetpack_api_include_comment_types_count and introduces jetpack_api_include_comment_types_count * changelog * Update projects/plugins/jetpack/changelog/add-jetpack-api-include-comment-types-count Co-authored-by: Samiff <samiff@users.noreply.github.com> * [not verified] Added doc blocks and switched existing apply_filters to apply_filters_deprecated * Update projects/plugins/jetpack/class.json-api.php Co-authored-by: Samiff <samiff@users.noreply.github.com> * Update projects/plugins/jetpack/class.json-api.php Co-authored-by: Samiff <samiff@users.noreply.github.com> * Update projects/plugins/jetpack/class.json-api.php Co-authored-by: Samiff <samiff@users.noreply.github.com> * Re-added 'since' tag to deprecated filter doc block * Update projects/plugins/jetpack/class.json-api.php Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> * Update projects/plugins/jetpack/class.json-api.php Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> * Update to final version See Automattic/jetpack#24512 (comment) Co-authored-by: Daniel Rodriguez <daniel.r@a8c.com> Co-authored-by: Samiff <samiff@users.noreply.github.com> Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/2456039741
Currently certain comment types,
comment
,pingback
, andtrackback
are whitelisted for being displayed in the comments list. Comment counts, on the other hand are calculated by excluding the comment typesorder_note
,webhook_delivery
,review
,action_log
.This is done using the
jetpack_api_exclude_comment_types_count
filter, and does not take into account other comment types that may be used by third-party plugins. As a result, there can be a discrepancy between the comments listed, and the comment count that are displayed.After discussing this further with @samiff we decided that it may be best to deprecate
jetpack_api_exclude_comment_types_count
, and instead introducejetpack_api_include_comment_types_count
, which will bring parity to the comment types displayed, and the comment types counted, defaulting to includingcomment
,pingback
, andtrackback
comment types.Fixes #63872
Changes proposed in this Pull Request:
jetpack_api_exclude_comment_types_count
and introducejetpack_api_include_comment_types_count
WHERE comment_type IN
matching those specified in the arrayDoes this pull request change what data or activity we track or use?
No
Testing instructions:
wp_comments
table, and edit thecomment_type
to something other thancomment
,pingback
,trackback