Skip to content

[Mailer] Fixed mandrill api header structure#37900

Merged
fabpot merged 1 commit intosymfony:4.4from
wulff:mailer/fix-mandrill-headers
Aug 21, 2020
Merged

[Mailer] Fixed mandrill api header structure#37900
fabpot merged 1 commit intosymfony:4.4from
wulff:mailer/fix-mandrill-headers

Conversation

@wulff
Copy link
Copy Markdown
Contributor

@wulff wulff commented Aug 20, 2020

Q A
Branch? 5.1
Bug fix? yes
New feature? no
Deprecations? no
License MIT

When using the Mandrill API transport, it is not possible to set a reply-to address.

How to reproduce
Create a new email and add a reply-to address:

$email = (new Email())
    ->from('from@example.com')
    ->to('to@example.com')
    ->replyTo('replyto@example.com')
    ->subject('subject')
    ->text('text');
$mailer->send($email);

The expected result is a payload which contains the following headers:

"headers": {
    "reply-to": "replyto@example.com"
}

But instead, the getPayload() method produces these headers:

"headers": [
    "reply-to: replyto@example.com"
]

Additional context
See https://mandrillapp.com/api/docs/messages.html.

@stof stof added this to the 4.4 milestone Aug 20, 2020
@dmaicher
Copy link
Copy Markdown
Contributor

dmaicher commented Aug 20, 2020

There is a test failure though:

Testing src/Symfony/Component/Mailer

.......................................................E.......  63 / 237 ( 26%)

............................................................... 126 / 237 ( 53%)

............................................................... 189 / 237 ( 79%)

.SS...........S.........SS......................                237 / 237 (100%)

Time: 1.32 seconds, Memory: 10.00 MB

There was 1 error:

1) Symfony\Component\Mailer\Bridge\Mailchimp\Tests\Transport\MandrillApiTransportTest::testCustomHeader

Undefined offset: 0

/home/travis/build/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Mailchimp/Tests/Transport/MandrillApiTransportTest.php:68

@wulff
Copy link
Copy Markdown
Contributor Author

wulff commented Aug 20, 2020

I have updated the relevant test case, but now the Travis test fails on an unrelated test: https://travis-ci.org/github/symfony/symfony/jobs/719745366#L7547.

dmaicher
dmaicher approved these changes Aug 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other test failures seem unrelated

@dmaicher
Copy link
Copy Markdown
Contributor

dmaicher commented Aug 21, 2020

@wulff can you rebase this to 4.4? There the issue also exists, right?

@fabpot fabpot changed the base branch from 5.1 to 4.4 August 21, 2020 06:31
@fabpot fabpot force-pushed the mailer/fix-mandrill-headers branch from 60c4191 to aeb4c5e Compare August 21, 2020 06:31
@fabpot
Copy link
Copy Markdown
Member

fabpot commented Aug 21, 2020

Thank you @wulff.

@fabpot fabpot merged commit 1f4c616 into symfony:4.4 Aug 21, 2020
fabpot
fabpot reviewed Aug 21, 2020
View reviewed changes
src/Symfony/Component/Mailer/Bridge/Mailchimp/Transport/MandrillApiTransport.php
}

$payload['message']['headers'][] = $name.': '.$header->getBodyAsString();
$payload['message']['headers'][$name] = $header->getBodyAsString();
Copy link
Copy Markdown
Member

@fabpot fabpot Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does Mandrill handle several headers with the same name? Here you will only ever get the last one.

Copy link
Copy Markdown
Contributor Author

@wulff wulff Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, Mandrill only allows one of each header in the headers field.

This was referenced Aug 31, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot left review comments

@stof stof stof approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@jderusse jderusse Awaiting requested review from jderusse

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

+1 more reviewer

@dmaicher dmaicher dmaicher approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug Mailer Status: Needs Review

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@wulff @dmaicher @fabpot @stof @carsonbot