Opened 10 years ago
Closed 8 years ago
#28603 closed enhancement (fixed)
Improve "previously approved comment" test for logged-in users
Reported by: | nevermoor | Owned by: | rachelbaker |
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 4.0 |
Component: | Comments | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
In order to restrict spam, my site uses the following settings to restrict the ability to comment (from /wp-admin/options-discussion.php):
1: Other comment settings / Users must be registered and logged in to comment
2: Before a comment appears / Comment author must have a previously approved comment
This generally works as expected, however the second requirement appears to test the user's profile information rather than user ids. This means that whenever a user changes either their nickname or e-mail, their next comment is held for moderation.
In my view, a better implementation would allow comments from logged-in users with previously approved comments immediately after a change to whatever profile information is currently tested.
Apologies if this issue has been discussed before, I certainly couldn't find that discussion.
Attachments (6)
Change History (22)
#1
@
10 years ago
- Component changed from Administration to Comments
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#2
@
10 years ago
- Keywords has-patch added; needs-patch removed
If user_id is available it checks if the user has been approved, else it checks the author and email.
This ticket was mentioned in Slack in #core by voldemortensen. View the logs.
10 years ago
#4
@
10 years ago
Updated patch to 1) strict check on $user_id, 2) run queries through $wpdb->prepare, and 3) provide unit tests for when a $user_id is passed in.
This ticket was mentioned in Slack in #core by voldemortensen. View the logs.
10 years ago
#6
@
10 years ago
- Milestone changed from Future Release to 4.2
The minor change in 28603.3.diff expects $user_id to be an integer, and defaults to 0 (instead of false).
Everything looked good in my testing.
#8
@
10 years ago
Some notes on 28603.3.diff:
- Patch still applies, unit tests pass
- Adding
$user_id
here would constitute an 8th (!) parameter forcheck_comment()
. Yikes. - If we do end up adding the
$user_id
parameter, we also need to add a changelog entry to the DocBlock mentioning that the new parameter was added e.g. "@since 4.2.0 The$user_id
parameter was introduced."
#10
@
10 years ago
@DrewAPicture I agree that adding an eighth parameter is not ideal, do you have a better suggestion on how to approach?
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
#12
@
10 years ago
- Milestone changed from 4.2 to Future Release
We can do a get_user_by( 'email', $email ) instead of passing in the 8th param.
Also, if the goal is to make it so that users who change there email address have approved comments, we should make sure to add a test for that scenario.
Overall, I think this is close, but not quite there for 4.2. Let's tighten it up for 4.3
#13
@
8 years ago
- Keywords needs-unit-tests added; dev-feedback removed
- Milestone changed from Future Release to 4.7
In 28603.5.diff I took @jorbin's advice and used a get_user_by()
conditional to avoid adding the evil 8th parameter to check_comment()
. I added one unit test, more would be nice.
#14
@
8 years ago
- Keywords has-unit-tests commit added; needs-unit-tests removed
28603.6.diff adds an additional unit test to 28603.5.diff and this looks ready for commit. @jorbin care to sanity check the logic here?
Hi nevermoor, thanks for the report. This does indeed seem like a good change.