Skip to content

[Mailer] Fix Error Handling for OhMySMTP Bridge #46603

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

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

paul-oms
Copy link
Contributor

@paul-oms paul-oms commented Jun 6, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix N/A
License MIT
Doc PR symfony/symfony-docs#...

The OhMySMTP Bridge does not handle errors correctly, and throws up an Undefined array key "Message" error when an API error is encountered (e.g. an invalid API key or Internal Server Error). This PR fixes this by showing the full error response.

@carsonbot
Copy link

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

@@ -67,7 +67,7 @@ protected function doSendApi(SentMessage $sentMessage, Email $email, Envelope $e
}

if (200 !== $statusCode) {
throw new HttpTransportException('Unable to send an email: '.$result['Message'].sprintf(' (code %d).', $result['ErrorCode']), $response);
throw new HttpTransportException('Unable to send an email: '.json_encode($result), $response);
Copy link
Member

Choose a reason for hiding this comment

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

Let's go with $response->getContent(false) instead

@nicolas-grekas
Copy link
Member

It looks like you unchecked the "Allow edits from maintainer" box

side note: please squash when pushing the change - or allow us to push on your fork if you prefer we squash on our side 🙏

@paul-oms
Copy link
Contributor Author

paul-oms commented Jun 9, 2022

Thanks! That should cover it? :-)

@@ -67,7 +67,7 @@ protected function doSendApi(SentMessage $sentMessage, Email $email, Envelope $e
}

if (200 !== $statusCode) {
throw new HttpTransportException('Unable to send an email: '.$result['Message'].sprintf(' (code %d).', $result['ErrorCode']), $response);
throw new HttpTransportException('Unable to send an email: '.$response->getContent(false), $response);
Copy link
Member

Choose a reason for hiding this comment

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

Can't we figure out all possibilities instead of "just" dumping the content?

Copy link
Member

Choose a reason for hiding this comment

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

ok, that's the same on line 64, let's do it that way then.

@fabpot
Copy link
Member

fabpot commented Jun 9, 2022

Thank you @paul-oms.

@fabpot fabpot merged commit 2faa354 into symfony:5.4 Jun 9, 2022
This was referenced Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants