Convert comment/nodeComments action to node/x/comments relationship

This was fixed in the latest comments update

heres a re-roll for the 7.x-3.x branch to be applied via git apply patchname

Here's lots of random and probably not very relevant comments but I don't have time to actually test the code now so all I could do was read through the patch and nitpick it ;)

---

+++ b/resources/node_resource.inc
@@ -130,6 +130,37 @@ function _node_resource_definition() {
+        'comments' => array(

We need a module_exists() somewhere as comments are optional - right?

+++ b/resources/node_resource.inc
@@ -130,6 +130,37 @@ function _node_resource_definition() {
+          'help'   => t('This method returns the number of new comments on a given node.'),

Weird indention?

+++ b/resources/node_resource.inc
@@ -130,6 +130,37 @@ function _node_resource_definition() {
+              'type'         => 'int',

Choose to either align all "=>" or none - what do we do in the rest of Services?

+++ b/resources/node_resource.inc
@@ -130,6 +130,37 @@ function _node_resource_definition() {
+              'source' => array('path' => 2),
...
+              'source' => array('path' => 3),

Should these properties really be part of the path or am I misunderstanding something? Count and offset should be query properties - not part of the path in my opinion

+++ b/resources/node_resource.inc
@@ -130,6 +130,37 @@ function _node_resource_definition() {
+              'name'         => 'start',

Are we using "start" elsewhere? Isn't "offset" a more common term?

+++ b/resources/node_resource.inc
@@ -130,6 +130,37 @@ function _node_resource_definition() {
+        ),        ¶

Trailing white space here

+++ b/resources/node_resource.inc
@@ -424,3 +455,38 @@ function _node_resource_load_node_files($nid, $include_file_contents = TRUE) {
+    $comments[] = comment_load($record->cid);

We could use comment_load_multiple() here instead - right?

Powered by Dreditor.

Heres an updated patch regarding voxpelli's comments

No comment_load_multiple() in that patch? Not possible to use it?

OK fixed, i also added the tests for the new location.

I don't think it is nice to do

if (module_exists('comment')) {

We should use hook_services_resources_alter in order to alter node resource and add comment relationship.

Callback function can stay in comment_resource.inc file but alter of resource should be done through hook.

Gents, what do you think about this?

I dunno, does that work?

@ygerasimov: Since we include all resource declarations anyway I think it's a bit overkill to use hook_services_resources_alter(). If the resources was declared by two separate modules I would agree with you, but for core I think it's best if all properties of the a resource is declared in that resource's include file.

A good use case of hook_services_resources_alter() in core is the REST Server's extension of node resources with feed models in rest_server_services_resources_alter(). As the REST Server is a separate module from Services core those extensions shouldn't be made in Services core but in the module itself.