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

Fuel logger logic moved to a handler #3

Open
sagikazarmark opened this issue Oct 24, 2014 · 9 comments
Open

Fuel logger logic moved to a handler #3

sagikazarmark opened this issue Oct 24, 2014 · 9 comments

Comments

@sagikazarmark
Copy link
Contributor

I would like to see the whole logger file creation logic in a FuelHandler for easy reuse.

Opinions?

@WanWizard
Copy link
Member

What do you mean exactly? Logging is done by Monolog?

@sagikazarmark
Copy link
Contributor Author

The logic in the log config should IMO go into a Monolog Handler.

So you can do $logger->addHandler(new FuelComponentHandler('path/to/log'))

@WanWizard
Copy link
Member

Ah, you mean the test code in the config/log file? Yes, absolutely, it's been put there to have some logging, it definitely has to move elsewhere.

@sagikazarmark
Copy link
Contributor Author

Yes, I mean that. I suggested a handler for easy reuse.

@WanWizard
Copy link
Member

I agree, but hadn't had the time to do it. So if you do, be my guest. ;-)

@sagikazarmark
Copy link
Contributor Author

I usually put extensions like this in the original library's namespace, so this would be Monolog\Handler\FuelHandler. Is it OK for you? If not, where would you like to see it?

@WanWizard
Copy link
Member

Haven't thought about it to be honest. The potential issue I can see is that people using this may assume it's part of the Monolog package, based on the namespace. I probably would turn it around, and use \Fuel\Foundation\MonologHandler or something (or move it into a Log namespace if we intend to have multiple log handlers).

I think we need to good discussion about bootstrapping the framework. Due to the static nature in v1, using config files returning array's of scalar values was fine. But v2 is a lot different, and perhaps we have to revise this, and go for more "active" configuration, like is now done for Environment and Log (and to a lesser extend for Routes, I think that needs changing too).

I have to say I quite like the closure based config, it provides a lot more flexibility to use code to provide configuration values, without having to go through the complexities of class extensions...

@sagikazarmark
Copy link
Contributor Author

My concerns with the closure config is that you can only make it working with a php config file. Furthermore if you do get an a specific config item by accident, based on the closure's argument you get an error, which you can't handle (you cant control what people do with closures, so its better not relying on it). But I also think it is easy and some timee I can't avoid using it.

In production env (where you possibly use opcode cache) it can be a pain using php configs (hence the stupid cache invalidation).

@WanWizard
Copy link
Member

hmm... yes, that is a very valid point. One more reason to have this discussion. ;-)

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

No branches or pull requests

2 participants