search

twilio / twilio-php

A PHP library for communicating with the Twilio REST API and generating TwiML.
MIT License
1.56k stars 559 forks source link

Unclear Upgrade Guide #773

Open mbabker opened 1 year ago

mbabker commented 1 year ago

Issue Summary

The upgrade guide in this repository points to a "how to use this library" page in the Twilio documentation.

The Twilio documentation has an upgrade guide page that links to the UPGRADE.md file in this repository.

Neither of these pages actually gives any information on what steps downstream consumers need to take to upgrade to the new major version.

I would expect the upgrade guide to clearly spell out all steps required for a downstream consumer to upgrade, but instead, I'm linked to a couple of pages that mostly say that the only change is in how this library is auto-generated (which is more of an implementation detail for the maintainers than a concern for downstream users).

Steps to Reproduce

  1. Visit https://github.com/twilio/twilio-php/blob/main/UPGRADE.md#2023-03-08-6xx-to-7xx to review the upgrade guide for 6.x to 7.x, note there isn't any meaningful upgrade information and links to https://www.twilio.com/docs/libraries/php which is the basic "getting started" or "how to use" documentation
  2. From that page, note that there is a link to https://www.twilio.com/docs/libraries/php/usage-guide; viewing that page notes that the upgrade guide in this repository should be reviewed for additional details
  3. Welcome to a recursive loop? </sarcasm>
AsabuHere commented 1 year ago

This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog

ejunker commented 1 year ago

Yeah, I'm still unsure if there are breaking changes or not. The major version number changed to 7 so I assumed there would be breaking changes but it also says:

We ensured that you can upgrade to Php helper Library 7.0.1 version without any breaking changes.

So maybe there aren't any breaking changes?

AsabuHere commented 1 year ago

Hi @ejunker, With this major version we changed the process of generating PHP helpers from internal API Spec to open API specs. From now on we will use custom Twilio OAI generator and OpenAPI spec to auto-generate PHP SDK.

This migration enables us to rapidly add new features and enhance consistency across versions and languages. This will also enable open source community to have better visibility and contribute to our SDK helpers.

We are updating the docs to reflect this information

adrianbj commented 7 months ago

This still seems to be an issue with the release of v8 - it says "Note: This release contains breaking changes, check our for detailed migration notes.", but the upgrade guide says "We ensured that you can upgrade to Php helper Library 8.0.0 version without any breaking changes of existing apis". This is all very confusing :)

ejunker commented 7 months ago

@adrianbj FWIW, I upgraded to the new version and did not have to make any changes. I suggest giving it a try and most likely there will not be any breaking changes.

mbabker commented 7 months ago

Behind the scenes Php Helper is now auto-generated via OpenAPI with this release. This enables us to rapidly add new features and enhance consistency across versions and languages.

This sentence from the 7.x to 8.x notes is the exact same change called out in the 6.x to 7.x notes. So, what actually changed to warrant the new major version? Because as a consumer of this package, the release notes for 8.0 (or really either of the last two major releases TBH) don't give me any indicators of what I should be looking for, how to address any breaks that were introduced, or how 8.0 adding support for the application/json content type would affect me.

adrianbj commented 7 months ago

@ejunker - I expected it will be fine based on digging through the commits, but thanks for the confirmation.

tiwarishubham635 commented 7 months ago

This still seems to be an issue with the release of v8 - it says "Note: This release contains breaking changes, check our for detailed migration notes.", but the upgrade guide says "We ensured that you can upgrade to Php helper Library 8.0.0 version without any breaking changes of existing apis". This is all very confusing :)

Hi! In this version, we have provided backward compatibility. I am not sure where you are seeing that note. Maybe this is because in general major version upgrade comes with a breaking change. However, we have aimed to provide complete backward compatibility.

adrianbj commented 7 months ago

Note is shown with the release here: https://github.com/twilio/twilio-php/releases/tag/8.0.0

Does "we have provided backward compatibility" mean that there is a layer of code to modify old API calls to work with new methods, or nothing actually changed with existing methods? This also isn't clear and would be nice to know so that we have the knowledge to upgrade our existing code / understand what we should be doing going forward, rather than relying on old methods.

tiwarishubham635 commented 7 months ago

Behind the scenes Php Helper is now auto-generated via OpenAPI with this release. This enables us to rapidly add new features and enhance consistency across versions and languages.

This sentence from the 7.x to 8.x notes is the exact same change called out in the 6.x to 7.x notes. So, what actually changed to warrant the new major version? Because as a consumer of this package, the release notes for 8.0 (or really either of the last two major releases TBH) don't give me any indicators of what I should be looking for, how to address any breaks that were introduced, or how 8.0 adding support for the application/json content type would affect me.

As said above, with backward compatibility you should not see any breaking change at your side. However, since in this version we have completely migrated to Open API specs, we have released it as a major version change. In 7.0.0 version upgrade, we were maintaining both internal API specs as well as standard OpenAPI specs. Since the back architecture of generation of this SDK has changed, we introduced a major version release.

As far as the breaking change note in Release notes of 8.0.0 is concerned, it is a standard note for all Major version changes. The details are given in UPGRADE.md. I hope this answers your queries.

ejunker commented 7 months ago

@tiwarishubham635 if you are following Semantic Versioning then the major version should only increase if there are breaking changes. I did not see any breaking changes from 6 to 7 or from 7 to 8 for the users of the library and so that is why we are confused on why there are new major versions.

Also, there are other PHP composer packages that use this library as a dependency such as laravel-notification-channels/twilio with a version constraint of ~6.0|^7.16 and when you keep on bumping the major version then these packages have to update their requirements to also specify that they support the new major version. If you were to just bump the minor or patch version since there are no breaking changes then these packages' constraints would allow us to upgrade to the new version.

mbabker commented 7 months ago

@tiwarishubham635 if you are following Semantic Versioning then the major version should only increase if there are breaking changes

The SDK's not strictly SemVer, there've been plenty of signature changes in SemVer minor releases (to the point that I now use constraints like 8.0.* in my apps and review the code diff for every minor).

This just makes this new major release more frustrating because from the code diff it genuinely looks like there are zero B/C breaks unless they are considering 8.0 adding support for the application/json content type a break, in which case I would've expected what to look for for those affected by this change to have been called out better in the upgrade guide. Instead, we mostly have a copy/paste of the 6.x to 7.0 notes and the note about added content type support as the upgrade guide, which as a consumer of this library (both in applications and Composer packages) gives me nothing to work with.