Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.I realise this is not technically a bug report, as the module actually contains the message above and warns the user about this... but I have changed the SQL statement (both in the base module, and the blog module) to fix this issue (and so removed the warning message).
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 290948.patch | 4.22 KB | nburles |
| subscriptions.patch | 2.61 KB | nburles |
Comments
Comment #1
salvisWhoah, that's quite a feat! Can you try to line-wrap the SQL in a logical way, with indenting for nested parentheses (see subscriptions_queue() for an example)? It's too long for me to understand...
And remove the leading space in subscriptions_blog_ui.module.
I'm a bit concerned that this doesn't scale well. Maybe the other modules ought to use hook_count_user_subscriptions() rather than having subscriptions.admin.inc do it all for them?
Thanks for looking into this!
Comment #2
nburles commentedI have line-wrapped the SQL in a way that seems logical to me.
I have also removed the leading space (and line-wrapped that SQL) - I hadn't noticed the unwanted space.
How do you mean about the scaling? I can think of two possible meanings...
1) Scaling of the database server to handle larger datasets?
- In which case this is not an issue, as the query and sub-queries are using indexed fields, and so scale perfectly (and I have tested this successfully on a fairly large dataset).
2) Scaling of the code to allow addition of other modules (such as blog, or view, etc)
- This is fine, as other modules can still hook in using the hook_count_user_subscriptions(), as shown by the blog_ui module.
NBurles
Comment #3
salvisThanks for wrapping and explaining! I'm trying to remember why I chose to add the 'may differ' text at the time, rather than implementing it properly. Should have written a code comment at the time...
I don't think you've caught all the issues yet. For example, unpublished nodes should not be counted under node/nid. For node/tid, the counts may be thrown off by access modules. For correct results, the query probably needs to be split up and each part passed though db_rewrite_sql().
What you're dealing with is the possibility of things (nids, tids, types) getting deleted (see #291929: Use hook_taxonomy() and hook_node_type() to catch deletions and clean out corresponding subscriptions), but there are definitely more situations where the counts can be wrong. I'm not sure what we get with the "handle any additional functionality" part.
For the blog count, you probably intended to check that the author had an actual blog type post, not just a post of any type.
To really get this right, we probably need to use hook_count_user_subscriptions() also for the Subscriptions core modules and have them do the actual queries that they use for filling their pages. The problem with this is that it's quite a bit of db work to get a few numbers that most people won't even look at.
The reason for having the numbers is to get an idea of what to expect on the details pages (like, see where the action is). Of course it would be nice if they were exact, but I'm still not sure it's worth the effort...
Comment #4
nburles commentedI see what you mean about the other cases of causing an incorrect count (and I found another one just 10 minutes ago)... so maybe the 'may differ' text should stay where it is, but the count SQL could be updated to my count - as it is currently more accurate, even if not completely accurate, and uses very little DB time compared to the previous query as it is using only indexes. (Maybe the blog count is not correct, I may have miswritten that).
Comment #5
salvisI've just committed #291929: Use hook_taxonomy() and hook_node_type() to catch deletions and clean out corresponding subscriptions. It includes a database update to clean up old orphans, and it will avoid creating new ones. After updating, you should see the same counts with your patch as without — otherwise I haven't fully understood your patch yet.
Would you please try this out? I think it makes more sense to keep the data clean rather than adding code to filter out the junk.
Comment #6
salvisAs I wrote in #5, the counts may still differ for reasons beyond what your patch did, and your patch doesn't provide any benefit over what we currently have, or am I mistaken?
Please reopen if you want to pursue this further.