if a node/comment/whatever is deleted, also delete related notifications

here is a patch against my hacked up 6.1

Nasty as it unserializes params, but the reality is deleting comments happens SOO rarely.

Maybe 6.2 actually uses the oid field for the cid, that would make this way easier.

Well the real approach would be to leverage notications_event.oid . I believe, just looking at the code that that is the cid. so you could then do

SELECT eid from notifications_event where (comment insert) and oid = $cid;

delete from notifications event where eid = %d
delete from notifcations_subscriptions where eid = %d and sent = 0; /* and cron = 1 ?? */

Which would be fast, in fact you could do a MySQL multitable delete as well.

Maybe we can limit the query by using where params like '%%%s%%' and put the cid in there?

That would make the query nasty. The only thing missing from that is a LOWER() :-D.

Because comments are usually deleted because they are by spam users, I would argue that looping through a users comment events would probably be faster then doing a nasty query like that. Of course this is different if my assumption about spam users is false.

Really I think notifications2 makes this much better then this ugly patch.

ok so notifications_content_comment() doesn't use the 'oid' property.

but notifications_content_nodeapi() does. So the steps to get this for notifications2

1.) notications_content_comment() set the 'oid' key for the notifications_event() array.
2.) in hook_notifications case: 'query' need to join node/comment on cid/nid AND status >= 1

This of course highlights a problem with notifications, it treats comments as secondary citizens. Makes checking the state of a comment very very difficult.

We don't need more patches. When the comment doesn't exist, the notification shouldn't be sent and the event deletec.

See notifications_content_notifications('event load'), event should be marked for deletion $event->delete.
See notifications_load_event, which should return NULL in this case, meaning we don't process the event anymore.

So this is actually a bug.

ahh perfect i got it then.

Needs review.

I haven't tested whether it really fixes the issue in question, but the module actually fixes a bug.

Thanks

Ahh so why it fixes the problem.

See the loading from the database returned FALSE. Yet, it still got a 'name' transfering it from a boolean variable to an object. Then it would pass the check in event_load, as its an object not a boolean.