Can't Control Log Level Easily Across Multiple Requests (Singleton problem)

[HeathNaylor]

After stepping through the code for Log.php I am seeing some code smell.

<?php
namespace net\authorize\util;

use net\authorize\util\ANetSensitiveFields;

define ("ANET_LOG_FILES_APPEND",true);

define("ANET_LOG_DEBUG_PREFIX","DEBUG");
define("ANET_LOG_INFO_PREFIX","INFO");
define("ANET_LOG_WARN_PREFIX","WARN");
define("ANET_LOG_ERROR_PREFIX","ERROR");

//log levels
define('ANET_LOG_DEBUG',1);
define("ANET_LOG_INFO",2);
define("ANET_LOG_WARN",3);
define("ANET_LOG_ERROR",4);

//set level
define("ANET_LOG_LEVEL",ANET_LOG_DEBUG);

First off, these are global constants in the application. As such if I attempt to set ANET_LOG_LEVEL during bootstrapping of the applications I will end up throwing a PHP noitce for the constant already being defined when the class is included later in the application.

As such all of these global variables should be member properties and a method of assigning a non-default value would need to be implemented.

Am I missing something, or is there not currently a way to define log level granularity?

[HeathNaylor]

Just a follow up, this looks to be fixed as of tag 1.9.

[mahngiel]

Fixed on 1.9 or not, I'm not even seeing how to manipulate the log level. There are no accessors exposed.

[mahngiel]

@brianmc

Can we get a response on this report and/or PR? It would do a lot for confidence from our developers that this SDK is taken seriously.

[HeathNaylor]

@brianmc Can we please have some response or understanding if this will get merged in at some point? Maintaining our own fork is less than ideal.

[ashtru]

Apologies for no updates to this thread in a long time.

1. I see a lot of controller classes modified in this PR. Controller classes are auto-generated for each of the APIs, so changing them would require us to change our controller generation scripts. Can you please share use cases for the logger injection?
2. Other than the log injection changes, you are trying to provide an accessor for log level. For setting the log level, the setLogLevel method can be used. Can you reply if that is sufficient?

[HeathNaylor]

Thank you for the reply, happy to see progress on this issue.

Firstly, the goal of logger injection is testability and flexibility, . Right now you have a LogFactory, it acts as more of a singleton than it does a factory. The goal of a factory should be to produce new instances of a class depending on what is passed into the factory. Right now we get a single instance back after the singleton instance was created. So now, in order to modify my Log object in any way, I have to do the following:

$log = \net\authorize\util\LogFactory::getLog('this parameter is never used');
$log->setLogLevel(4);

This would modify the singleton in a global scope and could have unintended consequences as we have now potentially changed state that could be unexpected elsewhere. The logger is now global, so if I wanted granularity difference across different method calls, I would have to do state modification to that singleton just before those methods each time, rather than having a logger or two that I inject depending on the needs at the time. This is a contrived example just to show the limitations of the singleton pattern with loggers.

Secondly, it is interesting to hear that these controllers are auto-generated. I think with some simple refactoring, we can eliminate the need for those auto-generated classes all together.

At this point, we are talking about a major bump for the SDK as these would result in breaking changes for consumers of the SDK.

The first step would be to eliminate all of the auto-generated classes as it is an anti-pattern. We would then convert the ApiOperationBase from an abstract class to a concrete class and rename it ApiOperation or ApiOperationController. We could then adopt a polymorphic implementation as follows:

public function __construct(\net\authorize\api\contract\v1\AnetApiRequestType $request, \net\authorize\api\contract\v1\ANetApiResponseTypeInterface $responseType, Log $logger = null)

This would involve switching to the paradigm of coding to an interface as ANetApiResponseTypeInterface does not currently exist. It also seems to me that the class ANetApiResponseType should be abstract, as it does not seem to warrant being implemented directly.

Now inside of the ApiOperationController class, that is now no longer abstract, we can change the following:

• $this->apiResponseType = $responseType;

• $this->apiResponseType = $responseType::class;

Honestly, this refactor could go much deeper and across all of this library, so I don't want to tangent too much from the original problem.

I am not sure you understand my intention with regards to the method of using define("ANET_LOG_LEVEL",ANET_LOG_DEBUG);. I am simply stating this as a poor practice. Any time you use define() you are bringing a named constant into global scope. There should be a very good reason to ever do this. As the log level, and really all define()s in this class can be neatly encapsulated within the class, I am proposing that we do so. This way you clean up your global scope (an easy win) and then you have the benefit of being able to create a more human friendly API. For example:

$log = new \net\authorize\util\Log();
$log->setLogLevel(Log::ANET_LOG_ERROR);

or

$log = new \net\authorize\util\Log(Log::ANET_LOG_ERROR);
$log->setDebugLevel();

or even

$log->setLevelToError();

and the Log class could do the following:

public function setLevelToError()
{
    $this->logLevel = self::ANET_LOG_ERROR;
}

Conclusion: I think we would need to define the amount of effort desired here. I could go all in and refactor this library into a new major version, removing some anti-patterns and bringing in some best practices such as PSR adherence for the logger, and coding to an interface throughout. More simply, we could limit the scope to keeping this singleton (that is called LogFactory) and just allow injection as well to avoid the singleton for those who want to. It doesn't introduce a major bump and allows the testing and flexibility desired.

[alexsds]

LogFactory is singleton. You can use it for set log level and log file.

private function setLogger($class)
{
   if ($this->options->getLogFile()) {
        $log = \net\authorize\util\LogFactory::getLog($class);
        if ($log) {
           $log->setLogLevel($this->options->getLogLevel());
           $log->setLogFile($this->options->getLogFile());
        }
   }
}

Exemple:

use net\authorize\api\controller as AnetController;
...
...
$this->setLogger(AnetController\GetAUJobDetailsController::class);
$controller = new AnetController\GetAUJobDetailsController($request);

[HeathNaylor]

Yup, I am aware of that. It is not ideal as a singleton though, in case for different requests I would want different log levels.

I will rename this issue as I don't think that it is helpful as it stands.

[HeathNaylor]

@gnongsie is there plans for this to become a feature and merged into a release?