Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

Question / Note regarding backwards compatibility #1712

Closed
nmokkenstorm opened this issue Jul 11, 2019 · 3 comments
Closed

Question / Note regarding backwards compatibility #1712

nmokkenstorm opened this issue Jul 11, 2019 · 3 comments

Comments

@nmokkenstorm
Copy link

I'm looking into an upgrade path for a community package, mostly to make it compatible with modern versions of Laravel, but along the way I solved all outstanding issues ( properly doing SSL, allowing for customisation of the Guzzle client and allowing multiple webhooks per Notification), and added some functionality I'd probably like to use in the future.

While we have a working version that we can use for the project that initially lead me to this package, I would like to merge the updates I have back into master when I'm done testing it in the app we're building, as it solves several issues that people have been mentioning for 1-2 years.

Ideally I would like to not break backwards compatibility, but for the desired functionality I need to at least be able to depend on a ServiceProvider being registered, so people on versions 5.3 or 5.4 of the framework would need to manually register it on upgrade. People on 5.5+ obviously have access to package auto dicovery, so it wouldn't be an issue for them. Following the rules of Semver, this upgrade would count as a major breaking change.

The issue I'm running into however, is getting PHPunit to play nice. Laravel 5.5 (LTS), supports up to PHP 7.0, which isn't officially supported anymore by the PHP team or PHPUnit. While this is a pain, it would be workable if not for some changes between some internal PHPUnit functions (in this case notably SetUp and TearDown having a return type void). Due to this I can't get the package I'm upgrading to work in both PHP 7.0 and 7.1+, due to newer PHPUnit versions not supporting the former.

My question now is as follows: what is the official advice regarding supporting PHP versions? I'd like to bump the PHP version up to 7.1.3, which is what Laravel has been depending on since 5.6. I need to make a breaking change for people who are on version 1.0 of this package anyways.

Tagging @themsaid here because he's listed on the Laravel Notification Channels website. Freek van der Herten, who's also listed as one of the maintainers, mentioned here that dropping support for PHP 7.0 might be a good thing to do.

@drbyte
Copy link

drbyte commented Jul 11, 2019

When it comes to packages and breaking changes, tagging a new major version is the best way to accommodate forced upgrade to newer versions of dependencies. People can use the older version of the package if they require the older dependencies.

So, I suggest that if some of your "fixes" are highly relevant for users of the current(old) version and don't need to change any package dependencies, then (as a courtesy) PR them into the current(old) version. (Of course, your tests for these new features will need to use the older phpunit syntax, and your code would have to comply with any other existing(older) dependencies.)

Then release a new major version. This would let you upgrade any dependencies. And then if there are any features you've written that really require the newer dependencies, include them in the new version (instead of backporting them to the old one as I mentioned above). And of course in this case you'd update your test syntax where required, as part of upgrading the package's dependencies.

That's how I'd initially think about it.

@mfn
Copy link

mfn commented Jul 11, 2019

7.0 can be really challenging, especially when Laravel < 5.5 comes into play.

I found the "sweet spot" with Laravel 5.5+ and PHP 7.1+ but, depending on the complexity there may still some hoops to jump through:

  • orchestra and very old versions need separate dependency on orchestra/database (Example)
  • older PHPUnit don't have all the assert methods, especially an issue when the old ones get deprecated (Example)
  • as for the PHPUnit void problem, yes, this requires a PHP bump unless you want to make an even more exotic travis CI

I also think 5.5 aligns nice because it's officially still supported => https://laravel.com/docs/5.8/releases#support-policy

@nmokkenstorm
Copy link
Author

When it comes to packages and breaking changes, tagging a new major version is the best way to accommodate forced upgrade to newer versions of dependencies. People can use the older version of the package if they require the older dependencies.

So, I suggest that if some of your "fixes" are highly relevant for users of the current(old) version and don't need to change any package dependencies, then (as a courtesy) PR them into the current(old) version. (Of course, your tests for these new features will need to use the older phpunit syntax, and your code would have to comply with any other existing(older) dependencies.)

Then release a new major version. This would let you upgrade any dependencies. And then if there are any features you've written that really require the newer dependencies, include them in the new version (instead of backporting them to the old one as I mentioned above). And of course in this case you'd update your test syntax where required, as part of upgrading the package's dependencies.

That's how I'd initially think about it.

First of all, thank you for your feedback. Highly relevant is a big word, but not being able to use the package in newer Laravel apps and the default disabling of SSL checking seems not to be the way to go in my opinion. Pretty much any change I wanted to make more or less required Laravel 5.5 to allow for a seamless (using auto discovery) upgrade, so I felt I was locked in releasing a major version anyways. The other 'problem' is that the package doesn't seem to be actively maintained anymore, because there are some old issues & Pull Requests that address (part of) the things that I wanted to address.

7.0 can be really challenging, especially when Laravel < 5.5 comes into play.

I found the "sweet spot" with Laravel 5.5+ and PHP 7.1+ but, depending on the complexity there may still some hoops to jump through:

  • orchestra and very old versions need separate dependency on orchestra/database (Example)
  • older PHPUnit don't have all the assert methods, especially an issue when the old ones get deprecated (Example)
  • as for the PHPUnit void problem, yes, this requires a PHP bump unless you want to make an even more exotic travis CI

I also think 5.5 aligns nice because it's officially still supported => https://laravel.com/docs/5.8/releases#support-policy

Yes, the reason I wanted to support 5.5 was definitely because of its status as LTS. Furthermore, I hoped package auto discovery would help a long way with a 'seamless' upgrade path. Ideally I'd also like to keep the minimum PHP version the same as for L5.5, but honestly it's a pain to get it working. I managed to work around the orchestra problem by just flat out dropping it as a dependency and doing the one thing the package needed from it, Mockery integration, manually in a base test class. This promptly became the main reason of the 'void problem' though.

All in all I think I will aim for a L5.5, PHP 7.1.3 release, tagged as 2.0.0. After that I will contact the people listed on the Laravel Notification Channel website, which happen to include the package author, on how to proceed with getting it listed. Thanks for your time 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants