Skip to content
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

Mail with attachment CSV = bad content-type #29847

Closed
yvestan opened this issue Sep 3, 2019 · 17 comments
Closed

Mail with attachment CSV = bad content-type #29847

yvestan opened this issue Sep 3, 2019 · 17 comments
Labels

Comments

@yvestan
Copy link

yvestan commented Sep 3, 2019

  • Laravel Version: 5.8.31
  • PHP Version: 7.2.21
  • Database Driver & Version: PostgreSQL

Description:

Hello,

I send specific email : plain text, with body totally empty and CSV file on attachment

    public function build()
    {

        return $this->from(env('MAIL_SENDER'))
                    ->bcc(str_replace('@', '+send@', env('MAIL_SENDER')))
                    ->subject($this->cmd_acces->filename)
                    ->text('emails.admin.ftth.cmd_acces.cmd_acces_mailer') // empty blade file
                    ->attach($this->cmd_acces->csvfile, [
                        'as' => $this->cmd_acces->filename,
                        'mime' => 'text/csv',
                    ]);

    }

No problem with Laravel 5.5 but i've just upgraded to Laravel 5.8 and there's a little bug :

The content-type should by "multipart/mixed" but it is now "plain/text"

Laravel 5.5 => OK

Capture du 2019-09-03 17-38-29

Laravel 5.8 => NOK => bad content-type and base64 on body message

Capture du 2019-09-03 17-39-23

I've change Illuminate/Mail/Mailer.php (https://github.com/laravel/framework/blob/6.x/src/Illuminate/Mail/Mailer.php#L332) and that's work fine

Actual

$message->$method($this->renderView($plain, $data), 'text/plain');

My change : just deleted "text/plain" parameter

$message->$method($this->renderView($plain, $data));

I suppose a bug ?

Steps To Reproduce:

send email : plain text, with body totally empty and CSV file on attachment

@driesvints
Copy link
Member

This has been the same for 3 years and no one has reported anything like this until now. Are you sure this is related to the framework?

@Sladewill
Copy link

Could have sworn I've seen this before when raw mail is used instead of Html due a change in 5.6.

@yvestan
Copy link
Author

yvestan commented Sep 4, 2019

@driesvints I found the difference between 5.8 and 5.5

https://github.com/laravel/framework/blob/5.8/src/Illuminate/Mail/Mailer.php#L242

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Mail/Mailer.php#L220

On 5.5, call_user_func is after addContent

        $this->addContent($message, $view, $plain, $raw, $data);

        call_user_func($callback, $message);

On 5.8 call_user_func is before addContent

        call_user_func($callback, $message);

        $this->addContent($message, $view, $plain, $raw, $data);

On 5.8, if I move call_user_func like 5.5, that's work fine...

@brunogaspar
Copy link
Contributor

Related pull request #22995

@driesvints
Copy link
Member

Hmm this is a tricky one. What would the implications be of reverting that PR?

@arthurvanpasselia
Copy link

I used the raw functionality and that made it work without changing the vendor files:

Mail::raw('email.generic', function($m) {
$m->from('[email protected]', 'Your name')
->subject(' CSV export')
->attach($path, [
                    'as' => $file,
                    'mime' => 'text/csv'
                ]);

@nishijain127
Copy link

Besides using Mail try some other Method to Send Email
=> Create Jobs
=> Create Events , Mail and Listener
=> They are running background side
=> And You can attach multiple file by binding in Array
=> Use this link, May be this can help
https://laravel.com/docs/6.x/events

@JavierTibi
Copy link

I have the same issue with Laravel 6.0.4

$sendMail->attachData($file, $name, [
                    'mime' => 'application/pdf',
                ]);

I receive the plain text instead of the file

--_=_swift_1579548528_6ed8fb3c26cfdc6b05db6e4bc17f28b4_=_
Content-Type: application/pdf; name=qrcodes_from_1094_to_1094.pdf
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=qrcodes_from_1094_to_1094.pdf

@StratusBase
Copy link

StratusBase commented Feb 29, 2020

Same issue as above, in Laravel 5.6 after upgrade...

//  Init Mailer - Empty Email w/ Attachment
Mail::raw('', function ($message) use($filename, $subject, $recipients, $pdf, $dry_run) {

                // Set Subject & Recipients
                $message->subject($subject)->to($recipients);

                // Attach PDF
                $message->attachData($pdf->output(), "{$filename}.pdf");
});

@StratusBase
Copy link

@driesvints I found the difference between 5.8 and 5.5

https://github.com/laravel/framework/blob/5.8/src/Illuminate/Mail/Mailer.php#L242

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Mail/Mailer.php#L220

On 5.5, call_user_func is after addContent

        $this->addContent($message, $view, $plain, $raw, $data);

        call_user_func($callback, $message);

On 5.8 call_user_func is before addContent

        call_user_func($callback, $message);

        $this->addContent($message, $view, $plain, $raw, $data);

On 5.8, if I move call_user_func like 5.5, that's work fine...

I did this to solve my issue after I upgraded from 5.5 to 5.6, this is ridiculous.... Please fix this!!

@Alexander--
Copy link
Contributor

I am the guy, who made 0cd8899 and ultimately broke everyone's email in Laravel 5.6

In retrospect the change in #22995 was a bad idea. But I had my reasons!

Anyway, content type like text/plain; boundary... is an obvious nonsense, which makes me think that this is a bug in SwiftMailer.

Internally SwiftMailer represents both primary message and all it's parts as subclasses of Swift_Mime_SimpleMimeEntity and decides on hierarchy (and mime-types of parts) via some weird logic. So perhaps invoking SwiftMessage#attach() before SwiftMessage#setBody() ­causes an attachment to be miscategorised as primary part, which subsequently has it's mime type set to text/plain? Just a wild guess.

@yvestan
Copy link
Author

yvestan commented Mar 4, 2020

Thank you for explanation @Alexander-- Do you plan to revert to the previous version?

@Alexander--
Copy link
Contributor

Alexander-- commented Mar 6, 2020

I am not a Laravel developer — just a guy, who tried to report bug in Laravel and was told to make necessary fixes myself. But personally I wouldn't revert. The change was breaking, but reverting it now will also break any code, that was written to depend on the current behaviour.

If we look back at the first public Git commit of Laravel, the callback used to be called before addContent() up to Laravel 5.1 (in 2015).

Then @arunpoudel moved it with justification being...

This allows... setting the encoding of the email

Then I moved it back in Laravel 5.6 (in 2018) with exact same justification.

We really should stop moving this callback around and start fixing actual bugs.

It seems like both attach() and attachData() immediately invoke attach() on underlying swift message. Afterwards addContent invokes setBody on a message, that already has some parts (e.g. attachments). This probably messes up some internal state of SwiftMessage.

Could we postpone composing actual SwiftMessage until the email is just about to be sent? Current behaviour of immediately invoking attach() goes back to the first version of Laravel, so changing it may break something else, but in my opinion it is worth a try.

@driesvints
Copy link
Member

Sent in a fix for this here: #32272

We now basically prevent the header from being modified if it's already been set on the message.

@driesvints driesvints reopened this Apr 7, 2020
@driesvints
Copy link
Member

We had to revert the fix because it was apparently causing some issues. We'll need to find a different way.

@taylorotwell
Copy link
Member

Fixed by using a single space instead of a truly empty string... 🤷

image

@taylorotwell
Copy link
Member

0557622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants