node_load() can return a non-object (i.e. FALSE). Validating the result is required. This is especially the case, when a node is being deleted by the Rules module just after its creation. I think the node might have a nid, but is already marked as deleted. So node_load() doesn't serve it, but instead returns FALSE.

An example of how to handle this:

<?php
function _userpoints_node_action_has_status_changed($nid, $action, $newStatus = 1) {
  // It seems the only reliable way to do this is to reload the node. It could be a direct
  // query to the node table, but node_load would be more resistant to change.
  // If the nid is empty, it's because they're making a new node
  if (empty($nid)) {
    return true;
  }
  
  $node = node_load($nid);
  
  if ($node===FALSE) {
    return TRUE;
  }
?>

Added the last 3 lines of code, which removed the error message. I'm not sure, if it's appropriate to return TRUE, though.

Comments

BillyMG’s picture

BillyMG commented
Assigned: Unassigned » BillyMG
Priority: Normal » Minor
Status: Active » Postponed (maintainer needs more info)

Well, the system returns new because it assumes that the node is being created if it doesn't already exist in the database. If the node didn't exist either way, then the status would have "changed" so I think returning true is still correct. The module already does validation, though:

if (empty($action) || empty($node)) {
    return true;
}

Is that not appropriate?

Anonymous’s picture

Anonymous (not verified) commented
Status: Postponed (maintainer needs more info) » Active

I'm sorry, I can't follow your thoughts. Perhaps I didn't make the issue clear enough?!

In the code posted above, $node can be FALSE. Therefore I added the validation (last 3 lines of the code).

The function otherwise will continue and reach a statement issuing the actual error message:

  if ($node->$action !== null && $node->$action != $newStatus) {

With $node = FALSE this results in an error ("property of non-object").

I didn't mean the other checks are wrong. I mean the $node has to be validated after fetching it via node_load(), because it is intended behavior of node_load() to return FALSE in certain cases.

BillyMG’s picture

BillyMG commented

Right, you're saying that node_load can return false if the node doesn't exist. However, here's the current code:

$node = node_load($nid);
  
// Oops? Must be a problem, return true by default;
if (empty($action) || empty($node)) {
  return true;
}

Do you see how the $node is already checked for false? The empty() function already checks to see if $node is false, which should protect against that issue.

BillyMG’s picture

BillyMG commented
Status: Active » Closed (cannot reproduce)

If this is actually an issue, please re-open or create a new ticket.