[Mailer] Mailjet - Allow using Reply-To with Mailjet API #38179
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OskarStark
approved these changes
Sep 13, 2020
Contributor
OskarStark
left a comment
OskarStark
left a comment
There was a problem hiding this comment.
Makes sense, but I would prefer an exception instead of the logged warning. 👍🏻
fabpot
requested changes
Sep 14, 2020
| } | ||
| if ($emails = $email->getReplyTo()) { | ||
| if (1 < $length = \count($emails)) { | ||
| $this->getLogger()->warning(sprintf('Mailjet\'s API only supports one Reply-To email, %d given. The first one will be used.', $length)); |
Member
There was a problem hiding this comment.
I would throw an exception instead. I think that having more than one Reply-To address is an edge case and if it happens, it's important to make sure that the dev understands that it is not supported. I understand the interoperability argument, but I think it's better here to fail when something is not supported.
Can you also make the same change in Sendgrid to be consistent?
Contributor
Author
There was a problem hiding this comment.
@fabpot wouldn't adding this change to Sengrid break backward-compatibility as some apps may rely on the current behaviour of the transport ?
Also, if transports are not fully interoperable, should we document it and maybe list transport specificities ?
t-richard
force-pushed
the
mailer-mailjet-reply-to
branch
from
069aa02 to
c8ed052
Compare
Contributor
Author
|
Changed from a logged warning to an exception and adapted tests. |
fabpot
approved these changes
Sep 15, 2020
Member
|
Thank you @t-richard. |
fabpot
force-pushed
the
mailer-mailjet-reply-to
branch
from
c8ed052 to
1ff3e29
Compare
fabpot
added a commit
that referenced
this pull request
Sep 15, 2020
…t-richard)
This PR was squashed before being merged into the 5.2-dev branch.
Discussion
----------
[Mailer] Mailjet - Allow using Reply-To with Mailjet API
| Q | A
| ------------- | ---
| Branch? | master
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | none
| License | MIT
| Doc PR | none
Using this bridge I got an error from Mailjet API given below and this PR fixes this issue.
<details>
<summary>Error from Mailjet</summary>
```json
{
"Messages": [
{
"Status": "error",
"Errors": [
{
"ErrorIdentifier": "xxxx",
"ErrorCode": "send-0011",
"StatusCode": 400,
"ErrorMessage": "Header cannot be customized using the \"Headers\" collection. Please use the dedicated property to set this header.",
"ErrorRelatedTo": [
"Headers[\"Reply-To\"]"
]
}
]
}
]
}
```
</details>
As detailed in [the Mailjet API doc](https://dev.mailjet.com/email/reference/send-emails/index.html#v3_1_post_send), `ReplyTo` is a single email address whereas the `Symfony\Mime\Email` class allows for several emails in `ReplyTo`.
I implemented the [same logic as the Sendgrid bridge](https://github.com/symfony/symfony/blob/0f5ac5dc8f944a719e7d71729c0a63e1cfc9c6b5/src/Symfony/Component/Mailer/Bridge/Sendgrid/Transport/SendgridApiTransport.php#L96) but added logging (I first thought about throwing an exception but this would reduce transport interchangeability).
Note : `Reply-To` is not listed in the ["Forbidden headers" section of Mailjet's documentation](https://dev.mailjet.com/email/guides/send-api-v31/#add-email-headers) but the API response is clear about the expected input.
Commits
-------
1ff3e29 [Mailer] Mailjet - Allow using Reply-To with Mailjet API
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using this bridge I got an error from Mailjet API given below and this PR fixes this issue.
Error from Mailjet
```json { "Messages": [ { "Status": "error", "Errors": [ { "ErrorIdentifier": "xxxx", "ErrorCode": "send-0011", "StatusCode": 400, "ErrorMessage": "Header cannot be customized using the \"Headers\" collection. Please use the dedicated property to set this header.", "ErrorRelatedTo": [ "Headers[\"Reply-To\"]" ] } ] } ] } ```As detailed in the Mailjet API doc,
ReplyTois a single email address whereas theSymfony\Mime\Emailclass allows for several emails inReplyTo.I implemented the same logic as the Sendgrid bridge but added logging (I first thought about throwing an exception but this would reduce transport interchangeability).
Note :
Reply-Tois not listed in the "Forbidden headers" section of Mailjet's documentation but the API response is clear about the expected input.