caponica / AmazonMwsComplete

Name-spaced wrappers for the full set of Amazon MWS client libraries
56 stars 53 forks source link

Extract echo and print_r logs to use a PSR-3 logger interface. #23

Closed codebymikey closed 6 years ago

codebymikey commented 6 years ago

The logger can be passed into the MwsClientPool and MwsProductClientPack constructors.

Classes like CaponicaClientPack and ThrottledRequestLogCollection will use the global LoggerService singleton for logging as the interface can't be easily passed through to them.

This change should be backwards compatible.

Addresses #21

caponica commented 6 years ago

I have made one change here - if no logger is defined I think LoggerService should echo the message (i.e. the old behaviour). Let me know if this is a sensible change (in which case I'll merge psr_logger into master) or if there's a better way to keep old code working.

Also, we should update the README.md in the project root to explain how to define a Logger (or at least point people in the direction of third party documentation for this).

codebymikey commented 6 years ago

I personally don't think it's a good idea to keep the old echo functionality enabled by default, it ends up forcing users to add a logger interface in order to hide the echos even if they don't actually want logging enabled. API wrappers like this library should only return the expected response; logging is just an extra and shouldn't be crucial to the core of the library.

If a user does want to keep the echo functionality, then they can create a simple echo logger like this and attach it once using LoggerService::setDefaultLogger(new EchoLogger()):

class EchoLogger extends AbstractLogger
{
    /**
     * Logs with an arbitrary level.
     *
     * @param mixed  $level
     * @param string $message
     * @param array  $context
     *
     * @return void
     */
    public function log($level, $message, array $context = array())
    {
        echo "$message\n";
    }
}

Can you let me know your thoughts on this?

I'll try make some time most likely in the weekend to update the README accordingly.

caponica commented 6 years ago

I agree... I'm just wary of changing the existing functionality in a way that might surprise people that are using it! I think a short entry on the README explaining how easy it is to get the echo functionality back (if you want it) would be enough to help them transition.

caponica commented 6 years ago

OK - I've added some documentation, removed the "echo by default" code and will merge the code into master.