Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Improve PHP types hinting for method parameters and class properties #1787

Open
prathmesh-stripe opened this issue Nov 18, 2024 · 1 comment

Comments

@prathmesh-stripe
Copy link
Contributor

👋 Hello PHP community, we are looking for community feedback on improvement we are working on.
We are extending type hints that exist in Stripe-PHP SDK. New type hints will be compatible with PHPStan. Type hint improvements we are planning to implement so far:

Create/Update/Retrieve/Delete/All/Search parameters

Existing parameters array in Stripe functions do not specify the keys of the array.

* @param null|array $params

After the changes, you will be able to know the keys that you can pass without switching out of your IDE.

* @param null|array{customer:string, components: array} $params

You will see hints in IDE(PHPStorm used in screenshot below) when writing array parameters for the Stripe API methods.
PHPStorm IDE with array type hints

Class properties

We will change the type of class properties from StripeObject to something more specific.

For example: Invoice settings is defined as a StripeObject in Customer resource.

* @property null|\Stripe\StripeObject $invoice_settings

After adding phpstan types in PHP docs, you will be able to call custom_fields and rendering_options on customer->invoice_settings without PHPStan complaining.

// New
/** 
 * @phpstan-type CustomerInvoiceSettingsCustomField object{name: string, value: string}&StripeObject
 * @phpstan-type CustomerInvoiceSettingsRenderingOptions object{amount_tax_display?: string, template?: string}&StripeObject
 * @phpstan-type CustomerInvoiceSettings object{custom_fields: array<CustomerInvoiceSettingsCustomField>, rendering_options: CustomerInvoiceSettingsRenderingOptions}&StripeObject
 * 
 * @property null|CustomerInvoiceSettings $invoice_settings
 */

Conclusion

Existing users shouldn't be affected by our addition of types in PHPDocs. These additions will improve development experience for you.
If you have more ideas about what you would like to see in our PHP SDK regarding types and type hinting, please add your suggestions in this thread.

@nickdnk
Copy link
Contributor

nickdnk commented Nov 18, 2024

Hello

I was linked to this by a Stripe dev given my history with this repo and PHPDocs in particular (hence why I'm here so fast).

I think this is a very good middle-ground. I'm assuming you want to avoid introducing breaking changes and also want to prevent actually breaking anything by accident, in which case adding only comments is a very safe approach.

I hope this gets auto-generated from somewhere, or it's going to be a lot of work for you guys (and/or the community) to maintain - like I used to help with via PRs.

Some things I would love to see in the SDK is more strict types from PHP itself (return types, parameter types etc.) and the use of some of the newer features, such as enums, where applicable. A string backed enum could potentially be a good way to get started, if you would allow a parameter to be specified like this:

public function foo(AnEnum|string $bar) {
}

In which case you could use both say a PaymentType enum and a regular string (PaymentType::card and just card), which should make migration/upgrading easier while relying less on comments. The advantage in this case of course being that you would see runtime errors if you pass in an invalid parameter (before the Stripe API tells you, which could be especially useful during integration testing where you may not actually send off requests). I'm unsure, however, how difficult it would be to automate something like generating enums.

I'm aware that this would require bumping the minimum PHP version, but on that note I think you should also be slightly less considerate of supporting PHP versions that are no longer maintained with security updates. Especially when dealing with something as sensitive as financial transactions, I think having SDKs compel users to upgrade to a secure version of PHP is acceptable. This repo has a history of going very far back for PHP support. It's currently sitting at 5.6 as the minimum version which is widely considered ancient and had EOL on 31 Dec 2018 - almost 6 years ago! It's not even listed on the "supported versions" anymore (https://www.php.net/supported-versions.php). This approach is holding back this package a lot, and I would urge you to consider bumping this to at least something like 7.2 or 7.3 so you can unlock some of the newer language features.

Edit: To support this argument, take a look at this article: https://stitcher.io/blog/php-version-stats-january-2024
5.6 is not even on this, but 7.1 is only being used to pull 1% of packages from packagist, so it should be a very small set of users affected by bumping to a more recent version. And this article is almost a year old.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants