Fixed #18907 - Change default queue, make all notifications queable. - #19126
Fixed #18907 - Change default queue, make all notifications queable.#19126uberbrady wants to merge 4 commits into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| Complexity | 3 medium |
🟢 Metrics 362 complexity
Metric Results Complexity 362
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@bzeus can you test out this branch and see if notifications work correctly? Dankeshein! |
|
@uberbrady Can you take a little time and test this please? I'd love it if we can ship this in the next release. |
|
Also looks like we have some mail-related failing tests here |
|
Yeah, so apparently moving some of those emails to the Queue is actually breaking some stuff - we had one Mailer that was using But to Queue the emails so that the "background" runner can actually run them, it needs to serialize and deserialize the Mailer. And that, apparently, only works right if you explicitly list your properties. I've got it so that the tests pass, but there are still plenty of warnings about unsent email in my laravel.log file, so this is not quite ready for prime time, just yet. And I shouldn't have had to disable email sending, so I marked those with 'FIXME's so that I can handle them at the same time that I handle everything else. So I'll keep this in draft until that is done. |
3 participants
Fixes #18907
Changes the default driver to 'background' which runs after the page has rendered. Doesn't require any DB tables, but if someothing in the message-sending system fails, the remaining messages will probably not get sent.
This is written without any tests to handle anything or even any physical testing. Just trying to show the way I would be going (and possibly, how we've gone) if we adopt this approach.