Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Mailer] Make sure Http TransportException is not leaking#42184
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Mailer] Make sure Http TransportException is not leaking#42184
Uh oh!
There was an error while loading.Please reload this page.
Conversation
82a061e tof888d56CompareOskarStark commentedJul 19, 2021
One minor question, why not against |
src/Symfony/Component/Mailer/Bridge/Mailgun/Transport/MailgunApiTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| thrownewHttpTransportException('Could not reach the remote Mailgun server.',$response,0,$e); | ||
| } | ||
| if (200 !==$response->getStatusCode()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd wrap the call togetStatusCode andtoArray in the same try/catch (same everywhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure. I will but why? This will never throwTransportExceptionInterface if I first get the body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd say to make things easy to reason about and consistent (some places were calling getStatusCode before toArray/getContent)
f888d56 to1681bc1CompareNyholm commentedJul 19, 2021
The PR is updated. I've targeted 4.4 and made sure we get both status code and body inside the try/catch |
nicolas-grekas commentedJul 20, 2021
Thank you@Nyholm. |
Uh oh!
There was an error while loading.Please reload this page.
We dont want the mailer to throw exceptions from the http-client component. This will make sure to catch such exceptions and rethrow the proper HttpTransportException from the Mailer component.