WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 19 months ago

Last modified 16 months ago

#21101 closed enhancement (fixed)

Allow get_comments() to query for explicit value of comment_approved

Reported by: nbachiyski Owned by: westi
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

If you have a comment with value of comment_approved different from the default ones (if you don't want it showing in the admin/frontend), currently you can't query for it directly.

The patch adds another query var, comment_approved which overrides the value of the status query var and searches for an explicit value of the column, instead of doing special cases and using 0/1 as default.

Attachments (5)

explicit-comment-approved-querying.diff (834 bytes) - added by nbachiyski 22 months ago.
21101.diff (1.1 KB) - added by dd32 20 months ago.
21101.2.diff (565 bytes) - added by SergeyBiryukov 19 months ago.
21101.3.diff (596 bytes) - added by ryan 19 months ago.
all|full
21101-ut.diff (1.6 KB) - added by ryan 19 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 batmoo22 months ago

  • Cc batmoo@… added

comment:2 westi20 months ago

  • Milestone changed from Awaiting Review to 3.5

This looks like a good enhancement for 3.5.

Would be nice to see some unit tests for the WP_Comment_Query functionality in general and it would be good to start with some for this new functionality - can test it against the built-in comment statuses.

comment:3 westi20 months ago

[UT979] adds some simple Unit Tests for this.

comment:4 westi20 months ago

In [21570]:

Comments: Allow the caller of get_comments() to request comments with a specific comment_approved value.

This allows for a custom comment status to be queried directly overriding the status argument.

See #21101 props nbachiyski.

dd3220 months ago

comment:5 follow-up: dd3220 months ago

I'm curious as to why the notion of introducing an extra query var in place of status is really needed in this case?

I don't see any discussion here as to why one was chosen over the other, and as more people start using custom comment types with improving API's, I think it's going to fast become confusing for some, especially for example, when the extra query variable offers no obvious benefit over something such as 21101.diff to the untrained eye.

comment:6 in reply to: ↑ 5 nbachiyski20 months ago

Replying to dd32:

I'm curious as to why the notion of introducing an extra query var in place of status is really needed in this case?

I don't see any discussion here as to why one was chosen over the other, and as more people start using custom comment types with improving API's, I think it's going to fast become confusing for some, especially for example, when the extra query variable offers no obvious benefit over something such as 21101.diff to the untrained eye.

We had the conversation in IRC earlier today, sorry for not posting it here.

In my opinion using status will be more confusing.

Currently, all the values for status are special strings, which map to a specific value of the comment_approved column. Developers know that if they use a value, which isn't one of the special ones, they will get the sensible default comment_approved = '0' OR comment_approved = '1'.

Also, in general, when a variable name is different from the column name, it sets the expectations that it is handled in a special manner and doesn't set the column value directly.

If we use the same variable for setting the column value directly we will both break those expectations, we may break some code, which relies on the old behaviour (for example setting it to random string like default), and we won't let developers use the values hold and approved.

The last one is the worst, in my opinion. Imagine the documentation: "status sets the comment status via the comment_approved table column. Except the cases of the values approve and hold when the values in the database are '0' and '1' respectively". When I read this I cannot help but think "whoa, there must be years of weird legacy behind this inconsistency". And I don't want people to think stuff like this.

comment:7 nbachiyski19 months ago

@dd32 any further comments on that?

comment:8 follow-up: dd3219 months ago

@dd32 any further comments on that?

I don't agree with adding API's which duplicate an existing API with a reasoning of 'someone may be using the current API in an unexpected way'. But I'm not going to argue this one, and will leave it up to westi and nacin to decide in this case. I would not be surprised to see one become a 100% alias of the other in the releases to come.

comment:9 in reply to: ↑ 8 nbachiyski19 months ago

Replying to dd32:

@dd32 any further comments on that?

I don't agree with adding API's which duplicate an existing API with a reasoning of 'someone may be using the current API in an unexpected way'.

This, I agree, is the weakest argument in favour of the change.

I feel more strongly that it's important to keep the consistency, and to let developers use hold and approve.

comment:10 nacin19 months ago

  • Owner set to westi
  • Status changed from new to assigned

I have a preference for 21101.diff. It's not a strong preference, but having two separate query vars doing the same thing is confusing.

westi, your thoughts?

comment:11 westi19 months ago

So the discussion we had in IRC was: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-08-21&sort=asc#m441254

Basically, a new query variable allowed us to implement a clean API instead of the "dirty" one with caveats that we would have had if we tried to overload the old status argument that interprets the data passed in.

I still think that the clean api is more important that the "duplicated" parameter.

Happy to discuss this on Wednesday if people want to - seems like the dev-chat would be the best venue for further discussion.

comment:12 nacin19 months ago

This argument:

the problem is that $status has special values, like 'hold', which translates to "comment_approved = '0'"

and it gets complicated, I wouldn't be able to use 'hold' as the value of comment_appoved

Would make sense if 'hold' was just a special name used only in get_comments(). But 'approve' and 'hold' is used throughout the comments API to be the string equivalents/aliases for 1 and 0. When we get to a custom comment status API, the names will likely be 'approve' and 'hold', with a separate "value in the DB" property of 1 and 0.

Hence why I still like 21101.diff. It's not much different than the default order of post_date or something, which is what occurs when you pass it a value it doesn't understand. When you actually look at it together, it's pretty clearly not a hack or a dirty API.

comment:13 nacin19 months ago

This will be on the agenda for the Sept 26 meeting.

comment:14 nacin19 months ago

  • Keywords commit added

Per IRC meeting, 21101.diff.

comment:15 ryan19 months ago

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

In [22068]:

Allow get_comments() to query for explicit value of comment_approved.

Props dd32, nbachiyski
fixes #21101

comment:17 follow-up: Otto4219 months ago

This seems to break the main listing on the edit-comments.php screen. It causes queries to be generated like this:

SELECT * FROM wp_comments WHERE comment_approved = 'all' ORDER BY comment_date_gmt DESC LIMIT 28

Naturally, there's no comments with comment_approved = 'all'.

comment:18 SergeyBiryukov19 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:19 Otto4219 months ago

  • Severity changed from normal to blocker

Confirmed, reverting [22068] fixes the issue. Suggest adding a check to have it ignore the "all" status. Something like:

elseif ( ! empty( $status ) && $status != 'all' ) 

on line 251.

SergeyBiryukov19 months ago

comment:20 in reply to: ↑ 17 c3mdigital19 months ago

Replying to Otto42:

This seems to break the main listing on the edit-comments.php screen. It causes queries to be generated like this:

SELECT * FROM wp_comments WHERE comment_approved = 'all' ORDER BY comment_date_gmt DESC LIMIT 28

Naturally, there's no comments with comment_approved = 'all'.

In 3.4.2 the query generated is:

SELECT * FROM wp_comments WHERE ( comment_approved = '0' OR comment_approved = '1' ) ORDER BY comment_date_gmt DESC LIMIT 28

Tested 21101.2.diff and can confirm that it fixes the query.

Last edited 19 months ago by c3mdigital (previous) (diff)

comment:21 nacin19 months ago

Seems like we are doing it wrong. "All" in 3.4 would have returned only 0 or 1. That isn't "all".

We should fix our arguments, or something. Passing "all" should not specify *any* where.

comment:22 Otto4219 months ago

Agreed, but if you change the meaning of all in this case, you're breaking backward compatibility. People might wonder why their spam or trashed comments suddenly show up in the "all" listing after upgrading.

Also, a lot of places in core use that "all" thing. Changing all of them to have more sane behavior might take a while. Could be something to put on the list for 3.6, mind you.

But right now, trunk (and Beta-1) is broken and not showing any comments on the main admin Comments page.

comment:23 nacin19 months ago

In [22081]:

Map 'all' to no status for get_comments() in the list table. See #21101.

comment:24 nacin19 months ago

Regardless of what we do (and I'm okay with the latest patch), [22081] seemed like a good idea, especially since it fixes the admin for the time being.

comment:25 Otto4219 months ago

  • Resolution set to fixed
  • Severity changed from blocker to normal
  • Status changed from reopened to closed

Yep, that seems to fix it for now. Later tickets should probably address the underlying issue with "all".

comment:26 c3mdigital19 months ago

Fixes the list table. Also fixes post.php ajax_get_comments when patch from #22039 is applied.

ryan19 months ago

all|full

ryan19 months ago

comment:27 ryan19 months ago

Usually 'all' really means all the things and 'any' means all "public" things. But I don't think we can change the meaning of all here. Patch uses 'full' to mean all the things. Or substitute your favorite synonym for all.

comment:28 ryan19 months ago

In [22090]:

Restore the behavior of the 'all' status for comment queries. Props SergeyBiryukov. see #21101

comment:30 nacin16 months ago

This caused a slight issue, #22901.

comment:31 stuffmc16 months ago

To me this is even more problematic with the (nice) implementation of being able to pass "any" comment status. Image a blog having 8 different statuses, it means to retrieve 6 of those 8, we need to send 6 XML-RPC calls? (Sorry to "mix" XML-RPC here but this is relevant I think). Maybe the problem could be fixed from the XML-RPC perspective/interface.

Note: See TracTickets for help on using tickets.