Skip to content
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

[6.x] Added ability for postmark transport to throw errors #30799

Merged
merged 2 commits into from Dec 10, 2019

Conversation

@stephan-v
Copy link
Contributor

stephan-v commented Dec 10, 2019

I just spent about an hour trying to debug why the postmark driver was not working. All thrown exceptions are currently not handled and fail silently.

By registering the plugin included in https://github.com/wildbit/swiftmailer-postmark package it will allow for exceptions to be thrown when the Postmark API does not deliver a HTTP 200 response.

A minor adjustment with a huge impact when it comes to debugging and actually detecting issue in production.

Perhaps this file is not the ideal candidate to register the plugin in but you get the idea. I hope this will be open for discussion if this is not the ideal solution.

@GrahamCampbell GrahamCampbell changed the title Added ability for postmark transport to throw errors. [6.x] Added ability for postmark transport to throw errors Dec 10, 2019
@mfn

This comment has been minimized.

Copy link
Contributor

mfn commented Dec 10, 2019

And how to go if someone (implicitly) depends on the old behaviour?

Maybe it should be done via a config (and have the sane default to enable it, exceptions shouldn't be silent in such case).

But maybe this change in behaviour should rather target 7

@stephan-v

This comment has been minimized.

Copy link
Contributor Author

stephan-v commented Dec 10, 2019

And how to go if someone (implicitly) depends on the old behaviour?

Perhaps I don't see the downsides but I can not really think of a way in which this would negatively impact an application.

The only thing this would be doing is providing an exception where there should be one because there is also no other way to currently detect anything going wrong in this process.

Especially when you have set up error reporting in the form of something like Sentry this would be a massive help.

Setting the target to 7 it also being too precautious because I have already wasted time on this black box which shows no errors and I can imagine other have done or will do the same.

@dwightwatson

This comment has been minimized.

Copy link
Contributor

dwightwatson commented Dec 10, 2019

I wonder if this means that previously, email sent on a queued job that failed on the Postmark end wouldn't end up re-running because no exception was thrown - the job would just finish and the email lost. Makes sense to me that an exception would be thrown so that the job would be attempted again.

@taylorotwell taylorotwell merged commit 47d936b into laravel:6.x Dec 10, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Dec 10, 2019

I definitely think throwing an exception is the expected behavior. I didn't know they were being silenced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.