#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)
Change History (36)
#5
follow-up:
↓ 6
@
12 years 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.
#6
in reply to:
↑ 5
@
12 years 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.
#8
follow-up:
↓ 9
@
12 years 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.
#9
in reply to:
↑ 8
@
12 years 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
.
#10
@
12 years 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?
#11
@
12 years 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.
#12
@
12 years 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.
#17
follow-up:
↓ 20
@
12 years 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'.
#18
@
12 years ago
- Keywords commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
#19
@
12 years 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.
#20
in reply to:
↑ 17
@
12 years 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 28Naturally, 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.
#21
@
12 years 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.
#22
@
12 years 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.
#24
@
12 years 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.
#25
@
12 years 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".
#26
@
12 years ago
Fixes the list table. Also fixes post.php ajax_get_comments when patch from #22039 is applied.
#27
@
12 years 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.
#31
@
12 years 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.
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.