[Mailer] [Mailjet] Use body MessageID instead of X-MJ-Request-GUID #49992
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
carsonbot
changed the title
|
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
|
Will submit updated tests tomorrow, seems the dummy response's |
|
AppVeyor is failing because it does not seem to support 64 bit integers. Is this a requirement for 5.4 or any of the later Symfony versions? |
src/Symfony/Component/Mailer/Bridge/Mailjet/Tests/Transport/MailjetApiTransportTest.php
Outdated
Show resolved
Hide resolved
|
AppVeyor appears to be failing for unrelated reasons now (Form component). If this is all good let me know so I can squash this branch since I've forked into our org. |
src/Symfony/Component/Mailer/Bridge/Mailjet/Transport/MailjetApiTransport.php
Outdated
Show resolved
Hide resolved
|
Thank you @Starfox64. |
…est-GUID (Starfox64) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Mailer] [Mailjet] Use body MessageID instead of X-MJ-Request-GUID | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #48550 | License | MIT | Doc PR | N/A As per #48550, the message ID of the `SentMessage` returned by the `MailjetApiTransport` is the `X-MJ-Request-GUID`. The issue is that this ID is only there for debug purposes by the MailJet engineers and cannot be consumed by the MailJet API to query the message metadata. This change instead uses the `MessageID` attribute returned in the JSON body of the response which can be used in further API requests. As the existing message ID was not usable for anything besides opening support tickets (which the new ID can still be used for), I've decided to submit this PR as a bug fix. A slight limitation however is that MailJet will return multiple messages when sending to multiple recipients, each having their own `MessageID`. This implementation therefore only returns the first `MessageID`, there is an ongoing discussion on #48550 about that behavior but I am submitting this PR as is since it's still a net improvement over the previous ID. Commits ------- 8b92751 [Mailer] [Mailjet] Use body MessageID instead of X-MJ-Request-GUID
As per #48550, the message ID of the
SentMessagereturned by theMailjetApiTransportis theX-MJ-Request-GUID. The issue is that this ID is only there for debug purposes by the MailJet engineers and cannot be consumed by the MailJet API to query the message metadata.This change instead uses the
MessageIDattribute returned in the JSON body of the response which can be used in further API requests.As the existing message ID was not usable for anything besides opening support tickets (which the new ID can still be used for), I've decided to submit this PR as a bug fix.
A slight limitation however is that MailJet will return multiple messages when sending to multiple recipients, each having their own
MessageID. This implementation therefore only returns the firstMessageID, there is an ongoing discussion on #48550 about that behavior but I am submitting this PR as is since it's still a net improvement over the previous ID.