Feature requests to improve deprecation handling

[keichinger]

Hi,

as a follow-up to #1005 (comment) I've decided to create a dedicated issue in order to track a feature request of mine.
I'd like to see Stripe's SDK make use of better deprecation handling so I, as a consumer, know up-front about a specific API change so that I can better plan keeping my apps up-to-date.

A bit background story

My project was using the API version 2019-03-14. A couple of days ago, I was updating all dependencies of my project. Since there were only minor version bumps, I wasn't expecting any significant change, or even a BC change as most libs adhere to SemVer.

After running my changes through our CI, it failed since it could no longer find two properties that I was using. Kind of confused and a little bit in disbelieve, I started investigating whether it was just our CI failing or whether it's a real problem. It turned out, our CI wasn't lying.

Spending some time looking at the code (and eventually finding the replacements I had to use now), I eventually learned about that Stripe has a really, really cool API versioning. Knowing that, I started looking into the API changelog and learned about what happened to my tax_percentage property, that has since been missing. My plan property was still missing in action, though.

At the end of the day I had to search through the closed GitHub PRs since most changelog entries were just super generic "Updated PhpDoc". The API changelog on the website was apparently locked in at the API version 2020-08-27, but the SDK was already at a higher version (the changes made to the SDK were newer than the API version).

That has actually got me thinking, whether I was missing something crucial from the docs when I was implementing the SDK into that project. It turns out, yes and no.

I'm certain that I could have saved a lot of time and confusion if the SDK could properly expose some kind of deprecations list, followed by a more specific changelog and maybe even a slightly different versioning strategy.

The actual proposal/feature request

Deprecations

1. It would be super awesome if static code analysis (as in my CI) could tell me beforehand that I'm using deprecated properties/methods before they're eventually removed. Since the SDK is using PhpDoc-based properties, we obviously can't use the @deprecated annotation, until they're converted to actual properties, potentially even with a getter and setter.

2. An alternative would be to use \trigger_error("This property has been deprecated. Use X::Y instead.", \E_USER_DEPRECATED); inside the magic functions, whenever a deprecated property is being read from or written to. Unfortunately, that would require to have a list with all deprecations per entity.

If that could be auto-generated and incorporated into your existing build and workflow, that would be superb!

The Symfony project is using a combination of both options, depending on the deprecation. If an entire method is deprecated, then the annotation is used. If a soft BC happens where the order or amount of parameters change, where people can still call a method with an old signature, which is internally handled to work with the new signature, then option 2 is used. All deprecations are logged into a dedicated log file, which tremendously helps upgrading.
Another big advantage is that it shows me right in my IDE (PhpStorm, and most likely others as well) that I'm doing something I need to pay close attention to.

Ultimately, this would be a huge DX improvement

More specific changelogs

I know this is a bit picky and nobody likes to write about every single change that has happened in a PR, but if most changelog entries (over at https://github.com/stripe/stripe-php/releases) just read "Updated PHPDocsUpdated PHPDocs", I'm not getting any actual information without having to read through the entire PR, looking for a specific clue.

Since I'm not sure how these PRs are made (manual labor or purely auto-generated base on an internal DSL with code-gen): Is it possible to generate a more specific changelog automatically?

Use better SemVer versioning schema

Composer makes it too easy to just upgrade the SDK to the latest version, hoping that it doesn't break anything in my app.

However, since this SDK highly depends on the correct API version being configured in your dashboard, there are a couple of problems I have with the current versioning strategy:

1. Composer is using the caret (^) versioning constraint by default (see https://getcomposer.org/doc/articles/versions.md#writing-version-constraints) if you just do composer require stripe/stripe-php. It's too easy to miss this crucial step that it's best to lock down your version, even though the package manager and even security and bug fixing updates encourage you not to do that
2. There is no real table that tells you exactly what SDK version matches which API version
3. Locking down a version is manual labor every time you're upgrading your dependencies, since you have to look up the matching SDK versions again and again. I can't even use build automation to tell me that a new version is available for use as I've locked down the version

All problems actually bother me on a different level:

1. I'd like to receive new updates for the Stripe SDK just like I do when I use packages from Symfony or any other vendor
2. Have to look the matching version up every time I upgrade my dependencies, which involves spending quite some time in the SDK, its release notes and the API changelog
3. Again, manual labor

My initial conclusion is that new API versions come with BC breaks, so they should be treated as a major version upgrade under the SemVer versioning constraints, so that my app doesn't break when I run a composer upgrade.
This can be achieved by using the API version number as major version, which means that the current 2020-08-27 version becomes 20200827.0.0.
This allows you, the maintainers, to create branches for each API version and still be able to publish bug fixing versions. Me, as a consumer of this SDK, I still can use the caret (^) versioning constraint and receive upgrades automatically with each composer upgrade.

That way I need to handle API version upgrades like any other major version upgrade: Manually and on my own schedule, when I want it.

---

Sorry for the huge wall of text :) ✌️

I'm super curious what you guys think about my proposals. If it helps, we can split them into dedicated GitHub issues and treat them separatedly.

[kevinpapst]

Seeing this post is almost a year old, I am really puzzled why there has been no reply by the Stripe team so far. All the raised points seem crucial to me and should be part of every development workflow and release strategy.

I just executed a composer update and my payment workflow broke apart.

Thanks to my CI the changes on your magic attributes were detected, but honestly guys, this is not any arbitrary library, this is one of the most crucial APIs we use in our projects. Removing attributes without any warning / deprecation warnings in minor version is a definitiv no-go.

I do understand that your entire workflow is based around automatic generation from a single point of truth, but please understand that deprecations and correct versioning should be part of a good DX - especially when it comes to payment libraries.

To give you one more point to think about: with the current workflow you achieve one thing: we will hard-code our composer dependency to one exact minor version of this library and likely not upgrade for a very long time. I can't believe this is what you want from a business perspective.

I wanted to backup the excellent explanations by @keichinger and ask for some feedback from @remi-stripe @richardm-stripe and any other involved Stripe developer.

[remi-stripe]

Removing attributes without any warning / deprecation warnings in minor version is a definitiv no-go.

We take semantic versioning extremely seriously and we defer all breaking changes to the next major version. When we release a new major we document all breaking changes in the changelog and often write a detailed migration guide such as here and here for the v7.0.0 major.

Obviously, we can make mistakes and remove things that we consider internal and non breaking but that some developers might rely on, but it's also something we take seriously. Do you have a concrete example of what happened during your upgrade, which stripe-php version you were using and which one you upgraded to so that we can better track this down and figure out what we missed?

[kevinpapst]

Thanks for the immediate feedback @remi-stripe !
Interestingly I did run into the exact errors that were covered by @keichinger before.

$ composer info|grep stripe
stripe/stripe-php                    v7.29.0 Stripe PHP Library
$ vendor/bin/phpstan analyse src -c phpstan.neon --level=5 --no-ansi
 274/274 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [OK] No errors 

followed by a composer update:

$ composer update stripe/stripe-php                                 
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading stripe/stripe-php (v7.29.0 => v7.92.0)
Writing lock file

$ vendor/bin/phpstan analyse src -c phpstan.neon --level=5 --no-ansi
 274/274 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------------------- 
  Line   Controller/Admin/Report/InvoiceController.php                  
 ------ --------------------------------------------------------------- 
  75     No error to ignore is reported on line 75.                     
  83     Access to an undefined property Stripe\Invoice::$tax_percent.  
  129    Access to an undefined property Stripe\Invoice::$tax_percent.  
 ------ --------------------------------------------------------------- 

 ------ ------------------------------------------------------------- 
  Line   Controller/User/SiteController.php                           
 ------ ------------------------------------------------------------- 
  52     Access to an undefined property Stripe\Subscription::$plan.  
 ------ ------------------------------------------------------------- 

 ------ ------------------------------------------------------------- 
  Line   EventSubscriber/InvoicePaymentSucceeded.php                  
 ------ ------------------------------------------------------------- 
  104    Access to an undefined property Stripe\Subscription::$plan.  
 ------ ------------------------------------------------------------- 

 ------ ------------------------------------------------------------- 
  Line   EventSubscriber/SubscriptionUpdated.php                      
 ------ ------------------------------------------------------------- 
  46     Access to an undefined property Stripe\Subscription::$plan.  
 ------ ------------------------------------------------------------- 

 ------ ------------------------------------------------------------- 
  Line   SubscriptionManagement/Stripe/Subscription.php               
 ------ ------------------------------------------------------------- 
  34     Access to an undefined property Stripe\Subscription::$plan.  
 ------ ------------------------------------------------------------- 

As you can see, the version hop was rather dramatic, but from my pov still okay'ish, because only minor version changes.

I'd like to highlight two points:

• I am still on that version, because I had the exact same issue in the past and wasn't confident enough to ship the library bump, but today instead of ignoring it again, I though I'd raise the topic here for clarification
• I already have multiple places where I added phpstan exceptions, because the old version is missing attributes in the PHPDoc, even though the fields were available in the API, eg:

This would be fixed with updating the library, my point is that it works but with a warning, so trusting your magic attributes is really difficult, especially if they suddenly vanish.

I didn't test if the new warnings are mistakes in the generated docs (which I assume) or if these are real BC breaks. By scanning the Changelog I cannot identify any BC change.

Lets take this one for example, because it seems to be the problematic one which removed the fields mentioned above. First look how you documented it in the Changelog:

and now compare that to the real changes.

Long story short: you are doing a great job here, don't get my first angry comment wrong. Initially I wanted to open a new issue and ask for advice, but then I saw that @keichinger already mentioned the missing attributes a year ago and nobody seemed to care (which shouldn't happen in a payment library). So I flipped a bit, sorry for that (!) but it got us talking, so mission accomplished 😈

I don't want to take over this issue entirely my my long poem, so I opened a new one about the missing fields.

[remi-stripe]

Thanks for the detailed report. I replied in the other issue, but in summary, the changes are expected as they are purely for docstrings and tests and don't change how the library itself works or the fact that the code would also still run as the API itself still has all those properties. Let's discuss in the other issue though!