Wrong php-doc types for Subscriptions properties

[UksusoFF]

\Stripe\Subscription have * @property int $trial_end but according to api documentation this can be now.

Same with * @property int $billing_cycle_anchor (it's also can be now) and * @property Plan $plan (it`s can accept hash string).

Maybe replace with int|string and Plan|string?

[UksusoFF]

Also seems like prorate property missed.

[virgofx]

Just a note -- I believe the special value now is only for sending I think the returned value is always int. (Would need to test to confirm) Not sure if that changes anything.

[UksusoFF]

But we can set property value and then call save() https://github.com/stripe/stripe-php/blob/master/lib/ApiOperations/Update.php#L36

This is used in Laravel Cashier https://github.com/laravel/cashier/blob/9.0/src/Subscription.php#L371

[ob-stripe]

This is a bit tricky. We're trying to move away from the "update properties then call save()" approach, precisely because it conflates resource attributes and request parameters in a confusing way (for instance, prorate is a valid parameter in subscription update requests, but it's not an attribute on the subscription object).

The now recommended way to update resources is to use the static method update() with the resource's ID as the first argument and the array of params as the second argument (cf. https://stripe.com/docs/api/subscriptions/update?lang=php). However, since many existing Stripe users rely on the save() method, we will keep supporting it for a long time.

My gut feel is that the PHPDoc should only describe the properties of the resource, not the possible update parameters, but I understand that this is a bit confusing because in many cases, resource properties and update parameters have the same names and types.

cc @brandur-stripe @nickdnk Do you have an opinion on this?

[brandur-stripe]

Oh crazy, I hadn't thought of that particular situation at all.

My preference would be what you said @ob-stripe in keeping the docs for actual properties only so as not to confuse things too much (also, in an auto-generated world I think it'd be a little unusual/complicated to have to pull properties out of a resource and then mix in some parameters as well).

I say that though understanding that there's a definite usability trade off — once you introduce something like PHPDoc users will expect it to be correct, and especially if you're using an IDE to assist it can be a little disorienting when it's not entirely accurate.

[ob-stripe]

My hope is that once we move to an autogenerated world, we would document update parameters in the PHPDoc of the update() method for each resource (we would no longer use common traits and generate the methods directly for each resource), and properly flag save() as deprecated in favor of update(). I'm not sure that PHPDoc supports documenting array keys though.

[brandur-stripe]

My hope is that once we move to an autogenerated world, we would document update parameters in the PHPDoc of the update() method for each resource (we would no longer use common traits and generate the methods directly for each resource), and properly flag save() as deprecated in favor of update(). I'm not sure that PHPDoc supports documenting array keys though.

Oh yep, totally — that's the optimal option. I was thinking if did end up putting parameters in the PHPDoc list and then wanted to try for perfect compatibility when porting from hand-maintained to generated.

[virgofx]

I'd have to agree that in order to be consistent ... the properties on the objects should be for reading only. I think we should move that direction and mark the save() method as @deprecated and simply remove it next bump -- say the next PHP 7.x bump... It's really easy to fix using setters and migrate those into an array for update().

Doing this would allow for consistent type hints for properties for objects that would improve static analysis.

[UksusoFF]

Also @property-read may be used for properties and @deprecated for save().

[ob-stripe]

Tagging this as future as I don't want to rush into anything here. We're currently in the process of deploying new tooling to generate the resources and methods in our client libraries, which will also enable larger refactors to the library. I don't want to prematurely flag some paths as @deprecated in favor of others until then.

We're not going to tackle stripe-php immediately, but should hopefully get to it in a few weeks. Thanks everyone for your feedback and your patience!

[virgofx]

@ob-stripe Can you clarify from a development/tooling perspective. Just curious -- Are you guys going to try to roll out changes such that all client-side libraries (by language) are able to receive changes somewhat simultaneously? Pretty cool, as I'm sure 99% of issues found in one client side lib are applicable in all the others. If this is the case ... I'm curious how the [ repo(s) structure / layout / build / process ] will work ... if you can share somewhat top-level details that would be awesome.

[ob-stripe]

@virgofx The project is still in a very early phase so I don't have much concrete information to share yet, but yes, the idea is that we would use the OpenAPI specification for Stripe's API (https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml) to generate the models and methods for all API resources, in all of our client libraries. (Technically, it would be an "augmented" version of the spec -- the general spec I linked above doesn't have all the information we need.)

We're already doing this for our Java library (e.g. stripe/stripe-java#777). The current implementation is specific to Java, but we're in the process of porting it to a more generic tool that would work for any language. This is what Brandur and I were referring to when we mentioned "auto-generated" in our earlier comments in this thread.

This change should be transparent to end-users of the library, but once it's live it will enable us to make wide-scope changes that are currently impossible because they would be too difficult to maintain manually -- up-to-date PHPDoc for every specific API method being one of them :)

[virgofx]

Can't wait -- Seems like every large org is playing catch up with every lang as they try to support the major 5-8 languages for all API-based projects. If designed generic enough ... could be used as an open-source framework for designing APIs that are cross platform.