Improve "previously approved comment" test for logged-in users

[nevermoor]

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.

[nacin]

Hi nevermoor, thanks for the report. This does indeed seem like a good change.

[voldemortensen]

If user_id is available it checks if the user has been approved, else it checks the author and email.

[voldemortensen]

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.

[rachelbaker]

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.

[DrewAPicture]

Some notes on 28603.3.diff​:

• Patch still applies, unit tests pass
• Adding $user_id here would constitute an 8th (!) parameter for check_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."

[voldemortensen]

Added the missing @since in the doc block.

[rachelbaker]

@DrewAPicture I agree that adding an eighth parameter is not ideal. Do you have a better suggestion on how to approach?

[jorbin]

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

[rachelbaker]

use get_user_by() instead of reaching parameter madness

[rachelbaker]

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.

[rachelbaker]

Same as 28603.5.diff with additional unit test

[rachelbaker]

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?

[rachelbaker]

In 38738:

Comments: Improve check for previous comments for authenticated users in check_comment().

When the 'comment_whitelist' option is enabled and the commenter is an authenticated user, query for the existence of an approved comment with a matching user_id. This allows authenticated users that have changed their email address to bypass having their comment held for moderation.

Props voldemortensen, rachelbaker.
Fixes #28603.