I'm curious about the architecture for Subscriptions as it relates to acting upon subscription records. As far as I understand it, the main Subscriptions module only manages the records relating to various subscriptions. It seems silly, however, to make every module that wants to perform actions with these records (such as sending them, in the case of subscription_mail) implement some variation of hook_cron() independently.
It seems like the modularity of the Subscriptions module as a whole could be improved by implementing hook_cron() in a subscription.cron.inc file.
I think it could be done in a task-agnostic manner by replacing the mail-sending code currently found in subscription_mail's implementation of hook_cron with two new hooks; one for digest notifications and another for sending individual notifications. Other subscriptions sub-modules could then implement these hooks to perform various actions (for example, in the case of subscription_mail, sending the notifications). This should make it easy for other developers to integrate the module with other messaging systems, Rules, and anything else they can come up with.
Does this make any sense? I'd be interested in submitting a patch, but I want to get some feedback from some more experienced developers.
Comments
Comment #1
salvisMoving the consumer of {subscriptions_queue} into the subscriptions.module would not exactly improve modularity. While I have not seen any alternative implementations of
subscriptions_mail.module, I don't want to give up that possibility.
No, on the contrary, that's how Drupal works. However, if multiple modules compete for the same resource on the same hook, then this can be problematic, without some kind of arbitration.
It would be nice to open up Subscriptions to other transport options, which is difficult as long as subscriptions_mail.module is the consumer of queue items and the sending agent. I could imagine splitting subscriptions_mail.module into two modules in order to add a variation point. Support for Rules is desirable, but I wouldn't want to introduce a mandatory dependency on Rules.
I'm currently working on moving Mail Editor and subscriptions_mail.module from the proprietary !variables to the D7 core [tokens]. This is a huge change for subscriptions_mail.module. I'm happy to discuss concepts, but the current code is doomed. That's why we're still in ALPHA.
Comment #2
areynolds commentedSo a task-agnostic consumer of queue items would be desirable? I know that every Drupal module that wants to hook into cron runs needs to implement hook_cron, but it seems that most potential sub-modules of Subscriptions would be more-or-less duplicating the subscriptions_mail cron hook to process queue items. Developers would have to figure out a way to make sure the duplicate hooks don't conflict (assuming they would want subscriptions_mail running alongside their custom module), which is a pain.
You're implementing the core token support, not doing a full rewrite, right? What if I floated a patch that didn't touch any of the token-replacement functionality?
By the way, I'm glad you guys have been putting energy into this module. To my knowledge, there aren't any great out-of-the-box notifications solutions for D7; I think Subscriptions is the closest thing. Thanks for the good work and for taking the time to answer my questions about it.
Comment #3
salvisYes. Let's call it subscriptions_notify.module for the sake of discussion, and the module doing mail could continue to be called subscriptions_mail.module. There would need to be two interfaces between the two:
1. a hook_subscriptions_notify() similar to hook_subscriptions_queue(), and
2. a Rules-based interface
subscriptions_mail.module would implement the hook and snatch away all the notifications by default, but it would offer an option to disable the hook. It should implement the actions required to run through Rules with the same functionality.
Yes, but this touches almost everything in subscriptions_mail.module. Also, I've moved _subscriptions_mail_cron() and its helper functions to an .inc file. You'd have to do a lot of work to put this back together...
Feel free to post a patch based on ALPHA4 if you want to try to get some third-party feedback, but I won't look at it until it applies to ALPHA5.
Comment #4
salvis