-
Notifications
You must be signed in to change notification settings - Fork 11.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[9.x] Ability to attach an array of files in MailMessage #43080
Conversation
Passing a filepath or an array of filepaths will now attach one or multiple files in mail.
Although I can absolutely get behind the intended functionality of this PR and think it is a nice addition to the framework, I don't feel this is the best way to handle it. Take the following example... $mailable->attach(['forge.png', 'vapor.jpg'], [
'as' => 'logo',
'mime' => 'image/jpg',
]); This means that you cannot assign intended names / mime types to the individual elements in the array. Instead of trying to pipe this all through the existing method, I would personally prefer to see an $mailable->attachMany([
'forge.png' => ['as' => 'forge-logo', 'mime' => 'image/png'],
'vapor.jpg' => ['as' => 'vapor-logo', 'mime' => 'image/jpeg'],
'envoyer.svg', // just a filename
'nova.svg', // just a filename
$echo, // instance of attachable
$horizon, // instance of attachable
]); If this is wanted for the framework, I would be happy to assist @Illusionist3886 getting this one in shape and tested. |
@tmacdonald |
@Illusionist3886 would you like me to jump into this and make the necessary changes? Or did you want to have a go at it first? |
Let me try a few days more. I can surely use your help about writing tests. |
No worries at all. Ping me and I can jump in, otherwise I'll check back with you in a few days. |
The method will take filename, array of filenames or attachable object as input and modify it to pass through current attach method.
@timacdonald
We can pass any of this and also the attachable object too. Can you confirm and write some tests for this, please? |
Awesome stuff @Illusionist3886 Gonna take a look at this today and then will get some tests written up and sent to your branch. |
@Illusionist3886 I've just pushed some tests + some formatting changes. I've removed the options array based on feedback from @BrandonSurowiec Would love to hear your thoughts after seeing my adjustments. If you are happy with this now I think you can open it back up for review 😀 I'm not sure if we need to consider things I've tested this new function against a live mail server to confirm that the attachments are being attached to emails as expected. |
Thanks for the support |
No troubles at all. Always excited to see new contributors making things happen in the framework!
I don't believe we should add an additional type check. We have specified the type in the docblock, which is the Laravel convention. |
So, I'm just curious, can you not just call |
I could do for Mailable's but when using MailMessage from Notification, I couldn't find a cleaner way myself specially when items are dynamic. |
Sorry - I don't follow. You can't just call |
Yes, we can definitely
But if we have unknown amount of items to be attached we need to do a
But problem comes when
These are all for So, I thought why not create a |
Passing a filepath or an array of filepaths will now attach one or multiple files in mail.
Then in build method