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

Remove LoggerAwareTrait from Email class #2369

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Remove LoggerAwareTrait from Email class #2369

merged 1 commit into from
Dec 6, 2019

Conversation

dafriend
Copy link
Contributor

I would like this considered because the Email class creates a Logger instance when it's instantiated. I'm trying to devise an EmailHandler for the Logger class which would require an instance of the Email class. I sure you can see the endless loop that is formed.

It doesn't seem like removing the LoggerAwareTrait would affect Email much. It only makes one call to the resulting logger object. Switching that call to using the common function log_message seems a good substitute for $this->logger->log...

If this PR is merged I've got a working EmailHandler ready to go.

This requires slight mod to Services::email
Needed to create EmailHandler for Logger system
@MGatner
Copy link
Member

MGatner commented Nov 13, 2019

I think using log_message() directly is actually desirable and consistent with other libraries, as we establish logging early as a service and the helper function will always rely on that. I would like @lonnieezell or @jim-parry to take a look as well but I'm good with this change. This library's days are numbered but it would be good to have it feel at least consistent for release.

@MGatner MGatner self-requested a review November 13, 2019 15:49
@MGatner MGatner merged commit 7c15ad4 into codeigniter4:develop Dec 6, 2019
@dafriend dafriend deleted the Email_remove_loggerTrait branch December 12, 2019 18:42
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