WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29885 closed feature request (fixed)

Add author__not_in for comment queries

Reported by: chriscct7 Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:

Description

It would be useful to have the author__not_in parameter that we have in WP_Query on comment queries, that way you can easily (for example) query all comments made on a particular post not written by a specific user.

Attachments (6)

29885.diff (1.1 KB) - added by nofearinc 5 years ago.
Add authorin and authornot_in for WP_Comment_Query
29885.2.diff (1.6 KB) - added by nofearinc 5 years ago.
rename post_author_ fields
29885-tests.diff (2.1 KB) - added by nofearinc 5 years ago.
phpunit tests
29885.3.patch (2.1 KB) - added by chriscct7 5 years ago.
First attempt at patch that implements all 4 parameters
29885-tests.2.diff (3.9 KB) - added by nofearinc 5 years ago.
post_author_ and author_ unit tests
29885-tests.3.diff (3.9 KB) - added by nofearinc 5 years ago.
post_author_ and author_ unit tests

Download all attachments as: .zip

Change History (32)

#1 @chriscct7
5 years ago

  • Keywords dev-feedback added

@nofearinc
5 years ago

Add authorin and authornot_in for WP_Comment_Query

#2 @nofearinc
5 years ago

Sample implementation added for author__in and author__not_in

#3 @boonebgorges
5 years ago

  • Keywords needs-refresh needs-unit-tests added; dev-feedback removed

author__in is ambiguous between comment author and post author. I think post_author__in is clearer.

Can we get some unit tests for these please?

#4 @nofearinc
5 years ago

Tests are ready, just need to override the name of the field.

@nofearinc
5 years ago

rename post_author_ fields

@nofearinc
5 years ago

phpunit tests

This ticket was mentioned in IRC in #wordpress-dev by nofearinc. View the logs.


5 years ago

#6 @nofearinc
5 years ago

  • Keywords has-patch added; needs-refresh needs-unit-tests removed

#7 @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Owner set to boonebgorges
  • Status changed from new to accepted

#8 @chriscct7
5 years ago

  • Keywords needs-patch added; has-patch removed

While I definitely see the usefulness of the ability to limit the comments returned by the author of the post, the ticket (which I apologize since it wasn't clear enough), was to be able to limit the comments returned by the author of the comments. For example, a query might be return all comments of post 14 except for ones written by user id 4.

While the patch for this issue adds post_author__in, I think the more helpful comment_author__in should also be added (since it was what I was actually proposing). I extremely apologize for any confusion.

#9 follow-up: @nofearinc
5 years ago

author_email allows for filtering by email in the comments query, and there is user_id support as well - would that work for you?

The only problem I can see is the lack of user_id__not_in which would also break the convention with user_id.

#10 @chriscct7
5 years ago

[ignore this comment...Trac went crazy]

Last edited 5 years ago by chriscct7 (previous) (diff)

#11 in reply to: ↑ 9 @chriscct7
5 years ago

Replying to nofearinc:

author_email allows for filtering by email in the comments query, and there is user_id support as well - would that work for you?

The only problem I can see is the lack of user_id__not_in which would also break the convention with user_id.

The problem with author_email is I want to do exactly opposite of what it does. For example, your patch adds post_author__not_in which, if passed a single int id, does the exact opposite of using post_author. I'm looking for the ability to get comments that are not written by a specific person, so we'd probably need a comment_author__in and comment_author__not_in.

Does that make sense?

Last edited 5 years ago by chriscct7 (previous) (diff)

#12 @nofearinc
5 years ago

  • Keywords dev-feedback added

I agree with your concern and I see where it could be applied. The only problem I see is still convention as we currently have author_email and user_id and what would be the proper naming for the opposite filters (we can't rename those due to backwards compatibility).

#13 @chriscct7
5 years ago

Its highly unlikely someone would be using both one of those and not_in. What we could do to get around this is if the parameter comment_author__in isset, then we use that parameter and ignore the email and the id one. If its not then we use what we already have. This would let a person using Comments Query to write a query where if its on WordPress 4.1, it would use the more powerful single param, else if its on the old version fallback (complete backwards compatibility) to the old params.

#14 @boonebgorges
5 years ago

  • Keywords dev-feedback removed

chriscct7 - Thanks for chiming in. I think the post_author__ arguments are still useful, so let's not discard them.

Re parameter names: We should aim to parallel WP_Query as much as possible, to make things easier for developers. So that suggests author__in and author__not_in. As noted, we have to keep user_id around.

Re internal logic: let's parse the existing user_id together with author__in and use the parsed value to build the IN clause.

What we could do to get around this is if the parameter comment_authorin isset, then we use that parameter and ignore the email and the id one.

No, I don't think we can do this. Currently you can pass user_id=5 and author_email=boone@example.com and you'll get back comments from *both* user 5 and boone@…. We can't have one of these params cancel the other one out. I do think we can parse together user_id and author__in in PHP, as noted above, but even this is optional - MySQL will figure out what to do with AND user_id IN (3,4,5) AND user_id IN (3,4).

#15 @chriscct7
5 years ago

Hi Boone,
I by no any means intended to suggest to discard the existing patch. I'm actually a really big fan of what it does. I just wanted to point out that while its really useful it wasn't the intended goal of the ticket, and suggested we instead append to it so we get both the intended functionality of the ticket and the functionality in the patch. I don't really care as to how its implemented, but somehow, we need to add the ability to be able to get all comments that aren't written by a specific author or array of authors.

While the goal is to parallel WP_Query, it should be noted that the author parameters always correspond to the author of the given data, in the normal case a post. On one hand while I get how author__not_in referring to the comment author, might be confusing, at the end of the day the author parameters always point to the data, not what its displayed on. Its the way that most developers will end up expecting comment querying to work, and actually the whole reason I opened this ticket, was because I mistakenly assumed author__not_in already was an existing parameter for comment queries and had pushed code assuming it was. While my mistake alone shouldn't be contrived to be a barometer of sorts for the developer community at whole by a long shot, I think its safe to assume when someone sees the author parameter in WP_Query and interprets it to be the author of the post, they'll see the same parameter for comment querying as the author of the comment. It could also be implemented as proposed above using a comment_author__not_in name.

What is your thoughts?

Last edited 5 years ago by chriscct7 (previous) (diff)

#16 @boonebgorges
5 years ago

What is your thoughts?

Yup, I totally agree. Sorry if that didn't come across in the above. I'm suggesting four new params:

'author__in', // comment author
'author__not_in', // comment author
'post_author__in', // post author
'post_author__not_in', // post author

#17 @chriscct7
5 years ago

Awesome, fantastic news!

Quick Trac question, when a patch exists for a ticket, and it adds functionality, but the patch needs to be updated to add more functionality (like in this case) is that needs-patch or refresh-patch?

Unless @nofearinc is already working on it, I'd like to take a stab at writing the update for the patch, and then ideally someone could review it. Would that be okay?

#18 @nofearinc
5 years ago

Happy to co-work on that, so I'll wait for the update if you'd like to tackle it yourself.

#19 @boonebgorges
5 years ago

Quick Trac question, when a patch exists for a ticket, and it adds functionality, but the patch needs to be updated to add more functionality (like in this case) is that needs-patch or refresh-patch?

needs-patch is correct. needs-refresh is for patches that no longer apply cleanly (I actually used it incorrectly above - sorry about that). See http://codex.wordpress.org/Reporting_Bugs#Trac_Keywords

Unless @nofearinc is already working on it, I'd like to take a stab at writing the update for the patch, and then ideally someone could review it. Would that be okay?

Sounds great. Thanks!

@chriscct7
5 years ago

First attempt at patch that implements all 4 parameters

#20 @chriscct7
5 years ago

  • Keywords has-patch needs-unit-tests needs-testing added; needs-patch removed

Okay, uploaded my patch for it, which someone should look over. Marking as needs unit tests, as if the new patch is accepted, the 2 parameters added in it will need to have tests written for it to be added to the existing unit test patch file. With the new patch, all 4 new parameters listed in comment 16 are implemented

Last edited 5 years ago by chriscct7 (previous) (diff)

#21 @boonebgorges
5 years ago

chriscct7 - Patch looks good to me, though we do need unit tests to move forward. If you've never written unit tests, this is a good time to learn :) https://make.wordpress.org/core/handbook/automated-testing/

#22 @nofearinc
5 years ago

+1 for the patch, and I'll "clone" the other unit test attached here for starters. Chris, let me know if you need help (or want me to submit the tests for your case as well).

Version 0, edited 5 years ago by nofearinc (next)

#23 @chriscct7
5 years ago

  • Keywords needs-testing removed

Will have an updated test case patch file later today. Didn't want to write the unit tests until the implementation was accepted, so that I knew what I was testing :)

#24 @chriscct7
5 years ago

My sincere apologies. I got so offtrack with other WordPress trac tickets I forgot about this one. Will update with updated testing patch file tomorrow

@nofearinc
5 years ago

post_author_ and author_ unit tests

@nofearinc
5 years ago

post_author_ and author_ unit tests

#25 @nofearinc
5 years ago

  • Keywords needs-unit-tests removed

Doing some gardening, submitting the unit tests for the author__in and author__not_in arguments.

#26 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 29935:

Comment/post author in/not_in for WP_Comment_Query.

Props nofearinc, chriscct7.
Fixes #29885.

Note: See TracTickets for help on using tickets.