search

fuelphp / demo-component

FuelPHP Framework - Demo application
9 stars 7 forks source link

Fuel logger logic moved to a handler #3

Open sagikazarmark opened 10 years ago

sagikazarmark commented 10 years ago

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

Opinions?

WanWizard commented 10 years ago

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

sagikazarmark commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

WanWizard commented 10 years ago

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

sagikazarmark commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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