WordPoints / wordpoints

Points plugin for WordPress
GNU General Public License v2.0
20 stars 15 forks source link

Register more entity children for comments #529

Closed JDGrimes closed 7 years ago

JDGrimes commented 8 years ago

Mostly attributes I think. Similar to #422.

JDGrimes commented 7 years ago

There are comment_author, comment_author_email, comment_author_url, and comment_author_IP, which kind of form a set. For the latter three, I don't really see a lot of use, unless we had a "not empty" condition. Also, they should each really be custom data types (email, URL, IP [probably not needed at all]). The comment_author is just the name that the user has entered; their ID if they are a logged-in user is stored in user_id, which we already have a relationship set up for. So in short, I don't see much use in registering any of these at the moment.

Then there is comment_date and its GMT equivalent, of which I suppose we should register the former at least, as we did for posts in #422. Making it actually usable will still depend on #568, though.

comment_karma is not, and apparently never has been, used in core. I found this good discussion on comment_karma, which explains that. So making this available would be more of a plugin integration thing, that a core feature. So let's leave it to the modules.

comment_approved doesn't seem necessary right now either, since it is basically the comment status field, and we don't have a good way of handling status fields yet. (We don't handle post status either.)

comment_agent is the author's HTTP user agent when they leave the comment, so I don't see the need to add that, it isn't really a user-facing thing. (Possibly would be a separate data type anyway.)

The comment_type field actually raises an interesting question. For regular comments, it can be either empty or 'comment'. Core also supports the comment types of 'pingback' and 'trackback'. I guess really we should be registering separate entities for pingbacks and trackbacks, apart from regular comments, if we want to support them (and really I don't think that is a priority). But for proper handling of the regular comments, I suppose that we need to modify the comment entity or the related actions so that only regular comments, and not pingbacks and trackbacks and other comment types, are triggering the comment-specific events. I can imagine that some users might want to award for all comment types the same, but then multiple reactions can be used to do that. The main issue that we might run into would be plugins that use custom comment types, where our events were previously firing for them, but wouldn't anymore. But that should ultimately be remedied by modules. And I guess really, this discussion should be moved to its own ticket. The main point for us here is that we don't need to introduce the comment type as an attribute.

JDGrimes commented 7 years ago

We hit an issue when it comes to the comment_parent: the relationship needs to specify the dynamic portion of the entity slug, which is the post type, for both the primary and related entity slugs (which are the same, because this is a relationship between to comments on a single post). For the relationship between a comment and a post we had the same issue, and it was solved by including the dynamic portion in the slug of the relationship itself, so that it was post\* instead of post. I guess we'll have to do the same thing here, although parent\* just wasn't quite what I had expected here.

Note that we have the same issue with the post parent entity introduced in #422, we just hadn't noticed it yet.

JDGrimes commented 7 years ago

Actually, there is no need to make the child slug dynamic (thankfully), we can just utilize the parent slug, which is actually passed to the entity relationship class on construct, though we don't usually do anything with it (it isn't even a declared parameter).

JDGrimes commented 7 years ago

For posterity: note that I was going to open a similar ticket for the user entity, but I don't think there is much to add there right now. I was also thinking that possibly there might be metadata for users/comments/posts that we should be registered as entity attributes/relationships as well, but it doesn't look like there is much in core that really would be useful, at least on that I can see at the moment. So we'll open tickets for anything else as we find it.

JDGrimes commented 7 years ago

I just noticed that the content attribute for post types wasn't showing up in the hook conditions UI. Investigation led me to find that we actually made a mistake in https://github.com/WordPoints/wordpoints/commit/ccf3535cab9accb578664fe44d00947ee5050d3b#diff-e0c0b5fd60a7b34201ea9bad9f3dbc24R369, copying and pasting the comment date class as the class for the content attribute. Let's get that fixed.