Service factories

[ob-stripe]

This PR adds "service factories", i.e. classes that help initialize and expose instances of service classes. The intent is to alleviate the need for users to create the service instances themselves. Instead, they simply create an instance of the client, and the client then exposes properties for all services:

$client = new \Stripe\StripeClient("sk_test_123");
$customer = $client->customers->retrieve("cus_123");
$issuingCard = $client->issuing->cards->retrieve("ic_123");

Here's how it works:

• AbstractServiceFactory is the base abstract class for service factory. Using the __get() magic method, it exposes properties for services using a mapping of property names to service classes defined in concrete factories. The service instances are lazily initialized the first time the property is accessed.
  • CoreServiceFactory is a concrete child that exposes services for API resources in the root namespace, as well as service factories for namespaces. This class will be codegen'd.
  • IssuingServiceFactory is a concrete child that exposes services for API resources in the issuing namespace. This class (and other similar classes for namespaced resources) will be codegen'd.
• StripeClient is renamed to BaseStripeClient
• A new StripeClient class is added that derives from BaseStripeClient and adds a __get() magic method to expose services through a CoreServiceFactory instance
  • the reason for splitting BaseStripeClient and StripeClient is that BaseStripeClient contains manually written infrastructure code, and StripeClient will need to be codegen'd in order to have the @property PHPDoc tags that will make autocompletion work for services in PHPStorm and other IDEs.

@brandur-stripe @rattrayalex-stripe @remi-stripe @richardm-stripe Thoughts on this approach?

[remi-stripe]

The changes do make sense to me but I don't have a well-formed opinion around the architecture of the client classes. Will defer to someone like @brandur-stripe instead.

Can you show a bit more example on how you would make multiple calls, how you'd pass custom options like the API version or the Stripe-Account header if you wanted to set those globally or per request?

[cjavilla-stripe]

This approach seems good to me. I'd love to see a doc with all of the benefits to moving from the class/static method approach to the instance of client approach.

\Stripe\Stripe::setApiKey('sk_test_123');

\Stripe\Customer::create([
  'description' => 'x',
]);

to

$client = new \Stripe\StripeClient("sk_test_123");
$client->customer->create([
  'description' => 'x',
]);

Do we intend on doing state management inside of the client? Some sort of caching? Do we plan on continuing to support this and the static method approach?

[ob-stripe]

The StripeClient instance does not maintain any state. It simply holds various configuration parameters (right now, the API key and OAuth client ID, but I'm thinking of making it possible to set things like the Stripe-Account or Stripe-Version header at the client level too) and serves to expose the service instances.

This is generally a better design than the existing methods on the model classes because:

• it does not rely on global state(*): configuration values are specific to the client instance, making it easy to create and manage multiple clients with different configuration values
• all service methods are instance methods rather than static methods, which makes them a lot easier to test and mock

While the above is true for basically all languages, for PHP specifically it's been requested for a long time (over 5 years now!) by some users: #124.

Do we plan on continuing to support this and the static method approach?

Yes, at least initially. We would update the documentation and mark the old model methods as deprecated though, and hopefully remove them entirely at some point in the future.

(*): not 100% true yet, at the moment StripeClient still uses ApiRequestor to send requests, which itself uses a global/static variable to store the HttpClientInterface instance. This can/should be fixed later.

[cjavilla-stripe]

Thanks for the added context, @ob-stripe! Those points all make sense and I agree it makes it much easier to work with multiple clients and for testing. I don't have a strong opinion on the structure of the code.

[brandur-stripe]

Hey @ob-stripe, sorry about the late review here!

I'm strongly in favor of the service idea — IMO they're a huge win compared to setting globals everywhere because they expose a more powerful API (because it becomes possible to have multiple co-existing clients with separate configurations), and they're often a boon for purposes of testing/stubbing/etc.

the reason for splitting BaseStripeClient and StripeClient is that BaseStripeClient contains manually written infrastructure code, and StripeClient will need to be codegen'd in order to have the @property PHPDoc tags that will make autocompletion work for services in PHPStorm and other IDEs.

Good idea. More inheritance is never good per se, but the reasoning here seems sound. It might be worth adding a comment to clarify that StripeClient largely exists as a codegen target and that any significant changes/additions should be applied to BaseStripeClient.

The one question I had is: if we're codegen'ing anyway, would it make sense to have explicit getters for all the various service types instead of magic __get() with lookup tables? I'm just thinking that the former might be a boon for code completion engines, etc., although I understand that some of those are based on the PHPDoc comments, which I see are present here.

Anyway, great job here! So good to see these huge fixes randomly fly in, haha.

[ob-stripe]

Thanks Brandur!

The one question I had is: if we're codegen'ing anyway, would it make sense to have explicit getters for all the various service types instead of magic __get() with lookup tables?

Very good question. The reason I went with this approach is largely stylistic: for PHP, I think exposing (magic) properties is preferable to exposing getters, e.g. this:

$card = $stripe->issuing->cards->retrieve('ic_123');

is prettier than this:

$card = $stripe->issuing()->cards()->retrieve('ic_123');

Admittedly this is very subjective!

We could get rid of the magic and use regular attributes, but we'd also have to get rid of the lazy initialization. This might not be so bad: there are not that many services (55 at the moment) and the cost would be paid when the StripeClient is initialized.

[brandur-stripe]

Very good question. The reason I went with this approach is largely stylistic: for PHP, I think exposing (magic) properties is preferable to exposing getters, e.g. this:

Ah I see! Okay that makes sense.

I'll add a minor note that language-specific LSP implementations are probably the way that the developer world is moving right now, and optimistically, one day PHP will have a good one too. That said, there are way too many unknowns around that right now (e.g. __get() is so common in PHP that maybe the LSP has to try and handle it) to justify any kind of a change, so I don't feel too strongly about it. We can also change course in the future if it ever does become a problem.

[rattrayalex-stripe]

We could get rid of the magic and use regular attributes, but we'd also have to get rid of the lazy initialization. This might not be so bad: there are not that many services (55 at the moment) and the cost would be paid when the StripeClient is initialized.

FWIW that sounds like the tradeoff I'd make, static analyzability (and readability) is probably worth the startup cost, unless we can show that's material