beyondcode / laravel-mailbox

Catch incoming emails in your Laravel application
https://beyondco.de/docs/laravel-mailbox/getting-started/introduction
MIT License
1.04k stars 127 forks source link

Queue getQueuableId issue #17

Open davidrushton opened 5 years ago

davidrushton commented 5 years ago

Thanks for this package.

I was running into this exception

BeyondCode\Mailbox\InboundEmail::id must return a relationship instance.
[stacktrace]
#0 vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(397): Illuminate\\Database\\Eloquent\\Model->getRelationshipFromMethod('id')
#1 vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(327): Illuminate\\Database\\Eloquent\\Model->getRelationValue('id')
#2 vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1388): Illuminate\\Database\\Eloquent\\Model->getAttribute('id')
#3 vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1398): Illuminate\\Database\\Eloquent\\Model->getKey()
#4 vendor/laravel/framework/src/Illuminate/Queue/SerializesAndRestoresModelIdentifiers.php(32): Illuminate\\Database\\Eloquent\\Model->getQueueableId()
#5 vendor/laravel/framework/src/Illuminate/Queue/SerializesModels.php(23): App\\Jobs\\ProcessInboundEmail->getSerializedPropertyValue(Object(BeyondCode\\Mailbox\\InboundEmail))

When running

Mailbox::catchAll(function (InboundEmail $email) {
    dispatch(new ProcessInboundEmail($email));
});

I see this is because the email model is only saved at the end of any Mailbox routes, so there is no id to serialize for queued jobs.

I can fix this by updating the queued job to the following, but wanted to check if you'd accept a PR to match Mailbox routes first then call storeEmail before running any routes?

Mailbox::catchAll(function (InboundEmail $email) {
    $email->save();
    dispatch(new ProcessInboundEmail($email));
});

Thanks :)

oliverbj commented 5 years ago

@davidrushton how does adding the $email->save() line before the dispatching works? If I do this, it will be inserted into the database, but not by the queue/job, but just normally.

davidrushton commented 5 years ago

@oliverbj The default behaviour of the package is to run all routes before saving the email - https://github.com/beyondcode/laravel-mailbox/blob/master/src/Routing/Router.php#L97.

Because Laravel serializes all Eloquent models and then re-fetches from the database when running queue jobs, this causes an exception - the e-mail wasn't saved so there's no id to serialize/fetch.

oliverbj commented 5 years ago

Ah OK, I see. It would make sense that we should be able to overwrite the save method - or at least call it ourself, based on conditions.

There might be situations where the incoming email is being redirected to the correct mailbox/class, but should not be saved if some other user specific conditions are met.

jhayg12 commented 2 years ago

@davidrushton @oliverbj I also have the same issue how did you manage to solve this? Thanks

https://github.com/beyondcode/laravel-mailbox/issues/102

davidrushton commented 2 years ago

@jhayg12

From memory, I think I just solved this in the project by adding in the $email->save() line:

Mailbox::catchAll(function (InboundEmail $email) {
    $email->save();
    dispatch(new ProcessInboundEmail($email));
});

Hope that helps!