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.
Comment | File | Size | Author |
---|---|---|---|
#9 | scheduler-deleted-node-handling-1410184-9.patch | 5.64 KB | wbobeirne |
#7 | scheduler-deleted-node-handling-1410184-7.patch | 5.53 KB | wbobeirne |
buildin_fallback.patch | 3.24 KB | anonym-developer | |
Comments
Comment #1
anonym-developer CreditAttribution: anonym-developer commentedComment #2
Cromicon CreditAttribution: Cromicon commentedI'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.
Comment #3
jonathan1055 CreditAttribution: jonathan1055 commentedThank 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
Comment #4
GaëlGIt 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.
Comment #5
GaëlGAnd the same check in _scheduler_unpublish() of course.
Comment #6
jonathan1055 CreditAttribution: jonathan1055 commentedYes 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:
Jonathan
Comment #7
wbobeirne CreditAttribution: wbobeirne commentedAs 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.
Comment #8
jonathan1055 CreditAttribution: jonathan1055 commentedHi,
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.
Comment #9
wbobeirne CreditAttribution: wbobeirne commentedGood call! Attached is a patch that does just that.
Comment #10
wbobeirne CreditAttribution: wbobeirne commentedComment #11
jonathan1055 CreditAttribution: jonathan1055 commentedThanks 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
Comment #12
jonathan1055 CreditAttribution: jonathan1055 commentedJust 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
Comment #13
jonathan1055 CreditAttribution: jonathan1055 commentedForgot to change the status. I'd like some others opinions on my idea in #12.
Comment #14
jonathan1055 CreditAttribution: jonathan1055 commentedThe 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
Comment #15
jonathan1055 CreditAttribution: jonathan1055 commented