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

Updated PHPDocs #1005

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Updated PHPDocs #1005

merged 1 commit into from
Aug 28, 2020

Conversation

remi-stripe
Copy link
Contributor

Codegen for openapi 2aeb1a7

r? @ctrudeau-stripe -stripe
cc @stripe/api-libraries

Copy link
Contributor

@ctrudeau-stripe ctrudeau-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ctrudeau-stripe ctrudeau-stripe merged commit ac62b1f into master Aug 28, 2020
@ctrudeau-stripe ctrudeau-stripe deleted the remi/codegen-2aeb1a7 branch August 28, 2020 20:10
@keichinger
Copy link

Hey @remi-stripe @ctrudeau-stripe I'm guessing some changes are in preparation for the next API version?

The reason I'm asking is: Today I've updated some of our dependencies to their latest version (including this package) and our CI has failed with some errors. The most nostable was the removal of Subscription::$plan. Looking at https://stripe.com/docs/upgrades#api-changelog I can't find any reference that tells me what to use as a replacement. In fact, I can't find anything related to this change noted there.

Since this change was done one day after the latest version, I'm guessing this one is for an upcoming API version?

@remi-stripe
Copy link
Contributor Author

@keichinger It's a bit orthogonal. We just undocumented plan in the API reference top-level on Subscription to nudge developers to look at items. The top-level plan property has been deprecated for over 3 years since it was only set for Subscription with only one plan/price.
Hope this clarifies the issue!

@keichinger
Copy link

keichinger commented Sep 9, 2020

@remi-stripe Thank you, I wasn't really paying attention to whether or not the property has been deprecated.

Maybe that's somewhat a problem of this lib. Due to the fact the properties are only PhpDoc comments + magic functions, there's really no way to use actual properties/getters/setters with a @deprecated keyword or a trigger_error() function call (like Symfony uses for its deprecated APIs with the \E_USER_DEPRECATED argument with a soft BC layer), so it's also possible to actively see these kind of changes in the CI.
If you want to keep the PhpDoc-based properties, maybe it's a viable workaround to add a static list somewhere with properties that are automatically checked inside the magic functions that then in turn trigger deprecation notices.

This could actually help developers seeing and addressing these kind of deprecations earlier, so the transition phase gets even smoother.

Is this a topic you're interested in discussing? Should I open a dedicated issue to further discuss this or is this something you don't want or can't invest too much time into, since there is already great docs and awesome API versioning?

@richardm-stripe
Copy link
Contributor

Hi @keichinger, I think a dedicated issue is a good idea. Helping developers detect and move away from deprecated APIs smoothly is definitely something we are interested in, and I appreciate you bringing up the prior-art with how Symfony does it.

We've considered moving away from PhpDoc-only properties and using class properties, although there are tradeoffs involved and we don't have plans to make that change in the short-term at least. Your idea of hard-coding a list of deprecations is interesting, although as a prerequisite for that there's some work we would have to do on our end to make sure that information makes it way into our code-generation infrastructure. It also sounds like a better changelog could have helped you have a better experinece, which is something to consider.

@keichinger
Copy link

Hey @richardm-stripe thank you for replying. I've gathered some of my thoughts and created #1012, where we can start discussing some of the problems I was encountering here.

I'm super excited and curious about where this is heading! ☺️

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

Successfully merging this pull request may close these issues.

4 participants