omise-php Refactoring, decoupling part of the code out from OmiseApiResource

Ojbective

Currently OmiseApiResource is coupled with many responsibilities, just make the code too fat, and too hard to test. This pull request is taking some part of the code that is responsible for making a request to Omise API out from its current class and move to those new client classes.

Please note that this pull request wouldn't focus on refactoring anything much more than re-organizing the code.

Eventually, OmiseApiResource should not worry about how to make a request to Omise API or how to handle HTTP body that is returned.

So here I'm trying adding a layer for HTTP Requestor, Client, and HTTP Response handle classes (however, this pull request is introducing the HTTP Client layer first).

Quality Assurance

[x] All test cases should be green
[x] Test making POST request to make sure that the library is working properly

define('OMISE_PUBLIC_KEY', 'pkey_test_*******************');
define('OMISE_SECRET_KEY', 'skey_test_*******************');
define('OMISE_API_VERSION', '2017-11-02');
define('OMISE_USER_AGENT_SUFFIX', 'Test/0.1');

$customer = OmiseCustomer::create([
    'description' => rand(),
    'email'       => 'nam+customer' . rand() . '@omise.co'
]);

Result: Screen Shot 2562-08-26 at 09 25 46

[x] Test making GET and PATCH requests to make sure that the library is working properly

define('OMISE_PUBLIC_KEY', 'pkey_test_*******************');
define('OMISE_SECRET_KEY', 'skey_test_*******************');

$customer = OmiseCustomer::retrieve('cust_test_5h0tuupznl4dn7qhfh0');
$customer->update([
    'description' => 'Updated description'
]);

Result: Screen Shot 2562-08-26 at 09 33 10

[x] Test making DELETE request to make sure that the library is working properly

define('OMISE_PUBLIC_KEY', 'pkey_test_*******************');
define('OMISE_SECRET_KEY', 'skey_test_*******************');
define('OMISE_API_VERSION', '2019-05-29');

$customer = OmiseCustomer::retrieve('cust_test_5h0tyrqei5aocygx3yo');
$customer->destroy();

Result: Screen Shot 2562-08-26 at 09 36 55

Aug 26 '19 02:08 guzzilar

Adding 30 lines of code, more classes, and more runtime overhead seems an odd way of reducing fat :)

Aug 30 '19 03:08 jonrandy

@jonrandy it's quite a bit unfair to quote only that particular point without its whole context, isn't it? :)

Aug 30 '19 06:08 guzzilar

More seriously, although there is nothing really 'wrong' with this PR, I really don't see any point to it at this time. Its only reason to exist appears to be to enable a test to be more easily written. Complicating/extending the codebase just for this reason does not seem a good reason to refactor. The primary function of the code is to work as intended, not to easily lend itself to writing tests.

If we needed to support more than one 'client' (i.e. something other than CURL) for whatever reason then I could see some point, but to my knowledge at the moment no such requirement exists

Aug 30 '19 07:08 jonrandy

@jonrandy thanks for the input. I see in a different way that there is huge benefit that we leaning the class (tidying up the classes, single responsibility, testable, etc.)

However, appreciate the thought. Besides of seems to not necessary, is there any reason that preventing us from approving and merging this pull request?

Aug 30 '19 08:08 guzzilar

I don't feel comfortable approving the PR as I don't see it has any tangible benefits and merely adds fat that doesn't need to be there

Aug 30 '19 08:08 jonrandy

@guzzilar maybe some benefit would be shown to @jonrandy if you would show any code example It could show the difference in how did we use stuff and how we use after this PR.