apinstein / phocoa

PHOCOA php framework
http://phocoa.com
15 stars 3 forks source link

support delegating log calls #61

Closed parisholley closed 7 years ago

parisholley commented 7 years ago

Allow web application delegate to define own object for handling logs, assumes there is no distinction between file or non-file, and uses ident/file interchangeably as a key (ie: consumer can detect if key contains ".log" and do something differently)


This change is Reviewable

apinstein commented 7 years ago

@parisholley not sure if this is helpful, but there is a monolog branch of phocoa I started a while back: https://github.com/apinstein/phocoa/tree/monolog to try to a) get out of PEAR dependency

parisholley commented 7 years ago

@apinstein Not sure there is a strong need at this point to introducing a logging library in phocoa. The pattern in this pull alows phocoa to be part of a broader eco-system of components without having to bootstrap so many things (such as logging), on its own.

apinstein commented 7 years ago

Totally support that. Just sharing legacy context.


Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.


phocoa/framework/WFLog.php, line 72 at r1 (raw file):

    * Allow web application to define its own logging facade instead of relying on PEAR module.
    */
    public static function getLogger($channel)

FYI - historically I'd put all functions that had this pattern (detect method on web app delegate) in the WFWebApplication class to hide this concern behind and interface.

that would allow you to consolidate the Log bootstrapping in WFWebApplication as well, and might be easier if someone has to follow things in the future.

But since that is a small likelihood, I will leave it to you to decide.


Comments from Reviewable

parisholley commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


phocoa/framework/WFLog.php, line 72 at r1 (raw file):

Previously, apinstein (Alan Pinstein) wrote…
FYI - historically I'd put all functions that had this pattern (detect method on web app delegate) in the WFWebApplication class to hide this concern behind and interface. that would allow you to consolidate the Log bootstrapping in WFWebApplication as well, and might be easier if someone has to follow things in the future. But since that is a small likelihood, I will leave it to you to decide.

i think that is fair a change, may as well be consistent :)


Comments from Reviewable

apinstein commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


phocoa/framework/WFLog.php, line 72 at r1 (raw file):

Previously, parisholley (Paris Holley) wrote…
i think that is fair a change, may as well be consistent :)

OK thanks


Comments from Reviewable

apinstein commented 7 years ago

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 2 unresolved discussions.


phocoa/framework/WFLog.php, line 38 at r2 (raw file):

    public static function log($message, $ident = 'general', $level = PEAR_LOG_DEBUG)
    {
        $logger = WFWebApplication::logger();

something doesn't look right about this ... not seeing where it bootstraps the sharedWebApplication... does this even work?


phocoa/framework/WFLog.php, line 59 at r2 (raw file):

    public static function logToFile($fileName, $message)
    {
        $logger = WFWebApplication::logger();

same here


Comments from Reviewable

parisholley commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.


phocoa/framework/WFLog.php, line 38 at r2 (raw file):

Previously, apinstein (Alan Pinstein) wrote…
something doesn't look right about this ... not seeing where it bootstraps the sharedWebApplication... does this even work?

hadn't tested the commit, last one is working :)


phocoa/framework/WFLog.php, line 59 at r2 (raw file):

Previously, apinstein (Alan Pinstein) wrote…
same here

hadn't tested the commit, last one is working :)


Comments from Reviewable

apinstein commented 7 years ago
:lgtm:

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable