phpxmlrpc
phpxmlrpc copied to clipboard
gggeek
Switch to PSR Logger and allow DI patterns
The logger used here is impossible to replace/override, because of the way it's implemented
Logger::instance()->debugMessage($message)
also it's debug = 2 mode just echoes/prints debug message to the output, which has not much use when phpxmlrpc is used for example in the queue consumer.
replacing it with PSR Logger would be much more flexible and allow users to use mainstream php loggers like Monolog.
this would also eliminate the PhpXmlRpc\Client::$debug variable by using proper PSR Logger loglevel methods.
You are right that atm it is not always easy or even possible to swap out bits of the library, such as the logger, with alternative implementations. The reason is simple: the library design predates by a wide margin the spread of DI patterns in the php ecosystem, or for that matter, PSR/Log.
In the latest release however, some work has already been done in order to introduce DI patterns, that would allow this.
Specifically, the 'top-level' classes all implement setLogger and getLogger methods.
Which means that you can already either subclass PhpXmlRpc\Client and take over getLogger, or keep the existing Client class and add in your code a setLogger() call to inject a different logger to your client instances. Of course it is up to you to create a wrapper that implements the phpxmlrpc "logger api" on top of a psr logger.
I noticed that classes Charset, Http and XMLParser do not yet follow this DI pattern, so there is room for improvement there.
On the other hand, the question also stands: when injecting a logger into, say, the client, should the client propagate it its Requests? And should the Requests propagate it to their XMLParser?
Last words of feedback: this library prizes stability and backwards compatibility over anything else. So, while I am willing to make it easy or at least possible to use psr\log and DI patterns, this will not be done at the cost of BC, at least not until there is a new major version in the pipe.
WDYT?
ps. DOH! I just realized that the code which makes it possible to swap out the logger is only in the master branch. Would you be willing to test if it fits your requirements? The current plan is to release it for version 4.6.0 - but I also would like to push in a few more new features in that version, and since these are busy times for me, it might not be tagged and released for a couple of months...
oh sorry, i wanted to reply but i forgot.
yes i am looking through it. i can make it PSR-3 compliant, but unfortunately, it won't be without breaking BC. for example the setLogger, getLogger, $logger are static. this should be replaced with proper DI. so releasing in another version with BC announcement is the way.
when injecting a logger into, say, the client, should the client propagate it its Requests? And should the Requests propagate it to their XMLParser?
absolutely yes. when instantiating/servicing the client, you inject logger once and expect everything gets logged into that one instance.
i will dig into it during upcoming days.
... i can make it PSR-3 compliant, but unfortunately, it won't be without breaking BC. for example the
setLogger,getLogger,$loggerare static. this should be replaced with proper DI
Care to explain how you are instantiating your Client objects? Are you making it a Service (singleton) via your framework's config, or are you creating a new instance manually?
i am making the client a service (multiple instances actually). but in both service & manual instance, i assume that Client and Server are the "entry points" which should get injected the logger and propagate it into other classes like Value, Request, Response.
looking at it, some classes which are used as "static property singletons" like the logger should also be serviced and injected, in particular Encoder and perhaps everything in Helper\.
there are also files in lib/ which make static calls mostly to Server, but they are all marked as "deprecated". so perhaps get rid of them toghether with most (ideally, all) static accesses.
Quoting my initial response:
Last words of feedback: this library prizes stability and backwards compatibility over anything else. So, while I am willing to make it easy or at least possible to use psr\log and DI patterns, this will not be done at the cost of BC, at least not until there is a new major version in the pipe.
I'll take a look at what can be done for your scenario, but don't hold your breath for breaking changes and removal of old code...
i got your point there, but maybe it's time for new major version then. i know this library started long time ago, but using it with today's frameworks by their means is quite cumbersome. it's a useful lib and imho deserves some refactoring, and to maintain BC keep it in two branches, legacy plus current one.
You are certainly not wrong with that suggestion. However there a few other things worth pointing out:
- I am maintaining an increasingly large number of open source projects. The time I can afford to dedicate to them is not infinite. Maintaining one extra branch with a mostly different codebase is not a trivial endeavour;
- the "modernize the lib" approach is quite a slippery slope, as there is a ton of things that I would change if shackled from the chains of BC. I even started doing that a little while ago! Just look at all the "phpxmlrpc-ng" repos in my github... Sadly, that project fell by the wayside due to other stuff taking precedence
- I am not sure how popular xmlrpc is nowadays in real life. My gut feeling is that this lib is used mostly by existing projects that adopted it a long time ago. How many users would pick up a new version with a vastly different API?
Btw, version 4.6.0 has been released, with the changes which were implemented in Feb. So definitely still relying on a lot of static methods, but it should now be possible to at least introduce usage of a PSR-3 Logger with the help of some glue code...
...aaand we are now approaching the situation where it is feasible, and almost, but not quite, nice, to use a Psr/Log compliant logger.
In the next version (planned to be 4.10.0):
- all classes which use the logger can finally have a logger instance injected into them (NB: this is still done via static methods, for BC)
- the logger methods which get called by the library have changed - now they implement the same spec as Psr\Log\Loggerinterface
- a demo file has been added, to showcase how the DI can be achieved. See https://github.com/gggeek/phpxmlrpc/blob/master/demo/client/loggerinjection.php
Still not planned until the next major version:
- making injector methods non-static
- having the Client and Server propagate the injected logger down their dependency chain
PS: I was thinking about adding add a method PhpXmlRpc\PhpXmlRpc::setLogger() that would alleviate the need for end users to know all of the classes which need the logger to be injected into...