Better DX for new strong PHP types

[bdsl]

Is your feature request related to a problem? Please describe.

I just upgrade from version 15 to 17, and I very much appreciate the new type safety from #1837 .

However the types as they are now coded are difficult to work with, since they are many hundreds of characters of docblock-defined types, often repeating the same or similar information across multiple functions - for instance in the arguments to Customer::create (

stripe-php/lib/Customer.php

Line 54 in 3ed5494

* @param null|array{address?: null|array{city?: string, country?: string, line1?: string, line2?: string, postal_code?: string, state?: string}, balance?: int, cash_balance?: array{settings?: array{reconciliation_mode?: string}}, description?: string, email?: string, expand?: string[], invoice_prefix?: string, invoice_settings?: array{custom_fields?: null|array{name: string, value: string}[], default_payment_method?: string, footer?: string, rendering_options?: null|array{amount_tax_display?: null|string, template?: string}}, metadata?: null|StripeObject, name?: string, next_invoice_sequence?: int, payment_method?: string, phone?: string, preferred_locales?: string[], shipping?: null|array{address: array{city?: string, country?: string, line1?: string, line2?: string, postal_code?: string, state?: string}, name: string, phone?: string}, source?: string, tax?: array{ip_address?: null|string, validate_location?: string}, tax_exempt?: null|string, tax_id_data?: array{type: string, value: string}[], test_clock?: string, validate?: bool} $params

) and Customer::update().

We check our code with Psalm and PHPStan. The complex types mean we get very long error messages and it's hard to see whats wrong when we aren't passing correct arguments, especially with Psalm.

As we wrap the stripe library in our own class, and have other classes that return arrays suitable for passing to stripe we tend to need to refer to these types across our application code. We can reduce this effort a bit by making supertypes with just the properties we actually use, but it's still quite tricky and I ended up suppressing errors in some places.

My feature request is to adjust this API to make the types much easier to understand and work with. To avoid unnecessary BC breaks it could be done by providing new functions alongside the existing ones for users to migrate to, or having the functions accept a union of what they do now and something better.

Describe the solution you'd like

IMHO the better options in my order of preference would be:

1. Accept PHP value objects using a combination of the native PHP type system, smaller value objects inside the big ones and docblocks on individual properties to ensure all required properties are present and correct.

2. Instead of repeating docblock based array-shape types many times in the API use type aliases, with
   PHPStan local type alias and/or psalm-types . Reference these type aliases (and unions & intersections of them) in function docblocks to avoid writing any complex repetitive types.

As an example of option 1, perhaps the usage of \Stripe\Service\CustomerService::create could change like so:

// current
        $stripe = new \Stripe\StripeClient('api_key');
        $stripe->customers->create([
            'name' => 'Jenny Rosen',
            'email' => 'jennyrosen@example.com',
        ]);

// new
        $stripe = new \Stripe\StripeClient('api_key');
        $stripe->customers->create(new \Stripe\CreateCustomerRequest(
            name: 'Jenny Rosen',
            email: 'jennyrosen@example.com',
        ));

In a transitional period to allow users to migrate the create function could accept a union of the existing array type and the new CreateCustomerRequest, or there could be a new function to use instead for people wanting to opt-in to the new class based API.

Potentially the name and email properties could themselves be value objects instead of simple strings, which might save stripe some processing power by having checks for sensible values run in those constructors on customers PHP servers as part of the CreateCustomerRequest value object creation.

Describe alternatives you've considered

No response

Additional context

No response

[bdsl]

It looks like your Java SDK does something very similar to what I'm hoping for in PHP, although thanks to constructor property promotion the CustomerCreateParams class in PHP could be more concise than it is currently in Java.

https://github.com/stripe/stripe-java/blob/3da957955e406b0216e90f2936a73021b6c4532d/src/main/java/com/stripe/service/CustomerService.java#L196

[bdsl]

And "making supertypes" as I mentioned is also difficult because for Psalm at least array{a?: string, b?: string} is not actually a supertype of array{a?: string, b?: string, c?: string}, as the equivalents would be in Typescript. Psalm does not treat arrays as "open " unless they include ... to indicate that the values may contain additional keys not mentioned in the type.

Only the open array type array{a?: string, b?: string, ...} is a supertype of array{a?: string, b?: string, c?: string}

The types currently used in the stripe docblocks are not open.

Additionally Psalm and PHPStan are not able to infer array shape types from values returned by function implementations, meaning that if when we have a function that returns an array with many keys we would have to repeat the names of all those keys in the function docblock to use that exact shape as a key. If we don't want to do that we have to either return a more vague type, or an open array type.

Once we have an open array type, even if it includes all required properties for calling a stripe function, Psalm doesn't consider it assignable to the closed array type that stripe docblocks require.

[bdsl]

Also relates to #1854

[prathmesh-stripe]

Thanks for the detailed writeup.

1. We initially did consider supporting Psalm but we ended up picking PHPStan since it was more popular. Although we currently have no plans to support Psalm, we appreciate your feedback and would like to continue iterating on our type support in PHP. I am curious to know more about your use case for using both PHPStan and Psalm?

2. PHP does not support inner classes like Java/Python. Supporting CustomerCreateParams like pattern that you described above would mean that we will have to expose a lot of inner type classes are top level classes. This would add confusion for existing and new Stripe SDK users. Therefore we chose to not go that route.

Would adding JSON like pretty formatting to existing single line types help you out with readability?

We will keep this thread open to gather more feedback from the community.

[bdsl]

Hi Prathmesh,

About Psalm and Phpstan - both tools read each others annotations (and non-prefixed annotations) so other than in edge cases supporting either one supports both.

Personally I've been using only Psalm for several years (as well as contributing to it) and that's the only one I have much experience with, and at least historically there have been quite a few things Psalm does that PHPstan doesn't, as well as vice versa. I know the authors of both tools have generally suggested people run both rather than picking one or the other. But more recently as I've seen PHPStan gaining popularity I wanted to add partly to mitigate risks of relying on continuing Psalm development, but also because of additional things it can detect etc.

I do find there are fairly frequently issues that one tool detects and the other doesn't so I think we get some value by having both. It also just means we can choose which CLI we prefer and run that one first during local dev cycles. PHPStan only very recently got support for the @internal tag for instance, which Psalm has understood for a long time.

The main PHP app I work on is actually public on at https://github.com/thebiggive/matchbot/ . You can see our phpstan and psalm configs and the class we created as a very thin wrapper around your library, mostly to allow switching it out for a stub during load testing: LiveStripeClient / Stripe

PHPStan definitely gives easier to read error messages for complex structural types.

Pretty-printing the types in the docblocks would definitely make them much easier to read, but then there's the tradeoff of adding many lines to the php files making it harder to scroll through to find the function I want to read etc, so not sure on balance if that would be good or bad. I think combined with type aliasing to reduce repetition (PHPStan local type alias ) it would be a very big improvement though.

I'm not convinced that the Java style solution would be more confusing if you could find good names for the request classes, and making them read-only would help to minimize any misuse of them.

But another option might be to take advantage of the fact that PHP supports named and optional function parameters - so you could have a create customer function that takes many params. Would be very annoying in a language that requires positional params but probably OK in PHP. So same as in my suggestion with the CreateCustomerRequest except instead of passing the params to a VO constructor we pass them directly to the create method.

        $stripe = new \Stripe\StripeClient('api_key');
        $stripe->customers->create(
            name: 'Jenny Rosen',
            email: 'jennyrosen@example.com',
        );

The same API would also be usable by passing an array with the array unpacking feature:

        $stripe = new \Stripe\StripeClient('api_key');

        $requestArray = ['name' => 'Jenny Rosen', 'email' => 'jennyrosen@example.com'];

        $stripe->customers->create(...$requestArray);

[bdsl]

The PR where we upgraded our integration from v15.10.0 to v17.2.0 may be of interest: https://github.com/thebiggive/matchbot/pull/1361/commits

[bdsl]

By the way @prathmesh-stripe can I ask what the 'future' label you have on this and other issues means? Thanks