Centralize API request/response debug logging and response decoding
Closed this issue · 3 comments
PR #112 introduced the Wikimate::request() method, invoked for all API calls. The latter still check the response body for some error situations -- the doc-page test and the decoding result -- and perform the JSON decode. The error checks happen only in token() and login() anyway. These could be moved into request(), but it is also worth considering not to return an error array but throw an exception. I have to ponder this a bit longer. Any thoughts, @waldyrious ?
Secondly, the debug logging of the request array and decoded response array can also be centralized in request().
Upon a first look, it makes sense for the extra checks (and the debug logging) to be incorporated into Wikimate::request() so that the other methods can benefit from them, and also to avoid potential confusion as to why they're absent from some methods. (That discrepancy would have been less puzzling before the existence of Wikimate::request().)
As for how to handle those cases (return an error array or throw exceptions), I don't have an informed opinion. I trust your judgment on that one.
In the past few days, I read up on (PHP) exception handling. While opinions vary (depending on the programmer's preference for them, even) about when to pass on error returns vs. when to throw an exception, one good practice appears to be to throw only for unexpected (run-time) errors. Error values then cover situations one can expect, e.g. invalid credentials at login or a duplicate file getting uploaded.
In a chain of calls, e.g. in our case getText()->query()->request(), checking for error values at each/most layers makes for unwieldy code. So if request() encounters a (rare/strange/unexpected) communication error with the API, it would take extra checks at least in getText() and maybe also query() to handle. An exception at that lowest layer gets passed straight on to the caller of the library. In reality in getText() there is already a check for $r['error'] (the API error block) but you get the point I hope. And a communication error is fundamentally different from a usage error.
So this looks like a practical, pragmatic approach in our case, and that's what I implemented in #136.
I agree, that approach sounds sensible.