omise / omise-php

Omise PHP library
https://docs.opn.ooo
MIT License
67 stars 41 forks source link

Proposal for 3-0-0 design #59

Closed iwat closed 1 year ago

iwat commented 7 years ago

I do not see any design document yet, so I propose a new one here:

If Omise is going to create a new 3-0-0 release, if backward compatibility is not an issue, I would like to propose 2 new designs:

  1. Use static as usual, but add service locator pattern for testability. Bonus: this can be partially implemented on 2.x.
use \Http\Client\HttpClient;

use \Omise\Credential\CredentialProvider;
use \Omise\Credential\EnvarCredentialProvider;
use \Omise\OmiseClient;
use \Omise\TestSupport\FixtureHttpClient;
use \Omise\ServiceLocator;

class ChargeTest
{
    public function setUp()
    {
        $locator = ServiceLocator::getInstance();
        $locator->restoreDefault();
    }

    public function testListCharges()
    {
        $locator = ServiceLocator::getInstance();
        $locator->register(HttpClient::class, function () {
            return new FixtureHttpClient(__DIR__.'/../fixtures/'));
        });
        $locator->register(
                CredentialProvider::class,
                function () {
                    return new EnvarCredentialProvider('OMISE_PUBLIC_KEY', 'OMISE_SECRET_KEY');
                }
        );

        $charge_list = OmiseClient::listCharges();
        $this->assertInstanceOf('\Omise\Resorce\List', $charge_list);

        foreach ($charge_list->data() as $charge) {
            $this->assertInstanceOf('\Omise\Resource\Charge', $charge);
        }

        // backward compatibility is still partially supported
        $charge_list = OmiseCharge::retrieve();
        $this->assertInstanceOf('\Omise\Resorce\List', $charge_list);

        foreach ($charge_list->data() as $charge) {
            $this->assertInstanceOf('\Omise\Resource\Charge', $charge);
        }
    }
}

The idea:

Edit

  1. No static methods, no service locator, all hard wired.
use \Omise\Credential\EnvarCredentialProvider;
use \Omise\OmiseClient;
use \Omise\TestSupport\FixtureHttpClient;

class ChargeTest
{
    public function testListCharges()
    {
        $http_client = new FixtureHttpClient(__DIR__.'/../fixtures/');
        $credential_provider = new EnvarCredentialProvider('OMISE_PUBLIC_KEY', 'OMISE_SECRET_KEY');
        $omise_client = new OmiseClient($credential_provider, $http_client);

        $charge_list = $omise_client->listCharges();
        $this->assertInstanceOf('\Omise\Resorce\List', $charge_list);

        foreach ($charge_list->data() as $charge) {
            $this->assertInstanceOf('\Omise\Resource\Charge', $charge);
        }
    }
}

The idea:

One unanswered question: $charge['captured'] versus $charge->captured(), does dynamic array access still make sense?

liverbool commented 7 years ago

Hi, @omise-php

After trying to use 2.x branch in my personal project but have no luck (case of pure static implementation), i decided to create an alternative repo https://github.com/phpmob/omise that designed for SOLID use.

Hope to see SOLID package in 3.x branch. 👍

Feedback & PR welcome!

guzzilar commented 7 years ago

@liverbool That is pretty interesting one! I'll definitely check!

aashishgurung commented 1 year ago

Since there hasn’t been any activity on this issue for a while, we’re going to close it. If you’re still experiencing this issue, please feel free to reopen it with more details and we’ll take another look.