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

Unclear Upgrade Guide #773

Open
mbabker opened this issue Mar 10, 2023 · 12 comments
Open

Unclear Upgrade Guide #773

mbabker opened this issue Mar 10, 2023 · 12 comments
Labels
status: waiting for feedback waiting for feedback from the submitter type: docs update documentation change not affecting the code

Comments

@mbabker
Copy link

mbabker commented Mar 10, 2023

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 AsabuHere added type: docs update documentation change not affecting the code status: help wanted requesting help from the community labels Mar 16, 2023
@AsabuHere
Copy link
Contributor

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
Copy link

ejunker commented Mar 16, 2023

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
Copy link
Contributor

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

@AsabuHere AsabuHere added status: waiting for feedback waiting for feedback from the submitter and removed status: help wanted requesting help from the community labels Mar 17, 2023
@adrianbj
Copy link

adrianbj commented Apr 4, 2024

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
Copy link

ejunker commented Apr 4, 2024

@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
Copy link
Author

mbabker commented Apr 4, 2024

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
Copy link

adrianbj commented Apr 4, 2024

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

@tiwarishubham635
Copy link
Contributor

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
Copy link

adrianbj commented Apr 4, 2024

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
Copy link
Contributor

tiwarishubham635 commented Apr 4, 2024

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
Copy link

ejunker commented Apr 9, 2024

@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
Copy link
Author

mbabker commented Apr 9, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for feedback waiting for feedback from the submitter type: docs update documentation change not affecting the code
Projects
None yet
Development

No branches or pull requests

5 participants