Ignore flood and duplicate check for notes #10448
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
adamsilverstein
force-pushed
the
fix/skip-flooding-check-for-notes
branch
from
6e64cf1 to
0c25783
Compare
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
src/wp-includes/comment.php
Outdated
| ); | ||
| // Notes require logged in users that can edit the current post, ignore flooding check. | ||
| if ( isset( $commentdata['comment_type'] ) && 'note' === $commentdata['comment_type'] ) { | ||
| if ( ! isset( $commentdata['comment_post_ID'] ) || ! current_user_can( 'edit_post', $commentdata['comment_post_ID'] ) ) { |
There was a problem hiding this comment.
Since there are no other capability checks in this function, this feels a bit out of place here.
Unless I am missing something, I believe that this should be handled by create_item_permissions_check()? If so, a 'note' !== $commentdata['comment_type'] check maybe could be wrapped around the flooding logic.
There was a problem hiding this comment.
Ok I'll try moving the logic there.
There was a problem hiding this comment.
To clarify, I am OK with skipping the flood protection here. I am uncertain whether there needs to be a capability check here at all.
There was a problem hiding this comment.
Moving to create_item_permissions_check is a good idea, we can skip calling wp_allow_comment entirely for notes. this also let me remove a redundant bit of code in wp_allow_comment that enabled duplicates for notes.
I updated the PR
adamsilverstein
changed the title
adamsilverstein
force-pushed
the
fix/skip-flooding-check-for-notes
branch
from
8fba138 to
77bdf0d
Compare
|
@desrosj I updated the PR with an alternate approach, let me know what you think. I also created a matching Trac issue since we'll need to commit the fix there directly. |
github-project-automation
bot
moved this to 🔎 Needs Review
in WordPress 6.9 Editor Tasks
t-hamano
left a comment
t-hamano
left a comment
There was a problem hiding this comment.
I'm not sure which approach is best, but this PR only allows flood and duplicate checks via the REST API, right? Currently, notes should only be inserted via the REST API, but I'm not sure whether we should add exclusion logic to wp_allow_comment() itself or not.
|
I tried that originally (see first commits) before moving to the controller - which is the only place core calls that function. It's a bit cleaner this way. |
adamsilverstein
requested review from
Mamaduka,
TimothyBJacobs and
kadamwhite
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
Collocating logic into a REST controller makes sense to me.
I've also confirmed that wp_allow_comment always returns 1 for users with admin caps.
| $prepared_comment['comment_approved'] = | ||
| 'note' === $prepared_comment['comment_type'] ? | ||
| '1' : | ||
| wp_allow_comment( $prepared_comment, true ); |
There was a problem hiding this comment.
wp_allow_comment() invokes is wp_check_comment_data() which "checks whether comment data passes internal checks or has disallowed content." This is mainly around looking for disallowed words and characters, checking for the presence of too many links in the comment's content, and whether the author has previously approved comments.
I think skipping all of, including the pre_comment_approved filter this is fine. The filter can't really be used to change a Note's status anyway (because "resolved" status is logged in comment meta). But just wanted to mention this so this is being done intentionally in case someone finds this PR in the future.
There was a problem hiding this comment.
wp_allow_comment() invokes is wp_check_comment_data() which "checks whether comment data passes internal checks or has disallowed content."
Right, skipping that is the intent here. Notes are from trusted internal users with edit_post, these checks are designed for external comments.
I think skipping all of, including the pre_comment_approved filter this is fine.
Thats a good point about skipping the filter, I hadn't thought of that. We can always apply it if there is a valid use.
I noticed spam plugins like Akismet hook into this filter, so we might not want to fire it for notes. If anything we could fire a new note specific filter instead.
Ps. the comment meta is only used to store resolution meta data - who resolved or reopened a thread and when. The 'comment_approved' field for notes indicates if the note is open or resolved (and thus mainly applies to the parent note of a thread). So filtering could still be useful.
|
Thanks all! I think this is looking good. As more ways to manage Notes are introduced, we can look at better ways to extract this into a better function/location. |
5 participants
Fixes WordPress/gutenberg#72892
Trac ticket: https://core.trac.wordpress.org/ticket/64199
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.