Scheduler breaks on Cron if a node has been deleted outside Drupal.

I've applied this patch as I was having the issue exactly as described. The error messages are now gone. Thank you.

What I have not yet fully tested is the ability for scheduler to continue running as intended through cron.

Also, why does this happen? Surely a deleted node shouldn't exist anywhere any more, shouldn't cleaning that up be the more elegant solution? I have an entry in the Scheduled table (admin/content/scheduler) of a non-node that I do not know how to properly remove as it appears nowhere else.

Thank you anonym-developer for reporting the problem and providing a patch, and also thanks Cromicon for your feedback. There are hooks which can be invoked when a node is deleted and we should use these to tidy up the scheduler table when a node is deleted. Marking this back to 'needs work' because a solution will probably involve doing both of these.

Jonathan

It looks like it already does. So I suppose it fails only if the node was deleted in a dirty way, not invoking the hook. If I'm not wrong, the above patch is sufficient.

function scheduler_node_delete($node) {
  db_delete('scheduler')
    ->condition('nid', $node->nid)
    ->execute();
}

And the same check in _scheduler_unpublish() of course.

Yes you are right, the problem would not normally occur if the node was deleted using drupal front end.

But the patch still needs work because as it stands the non-existent node row will remain in the scheduler table forever. It would be better to do something like:

if (empty($n)) {
  delete the unwanted row in the scheduler table
}
else {
  do the existing processing. 
}

Jonathan

As much as I don't want to support people deleting nodes in ways that aren't node_delete, it's always good to plan for the edge case. Attaching a patch that includes that, as well as handles unpublishing non-existant nodes as well.

Hi,
I've not run this code, but patched the file and compared. Yes, it all looks good.

May I suggest that in _scheduler_unpublish() the $result=TRUE is only set inside the block where the $n was a valid node which was unpublished, ie it should return false if the node(s) did not exist.

Good call! Attached is a patch that does just that.

Thanks for taking my suggestion. I see you have also added $result=FALSE in _scheduler_publish() which is good.
I have tested this and it works fine, cron does not break.

One very minor thing I noticed. Within the "foreach ($nids as $nid)" loop, sometimes the code refers to $n->nid and sometimes just to $nid whereas these are both the same value. Do you think it is worth making these consistent? Otherwise it gives the impression that they are different things and looks a bit confusing. Just an idea.

Jonathan

Just looked at the query which selects the nodes from {scheduler}. It is doing a left join with {node}. If that was changed to an inner join it would only return good real nodes for processing.

Then the majority of the code could stay as-is, and we could have a simple tidy-up query to delete rows in {scheduler} which have no corresponding row in {node}

Is that a simpler way to do it? rather than have the extra nested if-then block?

Jonathan

Forgot to change the status. I'd like some others opinions on my idea in #12.

The issue #2011692: Prevent cron errors when scheduled nodes no longer exist is dealing with a very similar problem, but in a tidier and more powerful way. It will cope with the same situation as described above, so I am marking this as closed (duplicate).
Jonathan