Add logging for debugging purposes

[mpedrummer]

Hey @jakoch - While I was messing around with this today, I ended up adding a logger object (Psr\Log\NullLogger) into the code so I could substitute a real logger, either in production or just while figuring things out.

I figured I'd submit a pull request for you to look at - this isn't perfect by any stretch - I just needed a better way to delve into the code and figure out what was happening.

By default, it's just a NullLogger - the logging just goes nowhere - so it shouldn't be too much of a performance hit. At runtime, you can chose to substitute any PSR-3 compliant logger, such as Monolog.

[jakoch]

I like the idea. Thanks for taking the time to implement it.

Right, the NullLogger doesn't cause a big performance hit, but the file is already quite huge and gets even bigger with logging calls added. Alternatively, we could rename your class to TracRPCDebug and add the psr/log dependency to the require-dev section of composer.json. By providing a separate class with the logging, we could keep the original class free from additional dependencies and null logging calls. But i will have to keep the files in sync, though.

What do you think?

(btw: i tagged v1.0.1 with the latest fixes)

[mpedrummer]

Not really sure - personally, I wouldn't want the added pain of keeping the two versions in sync, particularly since some of the logging that was actually valuable was pretty deep in - it's not like we could just have TracRPCDebug extend TracRPC, and just have it be wrappers that happen to log before doing a parent::x() call.

I understand not wanting to add to the existing class. For my own implementation of it, though, it's being added to a system that already requires Monolog (and the psr/log by extension) so it's no big deal. Having the logging integrated as actual development happens is valuable, too, rather than the potential to have the logging stagnate in a companion class.

Your call - I just wanted to offer it out there, and not have the effort be just for me if there was value to the wider world. I was having some issues getting started this morning, and the existing class error handling wasn't enough to figure out the exact problem (turns out that using the login/rpc URL doesn't work, as the class never sets the content type since it can't differentiate between the json and xml!)

[jakoch]

turns out that using the login/rpc URL doesn't work, as the class never sets the content type since it can't differentiate between the json and xml!

Yes, the URLs must contain jsonrpc or xmlrpc to get the matching content-type set, see https://github.com/jakoch/PHPTracRPC/blob/master/lib/TracRPC.php#L1300 URLs like http://trac.example.com/rpc & http://trac.example.com/login/rpc won't work.

To differentiate between json and xml, we could test, if the URL ends with /rpc and set the content-type according to a passed in parameter $params['content-type'] (https://github.com/jakoch/PHPTracRPC/blob/master/lib/TracRPC.php#L44)

$this->contentType = isset($params['content-type']) ? $params['content-type'] : 'json';

if (strpos($this->tracURL, '/rpc') !== false) {
  curl_setopt($ch, CURLOPT_HTTPHEADER, array('Content-Type: application/'.$this->contentType));
}

[mpedrummer]

From my perspective, it would be better to have an assumed default, perhaps json, and then change if xmlrpc is explicitly indicated. Trac appears to respond based on the content type sent, and so long as the public method signatures remain consistent, I don't see how it matters which is actually used behind the scenes.

Hardly matters, though. It just initially caught me unawares, and wasn't the simplest thing to diagnose.

On Mar 9, 2016, at 18:36, Jens A. Koch notifications@github.com wrote:

turns out that using the login/rpc URL doesn't work, as the class never sets the content type since it can't differentiate between the json and xml!

Yes, the URLs must contain jsonrpc or xmlrpc to get the matching content-type set, see https://github.com/jakoch/PHPTracRPC/blob/master/lib/TracRPC.php#L1300 URLs like http://trac.example.com/rpc & http://trac.example.com/login/rpc won't work. To differentiate between json and xml, we could test, if the URL ends with /rpc and set the content-type according to a passed in parameter $options['content-type'].

— Reply to this email directly or view it on GitHub.

[mpedrummer]

Or leave as-is, and throw an exception if neither is matched.

[mpedrummer]

I think we can abandon this one - with the changes in #17, I think I can accomplish most of this through extending the class.

[jakoch]

Ok... closing.