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

Catch Email Exceptions #1190

Merged
merged 4 commits into from
Sep 5, 2018

Conversation

puschie286
Copy link
Contributor

catch exceptions that are thrown in the send mail function
-> caused by wrong/invalid mail config

Signed-off-by: Christoph Potas [email protected]

~ fix inverted $method result

Signed-off-by: Christoph Potas <[email protected]>
@lonnieezell
Copy link
Member

The problem with this is that it hides that actual exception that happened from the developer. I believe that exception will only be thrown if the server isn't setup correctly, right, which is something the developer should know about.

I haven't had a chance to look through the causes in depth, but if it's something that would only crop up in development, or when testing a new server, it should throw it and fail very loudly so the developer knows. If it's something that can crop up during normal usage, then it should at least be logged so the developer can find the issue later.

@puschie286
Copy link
Contributor Author

puschie286 commented Aug 30, 2018 via email

@lonnieezell
Copy link
Member

Ok, cool. As long as we log it, then, I think it should be fine.

Signed-off-by: Christoph Potas <[email protected]>
@lonnieezell
Copy link
Member

Thanks for that addition. I should have been clearer, though. Instead of using Services within the class, I would rather use the LoggerAwareTrait and add the logger instance in the constructor (in system/Config/Services.php)

See the Session class and it's service method for an example.

@lonnieezell
Copy link
Member

Thanks for doing that. Merging.

@lonnieezell lonnieezell merged commit b80acf2 into codeigniter4:develop Sep 5, 2018
@puschie286 puschie286 deleted the CatchEmailExceptions branch September 11, 2018 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants