node_service_save in node_service.module would be significantly more useful if it returned the newly created or updated node.

I'm pretty sure that was the author's intention since node.save specifies a "struct" return type in node_service_service(), though no struct is returned.

This is easily solved by adding:
return $node;
at line 88.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bas.hr’s picture

Category: bug » feature

this would be very useful, I've patched myself node_service.module since I need ID of inserted node

Voting for this

RobLoach’s picture

Status: Active » Postponed (maintainer needs more info)

Could you post the code you had?

RobLoach’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
1.15 KB

We could return the Node ID. If we return just the $node, I'm not entirely sure if it would include all "new" data associated with it.

eli’s picture

Returning the entire $node works fine. I've got code that relies on it.

That's almost certainly what the author of this module intended -- node.save specifies a "struct" return value.

snelson’s picture

If any bugs popup, we can always do a node_load after the save. Either way, I agree that the node should be returned.

RobLoach’s picture

Status: Needs review » Needs work

eli: Could you post a patch to return the $node object?

eli’s picture

Category: feature » bug
Status: Needs work » Needs review
FileSize
463 bytes

Here you are.

Attached patch simply adds a "return $node" to node_service_save().

marcingy’s picture

Assigned: Unassigned » marcingy
marcingy’s picture

The only issue with this is that as the node has not been saved way may not have a true node to return,. If it was a update this will be ok however for a new node we won't have an id etc.

eli’s picture

You sure about that? I'm using it to create new nodes and I'm getting a full node object...

marcingy’s picture

My comment yesterday was made after just eye balling the code.

I'll do a full test today and let you know how it goes.

marcingy’s picture

Status: Needs review » Patch (to be ported)

Patch commited needs to be ported to d6

RobLoach’s picture

Version: 5.x-1.x-dev » 6.x-1.x-dev
marcingy’s picture

Yes. Sorry Rob at the moment I'm sort of using this as a reminder and todo list!!!

RobLoach’s picture

Status: Patch (to be ported) » Fixed
marcingy’s picture

Status: Fixed » Closed (fixed)
NaX’s picture

Status: Closed (fixed) » Needs review

This patch only works on saved nodes not newly created nodes.
It looks like drupal_execute() returns the new node path.
So I have hacked my node.save method like this.

function node_service_save($edit) {
  if ($edit['nid']) {
    $node = node_load($edit['nid']);
    if ($node->nid) {
      $ret = drupal_execute($node->type .'_node_form', $edit, $edit);
    }
  }
  else {
    $ret= drupal_execute($edit['type'] .'_node_form', $edit, $edit);
    $path = explode('/', $ret);
    if ($path[0] == 'node' && is_numeric($path[1])) {
      $node = node_load($path[1]);
    }
  }  
  if ($errors = form_get_errors()) {
    return services_error(implode("\n", $errors));
  }
  watchdog('services:node.save', t('@type: updated %title.', array('@type' => t($node->type), '%title' => $node->title)), WATCHDOG_NOTICE, l(t('view'), 'node/'. $node->nid));
  return $node;
}

Sorry I did not have time to create patch.

DJL’s picture

Thanks for the 'patch'.

Why return the whole node? (I am bandwidth limited)
I have just created it, so I should already know all that is in it?
All that is needed is to return the node->nid
so I can remember it for future reference or ask for it using node.load.

eli’s picture

NaX: Good catch. Something changed with the way this whole module works starting in rev 1.5.2.3. It used to just call node_save($node). I don't quite understand the reason for the change.

DJL: There are other reasons why people would want the whole node. If you just want to return the nid, I suggest creating your own services module with a function that does just that (it's easy, I promise!)

eli’s picture

Ah, I see. It looks like it was to fix this security issue: http://groups.drupal.org/node/12573

There's gotta be a better way to do this (and check permissions) though.

DJL’s picture

There are other reasons why people would want the whole node. If you just want to return the nid, I suggest creating your own services module with a function that does just that (it's easy, I promise!)

Surely it is easier to expect the user to issue a noad.load (nid) if they need the extra information - which does not require the creation of a new service. The simpler the better?

eli’s picture

I'm not sure either way is "easier."

But returning the whole node is the documented behavior, it's how things used to work, and people may have code that relies on it. I think it makes a lot more sense to just fix the bug and put things back the way they were rather than use this opportunity to change the function's signature.

DJL’s picture

But returning the whole node is the documented behavior,

Where is this documentation? - I must admit I am operating somewhat in the dark here.
Have used the old xmlrpc to post pages of stuff so am basing what I do on my experience with that.
Reading the docs would help me progress and stop me making inappropriate suggestions!

it's how things used to work, and people may have code that relies on it.

Forgive me, I though services was a new module still under development.

jrbeeman’s picture

NaX's patch seems to do the trick. Any chance of this making it into the distribution? The node.save method is far more useful to remote services when they can get back data about the node they just created.

jrbeeman’s picture

Just ran into a case where NaX's code isn't sufficient. If the node being saved exists already and is being altered, the returned node is still the static cached node (i.e. the original node). This altered version of the method should fix that:

function node_service_save($edit) {
  if ($edit['nid']) {
    $node = node_load($edit['nid']);
    if ($node->nid) {
      $ret = drupal_execute($node->type .'_node_form', $edit, $edit);
    }
  }
  else {
    $ret = drupal_execute($edit['type'] .'_node_form', $edit, $edit);
  }
  if ($errors = form_get_errors()) {
    return services_error(implode("\n", $errors));
  }
  $path = explode('/', $ret);
  if (($path[0] == 'node' && is_numeric($path[1]))) {
    $node = node_load($path[1], NULL, TRUE);
  }
  watchdog('content', t('@type: updated %title.', array('@type' => t($node->type), '%title' => $node->title)), WATCHDOG_NOTICE, l(t('view'), 'node/'. $node->nid));
  return $node;
}
civicpixel’s picture

I was working through one of the great screen casts (http://files.thisbythem.com/screencasts/services-2.mov), but ran into several issues when I got to the pieces involving saving / editing nodes. Saw the comments posted on the group http://groups.drupal.org/node/13138, but didn't notice this issue until later. In order to get through the rest of the screen cast this evening I updated the drupal_execute code in node_services.module in the current dev release. I'm not sure if the above suggestions were already implemented in dev, or how much overlap my patch has with the above -- but I'll add my patch here if anyone wants to take a look at it. It's working for me, but I haven't tested it beyond the screen cast =)

--
Brian Hiatt
Civic Pixel

ddanier’s picture

@civicpixel: Your patch really did the trick, wothout it I was not able to save anything. Thanks ;-)

Anyway, I have updated your patch to return the node-nid of the created node...to fix the original issue here. Patch is appended.

brmassa’s picture

Assigned: marcingy » brmassa
Status: Needs review » Fixed

Guys,

i just commited the code David (with few changes). Thanks everyone! Its on CVS and soon on the next release.

Just remember that it will return the new node ID, not the entire node. Its consistent with several other web services.

regards,

massa

ddanier’s picture

Cool, thanks! Great work ;-)

marcingy’s picture

Status: Fixed » Closed (fixed)