Once I deleted a node, which should be published somewhen. unfortunately the cron job has not been deleted from ´scheduler´ and everytime the cron has run I had the following error:

• Notice: Trying to get property of non-object in _scheduler_publish() (line 721 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$created in _scheduler_publish() (line 722 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$type in _scheduler_publish() (line 723 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$type in _scheduler_publish() (line 727 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$type in _scheduler_publish() (line 734 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$title in _scheduler_publish() (line 734 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$nid in _scheduler_publish() (line 734 of /sites/all/modules/scheduler/scheduler.module).
• Notice: Undefined property: stdClass::$type in _node_extract_type() (line 370 of /modules/node/node.module).
• Notice: Undefined property: stdClass::$title in node_publish_action() (line 3674 of /modules/node/node.module).

the protocol was saying:
EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7409 of /includes/common.inc).

what happened is, that in the script scheduler.module has tried to load the node on line 740 which wasn't existing. I could not reproduce the situation when the node has been deleted an the reference in the scheduler table has stayed.

the real problem was, that the script has stopped after the error, so all my nodes stayed unpublished.

anyway I have added a fallback to be sure that the script does not stop at this point.

Comments

Status:Active» Needs review

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.

Priority:Major» Normal
Status:Needs review» Needs work

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

Status:Needs work» Needs review

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.

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

And the same check in _scheduler_unpublish() of course.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.53 KB

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.

Status:Needs review» Needs work

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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

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

Title:Scheduler breaks on Cron if a node has been deleted.Scheduler breaks on Cron if a node has been deleted outside Drupal.
Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Closed (duplicate)

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