Make WordPress Core

Opened 8 years ago

Last modified 8 years ago

#40073 new enhancement

WP_Comment_Query should support a comment type of "comment"

Reported by: rogerlos's profile rogerlos Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Comments Keywords:
Focuses: Cc:

Description

Scenario: User is leveraging the comment_type field in their theme or a plugin, and desires all comments to have a type. They assign "regular" comments a comment_type of "comment" as that seems to make sense.

If they then ask for those comments using get_comments or another function which leverages WP_Comment_Query:

$comments = get_comments( [
    'type' => 'comment',
] );

The returned list will not return any comments which have comment_type set to "comment", only those with no type at all.

As this behavior (the swallowing of "comment" as a type entirely, rather than looking for ['','comment']) is undocumented, perhaps WP_Comment_Query could be altered as such:

715   case 'comment':
716   case 'comments':
717     $comment_types[ $operator ][] = "''";
___     $comment_types[ $operator ][] = "'comment'";
___     $comment_types[ $operator ][] = "'comments'";
718     break;
719	
720   case 'pings':
721     $comment_types[ $operator ][] = "'pingback'";
722     $comment_types[ $operator ][] = "'trackback'";
___     $comment_types[ $operator ][] = "'pings'";
723	break;

A sounder approach would probably be to allow a flag to be passed:

708   foreach ( $_raw_types as $type ) {
___      if ( empty( $this->query_vars['ignore_default_types'] ) ) {
709         switch ( $type ) {
...
729         }
___      } else {
___         $comment_types[ $operator ][] = $wpdb->prepare( '%s', $type );
___      }
...
735   }

An alternative might be to throw an exception when setting comment_type to one of the default values using wp_new_comment, wp_insert_comment and similar functions.

(I feel since there is no way to retrieve a comment when the type is one of these "reserved" words with standard WP functions, comments should probably not be able to be saved with a "reserved" type using standard functions if WP_Comment_Query is not altered.)

At the very least, the documentation for comment "insert" functions should mention that there are reserved keywords on comment_type, note what they are, and that if you use one of them you will need to use the comments_clauses filter to allow them to be retrieved.

Change History (2)

#1 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to Future Release

@rogerlos Thanks very much for the clear and sensible ticket.

I agree that the current situation is not ideal. If we're going to allow comment_type to be set to 'comment', then we need to be able to allow these comments to be queried by type as well. As you indicate, this is a problem not only for 'comment', but for 'pings' and 'all' as well.

Comment query type handling is already too "magic" for my taste. The fact that 'comment' fetches and 'pings' fetches 'pingback'/'trackback' is confusing, and is there only for legacy reasons. Let's not make things worse by trying to lump more into these clauses, as in your first suggestion.

I think I like the idea of combining a few of your suggestions:

  • Introduce a new parameter (like your suggested ignore_default_types, but maybe we can come up with a better name, since it's less about "ignoring" than about translating reserved kewords) that causes type to be interpreted literally.
  • Update the wp_insert_comment() and wp_update_comment() documentation to strongly discourage the use of comment type 'comment', 'pings', 'all'.

I don't think we can actually *prevent* the use of these comment types, for backward-compatibility reasons. I can imagine that there are many cases like yours where developers are passing 'comment' and expecting it to be recorded in the database.

I'd like a second opinion from @rachelbaker about this.

#2 @rogerlos
8 years ago

Thanks for looking at this.

I agree that throwing an exception is the least desirable solution, and that passing a flag is probably the most flexible and backwards-compatible.

I also agree we should let people set their comment types to 'comment' or 'all' or 'pings' if that's what they want to do, as long as it's documented somewhere how to get them back.

Naming parameters and variables is something I'm awful at, so I'm confident there is a better name for the flag which might actually even tell a user what it does just by reading the var name.

One thing I did not note about get_comments:

Even when the type parameter failed, I expected type__in to NOT fail, as I assumed it would be "literal", ie, outside of whatever magic was happening that caused type to fail. I think my line of reasoning was "if someone is going to specifically generate an array of types, surely the class will take them all literally".

I have no basis for that other than my own prejudices, but it might also be a way forward if a flag is decided against. Seeing as there are only two magic types, plus a choice which returns everything, it seems unlikely a user sending type__in would want anything other than literal types.

Finally, if WP_Comment_Query is modified, I suggest WP consider that core start setting a comment type when saving (how about "comment"!).

There are a number of interesting things which can be done by saving appropriate content as "comments" even when they're not actual comments (for example, gaining access to spam checks, flood prevention, etc), and having no type by default can make it difficult to work within the data saved in the table.

Note: See TracTickets for help on using tickets.