william26
commented
10 years ago Thank you for the time you spent reviewing our SDK, we appreciate it. Many of your changes indeed make our architecture easier to understand and to use. We'll be happy to work with you on improving the sdk for a future release.
I tested your changes with our tutorial project. In the .create method, you still return a Request object though the class has been removed. I think it would be better to keep that class and to move the request logic out of the Client class. Is there a particular reason why you moved the request methods to the Client class ?
After inspecting the architecture of this module I feel a rewrite is in order. I've begun this work here: https://github.com/TomHAnderson/sdk-php/tree/rewrite
The client isn't unit tested or functionally tested but the general shape of the class is coming into view. Rather than an injector pattern this rewrite uses a factory pattern with getters and setters for all options so if what you get from the factory isn't what you need you can set all configurable options manually with setters.
One change stood out in particular: The session use would namespace the app into a [oauthio] namespace. But the code was coded to have an already-namespaced namespace in the session you pass to the client, so all references to this session namespace have been removed. For the sake of a default $_SESSION $_SESSION['oauthio'] is used.
In this rewrite I'll substitute Guzzle for unirest. I'll follow the patterns in the excellent KeenLabs client https://github.com/keenlabs/KeenClient-PHP
I've created this ticket as a forum to bring ideas about a rewrite together and work out the issues between the current and rewrite.