In the past when notifications tries to get the user subscriptions I have received an incorrect query which would look something like:

SELECT s.*, f.* FROM notifications s INNER JOIN notifications_fields f ON s.sid = f.sid WHERE s.uid = 100 AND event_type = 'node' AND (())

What I noticed was that there was an implode to add other constraints; however users might not necessarily have them (or other modules might not implement them).

I suggest changing this portion to (its on line 349 of notifications.module) - I'll just show the patch

@@ -347,6 +347,8 @@
 
     $sql = 'SELECT s.*, f.* FROM {notifications} s INNER JOIN {notifications_fields} f ON s.sid = f.sid '.
-      implode(' ', $query['join']) . 
-      " WHERE s.uid = %d AND event_type = '%s' AND ((". implode(') OR (', $query['where']) .'))';
+      implode(' ', $query['join']) ." WHERE s.uid = %d AND event_type = '%s'";
+      if (sizeof($query['where']) > 0) {
+        $sql .= " AND ((". implode(') OR (', $query['where']) .'))';
+      }
     $result = db_query($sql, $query['args']);
     while ($sub = db_fetch_object($result)) {

Comments

David Goode’s picture

David Goode commented

Hello,

Could you please provide more details about your past usage case where this presented an issue? While the fix looks pretty harmless, none of the current notifications modules *should* ever run into this issue because:

1. notifications_user_get_subscriptions is and should only be called when there is an object to which you need to find out if a user is subscribed. To get all the subscriptions (as in the user/%d/notifications/thread page), your own query should be written. This should be documented in the function comments, but isn't.

2. This means that this function will always expect to have a user, event_type, and an object supplied. Then, when it calls the notifications hook to fill the query, the module implementing that event_type will always be expected to have some conditions for whether any subscriptions of that event_type for that user apply to the current object.

For instance, the notifications_content module implements event_type 'node'. When its notifications hook is called to fill the query, it checks that the event type is 'node', takes the node it is passed, and adds conditions for the fields so that it only returns subscriptions for which either the 'content_type', 'nid', or 'author' field match the node.

Basically, having an event type that applies to every single node, without any conditions provided in 'query' operation, is *NOT SUPPORTED*, and will crash the query as in the use case you provided. Do you have a possible use case for this type of subscription? If it is a major need, it would require not just repairing this function as you said, but also editing and testing many other functionalities in notifications to add this support.

If you have an event type like this, your immediate options are to either modify it to use conditions, or create a dummy field-value set for all notifications within this event which can be queried for.

If, on the other hand, you or a module ends up calling notifications_user_get_subscriptions improperly, look into whether additional checks are needed to determine if you actually have an object to pass into it for which to find subscriptions.

David

jose reyero’s picture

jose reyero commented
Status: Active » Fixed

While I basically agree with David here, we'll try to provide flexibility for other modules doing whatever they like as long as standard modules keep working.

So I've added a a pair additional checkings for that query building, should work now with any parameters.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.