Exposing Commenting Functionality via Services

how do i apply this patch?

I did quite a bit of testing of this today and it is good functionality but the patch needed some work, so I've rerolled it. Specifically

- many coding style cleanups

- now uses paramaterized db_query (previously had $nid right in the sql string)

- forced the input filter to 1 ('Filtered HTML') on new comments. I removed this, and it now acknowledges the input filter in the array, or the default on the remote system if not specified. I don't think this is a problem.

I couldn't figure out how the heck you properly deal with a new directory for patching, so this actually creates the two files in whatever directory the patch is run in. In the end they should be in comment_service.

and this is more appropriate

Re-rolled with:

* code style cleanup
* renamed comment.save to node.comments.save
* renamed comment.getNodeNewCommentCount to node.comments.countNew
* renamed comment.getNodeCommentCount to node.comments.countAll
* changed comment.getNodeComments to node.comments.all
* removed param checking in individual functions, this should be handled by services core eventually
* wrapping help string and field descriptions in t()
* removed most commented nodes, can be done with views
* fixed CVS id tag in .info

Would like some feedback on the method naming and namespace "node.comments.*" instead of "comment.*".

Can I ask what the thought was behind changing the namespace? Was there an issue with "comment.*"? (I'm curious)

I think namespaces are good in general. Could there be confusion that "comments" are "nodes" in the db to people who see this namespace starting with node?

You're probably right ...

I've modeled much of my original services off of the Flickr API. They do things like 'flickr.photos.comments.getList'. Also, in Ruby on Rails and ActiveRecord land (which is pretty awesome stuff for those that don't know), we would see it going a similar direction with the way it handles associations. Like if nodes were done in ActiveRecord and OOP, we would do something like (and I'll do this in php):

$node = Node::load($nid);
$comments = $node->comments();

Since comments are always tied to a node, it makes the most sense (to me) to have them in the namespace node.comments. It was more of a personal preference which is why I was asking for feedback.

Changing the names is a minor task, and I'd to hear from a few others so we can lock it down. Too, because this patch has been sitting in the queue for a while, I'm sure there are plenty of installs using it so I'll have to be careful. I just didn't like the original naming, and wanted to make sure to put some thought into it before it goes in.

Thanks Chris,
Scott

Here's a patch with some more changes:

1. More checks for user access
2. Do not show password, hostname and mail to user who doesn't have permission to administer comments.
3. Changed namespace comments into comment (module is called comment_service, so i tried to keep the same naming)
4. Changed comments.all into comment.load and added optional arguments "count" and "start", so it can load only X comments, starting from Y :).

Until this is accepted by maintainers, perhaps someone could ship it as a Contrib module? Good idea?

These services need access control. See node.save service for an example. This is a relatively new feature of Services.

I need this for a client so I've pulled the existing patch out and done the following

- Upgraded to 6.x
- Added access functions
- Pulled out functionality into a .inc file
- Added docblocks, brought up to coding standards, etc

This has had some basic testing but not a great deal. I'd love to see someone pound at it a bit. I would especially like attention / comment to the following:

- I'm not sure what the original poster was getting at by joining user in comment_service_node_comments_all. It seems to me like it should just _comment_load() each comment and return those, and if the caller wants user info they can get it themselves.
- I didn't create any new perms for these services, I just pass requests off to user_access for the existing comment permissions. I think this is perfectly fine personally but I'd be willing to hear other opinions.
- In the permissions for comment_service_save, I just check to see if the user has either 'post comments' or 'post comments without approval' permissions, and if so just pass them through to let the comment_save() handle it from there.

I don't know how to create a new directory in a patch file, so right now if you run this from services root, it creates three files right in services/services

comment_service.info
comment_service.module
coment_service.inc

You'll have to create the comment_service directory and move them in there yourself if so desired.

Comments welcomed. I'd love to see this get in, I think its an important part of the package.

Also regarding the namespace issue, I feel this is appropriate in the comment.* namespace, following the logic that services interfaces to drupal functionality should live in the same namespace that drupal uses. So interface to comment_save(), comment_num_all() should live in the comment.* namespace, just like interfaces to node_load() and node_save() should live in the node.* namespace. For the sake of Drupal maintainability and consistency this is really the way to go.

sorry....but doesn't exist an easy way to install this service without the awful and complicated install patch process in windows?
i tried to create tree files, and used the code from the patch without the "+" but it doesn't show in services pages...

how can i do it without install any program? i'd like to do manual.

Just for my own reference there was some conversation around this patch at this sprint and I'm going to reroll, but i wanted to get it into the issue

[15:52] About the comment service, shouldn't it be possible to load a specific comment?
[15:54] I expected comment.load to have the argument cid
[15:55] Maybe the current load function should be named nodeComments?
[16:03] I second that
[16:04] there should be a way to load just a single comment
[16:14] as far as a single comment load, that should be pretty easy. I'll add that in.
[16:23] it says nid and comment object are required
[16:24] while argument list only contains comment object
[16:24] * heyrocker looks
[16:24] I've tested comment save with the jsonrpc_server and it works fine
[16:25] ah the wording in that comment is unclear, i'll adjust it
[16:25] it should say "Required fields in the comment object"
[16:25] I believe it does. It doesn't however when you use admin/build/services/browse/comment.save because you can't fake an object in the text field

Also this patch does need access control so that will be coming

New patch, same install instructions as above. Changes:

- I decided the whole joining out to user and specifying the fields to get was not useful and pulled it out. Anyone who has a good use case for this, feel free to to say so.

- After discussion with marcingy and snelson I added in access parameters for the services

- I added comment_load so you can load a single comment by cid

- I renamed the function to load comments by node to load_node_comments

I think that's it.

* comment_service_load_node_comments has a $fields parameter that is not used at all.
* comment_services.module doesn't seem to be included as part of the patch just the .info and .inc

I haven't tested the patch just done a visual review.

This is what I get for not actually testing the patches. New patch attached.

Hehe eyeballing it looks good will test in the next few days

Here's a D5 version that incorporates heyrocker's changes (as of #18). Also attached patch #18 revision with a minor change:

function comment_service_node_comments_count_new($nid, $since = 0) {
-  return comment_num_new($nid, $timestamp);
+  return comment_num_new($nid, $since);
}

Looks like this is pretty good... when's it going to get rolled into the main services release?

Where I can download this service without patching?? Anyone have a link?

Thanks.

Nick

Will review however d5 version won't be committed

Yeah, really expect it rolled into the main services release, when is it possible?? really expect that.

Did a little of tidy up removing some checks that were need for optional parameters and removed the key = false to keep it consistent with every other service.