Problem/Motivation
The recurring_events_registration module sends notification emails to registrants when certain actions or events take place in the system:
- An Event Instance is modified
- An Event Instance is deleted
- An Event Series is modified
- An Event Series is deleted
- An upcoming event is about to occur, so it sends a reminder to all registrants
- etc
For a complete list of the type of notifications, visit: /admin/structure/events/registrant/settings
The module uses the function recurring_events_registration_send_notification($key, $registrant) to attempt to send each email sequentially, inside foreach loops.
See the the lines 221, 253, 277, 299 in this module file: https://git.drupalcode.org/project/recurring_events/-/blob/2.0.0-rc7/mod...
If an Event Instance has a large number of registrants, trying to notify them all in one fell swoop could lead to problematic scenarios, such as PHP timeouts and memory errors.
Steps to reproduce
- Have an Event Instance with a very large number of registrants
- Trigger one of the actions that end up with the sending of email notifications
Proposed resolution
Send email notifications using Drupal Queue operations
Remaining tasks
- Design a solution to incorporate a queue approach for sending email notifications from the
recurring_events_registrationmodule - Determine if there must be some administrative UI to control and configure the queues behavior
- Discuss potential risks and side effects for other subsystems
- Implement the solution
Issue fork recurring_events-3362297
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #4
owenbush commentedI can definitely see how this makes sense for sites with a large number of registrants.
How would you propose to execute the queue? With a cron? This will inevitably create a delay in the sending of some emails, is that acceptable?
The standard drupal cron is controllable by an admin in terms of how often it runs, but it is not granular and there are other admin tasks that run during cron, so there's no guarantee all jobs will execute each time cron runs.
There's ultimate cron available in the contrib space, but I am a little hesitant to add a dependency on it.
There's also the batch api but typically that happens client side, and not all the email sends happen after direct user interaction.
I'm just trying to think through the best approach.
Comment #12
podaroktnx
Comment #13
chrisla commented@podarok @camilo.escobar could you provide some answers to owenbush's points raised in #4? perhaps you have discussed with him privately but as the owner of a site that uses this module, I would like to ensure that this major change will not have repercussions. My site has email notifications that can be sent out on the same day as an event and if they got stuck in the queue and not sent, that could be very bad
Comment #14
camilo.escobar commentedThe changes in the MR imply that those notifications that by design are intended to be sent in bulk to multiple recipients, namely those corresponding to the keys below, are not sent immediately as soon as the triggering action occurs, but are added to a queue instead:
It means they don't go through
recurring_events_registration_send_notification()immediately inside thoseforeachloops, but the Queue Worker will start processing them on the next cron run.In those cases, the line
recurring_events_registration_send_notification($key, $registrant);has been replaced by:
\Drupal::service('recurring_events_registration.notification_service')->addEmailNotificationToQueue($key, $registrant);The new function
addEmailNotificationToQueue($key, $registrant);is in charge of adding the notification to the queue. The function makes sure of invoking the hookhook_recurring_events_registration_send_notification_alter($send_email, $registrant)to allow modules to determine whether the notification should be sent (added to the queue) to the$registrantin question (the same asrecurring_events_registration_send_notification($key, $registrant)does).Also, the function introduces a new alter hook
hook_ recurring_events_registration_message_params_alter($params, $registrant)to allow modules to add data to the$paramsarray that is passed to themail()function. Developers can use the data of the$registrantentity to set these params. Those$paramscan be used later as the$message['params']inhook_mail_alter().Actions that trigger only an email notification to a recipient (registrant) were left intact and continue to use the function
recurring_events_registration_send_notification($key, $registrant), therefore the notification is sent out immediately. Namely:These changes are my initial solution to overcome a big problem we were facing on our website: when an email event notification (e.g., instance_modification_notification) is triggered to a large number of registrants, the system may fail in those iterations and hit time and memory limits depending on the PHP configuration.
How long it takes for the queued notification list to be fully processed depends on three factors:
This is acceptable in our case, since we have a large number of registrants in our events and we are aiming to run the Drupal cron every 5 or 10 minutes.
As I mentioned in the notes in the MR, there is a pending task:
@owenbush @podarok I think completing this to-do is important before merging the MR and releasing a new version, as not all the projects using the module may be interested in the queue implementation and may prefer to continue operating as usual: sending all emails immediately.
I know I owed you this technical background of the approach I implemented, and I was honestly expecting a MR review, some feedback and certain back-and-forth discussion before merging the MR. As I'm seeing this was already included in 2.0.0-rc8, I highly recommend to revert that release or to revert the MR and release a new version without those specific changes, since it is crucial that the to-do above is resolved, so as not to affect the normal operations of projects using the module.
@endless_wander, thanks for pointing out the importance of this case, please stick to a module version prior to 2.0.0-rc8, while I work in the above to-do.
I'll be working this weekend and create a new PR to resolve the pending item, so this solution, that can be very favorable in different cases, can be launched. This is what I will do:
Comment #15
camilo.escobar commentedComment #17
podarokComment #18
chrisla commented@camilo.escobar your work and responsiveness on this is incredibly appreciated. thank you!
Comment #27
podarokthe guy definitely crazy )))
I appreciate.
Gracias
Merged and tagged