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

Logger refactoring #281

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Logger refactoring #281

wants to merge 13 commits into from

Conversation

JanTvrdik
Copy link
Contributor

  • bug fix? no
  • new feature? yes
  • BC break? no

Implements #280 with lots of hack to maintain compatibility.

@JanTvrdik
Copy link
Contributor Author

JanTvrdik commented Feb 20, 2018

Not sure if the hacks are worth it. It may be better to only add StreamLogger and allow logger instance as alternative to log directory but do not modify the Logger at all.

And possibly release the refactored logger in the future with BC breaks as part of Tracy 3.

) {
call_user_func($this->mailer, $message, implode(', ', (array) $this->email));
}
$reflection = new \ReflectionMethod($this->mailLogger, 'sendEmail');
Copy link
Member

Choose a reason for hiding this comment

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

Why is reflection needed? If MailLogger is not interface, it might have sendEmail public. WDYT?

@f3l1x
Copy link
Member

f3l1x commented Feb 20, 2018

I like this PR, maybe it could be more cleaner (without hacks) for nette 3.0? If we break the API, let's make better one.

@dg dg force-pushed the master branch 4 times, most recently from d13a9ce to 7f24887 Compare February 25, 2018 23:58
@dg dg force-pushed the master branch 8 times, most recently from 427177a to 7ac33a2 Compare March 26, 2018 11:48
@dg dg force-pushed the master branch 15 times, most recently from 4dc8213 to d53b9c3 Compare April 6, 2018 17:58
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.

3 participants