Version 4.0.0

[hollodotme]

This is a discussion issue for the upcoming release of version 4.0.0.

I'd like to do the following things:

• Require PHP 8.0 as minimum PHP version (dropping support for PHP 7.1, 7.2, 7.3, 7.4)
  This would also drop the necessity of 3 different PHPUnit versions and would improve the maintainability of the test suites.
  Version 3.x will continue to support PHP 7.1-8.0, but won't add or backport new features, just bug fixes.
• Upgrade the codebase to use PHP 8.0 language features like typed properties etc.
• Require ext-fileinfo in composer.json to rely on mime type detection for multipart/form-data requests
• Refactor the AbstractRequest class, so that it always expects a request content object by requiring the ComposesRequestContent interface for the $content property.

  public function __construct( string $scriptFilename, ComposesRequestContent $content )
  public function setContent( ComposesRequestContent $content )

  This would make the named constructor newWithRequestContent(), that was introduced in v3.1.0, obsolete and should be less confusing how to compose the content of a request for users. See BC-Break with final constructor in AbstractRequest #56
• In general, find sources of potential BC breaks or usage confusion and eradicate them if possible
• Allow to configure/overwrite the STREAM_SELECT_USEC value used by stream_select, see configuring STREAM_SELECT_USEC #18
• Make the use of GET requests easier by adding a possibility to pass an array of query parameters to the request instead of composing a valid request URI in userland
• Add request methods with retry mechanism, first thoughts about that in [BUG]: Remove broken sockets from socket collection if write request fails #61. If writing to an idle stream fails for some reason try to use a new one instead of throwing an exception. Method names could be:
  • Client#tryRequest( $connection, $request, $maxTries = 5 ) and
  • Client#tryAsyncRequest( $connection, $request, $maxTries = 5 )
• Refactor and rearrange some tests, especially those which test for the behaviour of interrupted php-fpm processes.
• Remove license information from all PHP files and use the LICENSE file only
• Cleanup and split the documentation by major version
• Cleanup and split the CHANGELOG by major version

My main goals are:

• Take the library to the current language level
• Make maintenance easier
• Avoid unwanted BC breaks in future releases

---

@theseer, @sebastianheuer, @mnapoli, @Nyholm, @taylorotwell I'd like to hear your thoughts and ideas on this, also for improvements, features or things to consider from your perspective as you are the "main" users of this library, afaik.

Everyone else is also welcome to the discussion of course.

[theseer]

On first glance, all points seem to make sense. I won't have time before the weekend though to look into it in more detail. Just wanted to give a heads up :)

[mnapoli]

Hi @hollodotme, thanks for the ping. What timeline do you have in mind for this new major version?

Bref still supports PHP 7.2 and 7.3. Dropping 7.2 could make sense in Bref in the near future, but 7.3 would be a big step I'm afraid. Of course I definitely understand that it would be less maintenance effort for you, so I respect that.

Another question: when 4.0 is released, would 3.x be abandoned? (in the sense that you don't plan to tag new 3.x releases)

[hollodotme]

@mnapoli Thank you for your feedback.

I currently have no actual deadline for the 4.0.0 release, but would like to start as soon as a clear roadmap was extracted from this issue here. :)

My general approach regarding support of older versions is to keep them as they are and provide bug or security fixes if needed. I usually branch out a MAJOR.x-dev & MAJOR.x-stable branch before starting a new major version, so patching backwards is easy.
That means dropping support for PHP < 7.4 in 4.x release would not affect the current PHP compatibility for PHP 7.1-7.4 of the 3.x version. I just won't (automatically) add new features to 3.x once 4.x is available. If a new feature would be so important that it makes sense to also backport it to a 3.x release, I'd surely do that. That already happened and was no big deal.

[mnapoli]

OK that makes sense, thanks for clarifying.

That means that Bref will stay on 3.x until we drop 7.3. Since the 3.x version will not be closed for new patch releases (bugfixes) then that sounds good!

[hollodotme]

Added the feature of a retry mechanism to the list above, also discussed in #61 already.

[sanderdlm]

@hollodotme is there a 4.0.0-dev branch that we can send patches to? I have a basic one with typed properties lined up: sanderdlm@64a65d3

If you're still working on this, I could probably assist with the following tasks:

• Refactor the AbstractRequest class, so that it always expects a request content object by requiring the ComposesRequestContent interface for the $content property.
• Allow to configure/overwrite the STREAM_SELECT_USEC value used by stream_select, see
  configuring STREAM_SELECT_USEC #18
• Make the use of GET requests easier by adding a possibility to pass an array of query parameters to the request instead of composing a valid request URI in userland
• Remove license information from all PHP files and use the LICENSE file only

Let me know if I can help

[hollodotme]

@dreadnip Thank you for asking to contribute to this project. This is very welcome! ❤️

I created 3 new branches from current main:

• https://github.com/hollodotme/fast-cgi-client/tree/3.x-stable
• https://github.com/hollodotme/fast-cgi-client/tree/3.x-dev
• https://github.com/hollodotme/fast-cgi-client/tree/4.x-dev

So please feel free to create a pull request with 4.x-dev as target.
Looking forward to review it.

Thank you for your help!

[sanderdlm]

Cool! I've opened the first PR with the property type hints.

What is your opinion on bumping the minimum PHP version to 8.1 instead of 8.0? 8.0 only has 8 months of security updates left, and most major players in the PHP ecosystem are moving to 8.1 for new versions (PHPUnit 10 is ^8.1, Symfony 6.1 is ^8.1, Laravel 10 is ^8.1, etc..)

[theseer]

I'd be in favor of 8.1, maybe even 8.2.

Given the current composer.json claims compatibility with PHP 7.1+, we should ensure it works either way. Maybe we should restrict the current release to not be that open to newer PHP releases by default. ;)

But if we introduce potentially breaking changes, we can just as well raise the PHP Level to a current version.

All imho ;)

[mnapoli]

Please keep support for 8.1 at least 🙏

This package is a core part of Bref, we still support 8.0, I could imagine requiring 8.1 in 6 months but 8.2 would be pushing it unfortunately.

This is a low-level library, would there be significant pains in keeping compatibility with these versions?

[theseer]

The current version says PHP 7.1+.

It won't magically stop working when a 4.0 version of this library is released, regardless of what PHP version that one would require.

Given it's 7.1+, some rule should probably be defined as to how the support should continue - but so far I don't see a direct need to even raise the minimum version per se. It might not get any new features, but that's not expected I guess.

Pretty much what we at PHPUnit call "life support" ;)

[mnapoli]

It won't magically stop working when a 4.0 version of this library is released, regardless of what PHP version that one would require.

I know, I've used this argument as well myself in other situations 😄 But we both know this isn't the end of the story.

Bugfixes will go on v4, not v3. So for Bref, we'll want to be on the maintained branch.

I think it's worth considering more variables than just "doing like PHPUnit", for example the activity on the project: https://github.com/hollodotme/fast-cgi-client/graphs/contributors

Is there a lot of maintenance pain caused by supporting PHP 8.1, or even 8.0? I don't want to cause unnecessary burden on maintainers, but also feel like we shouldn't cause unnecessary burden on users :p

[theseer]

I do understand your point - though I don't exactly see the problem in this case.

There hasn't been any release (needed?) since December 2017 for the 3.x branch, so chances are there are not going to be many bugfixes required. Again, I'm not proposing to retire 3.x and force everybody to use 4.0 to get fixes.

All I'm saying basically is that when going for a 4.0, we should not restrict ourselves to any old PHP version. We can break BC, can change concepts and, maybe, make use of new syntax features to make the code more maintainable. If that even is the case. If we find, after creating a 4.0 it works fine with 8.0 even, we could simply lower the bar instead of artificially raising it.

[mnapoli]

so chances are there are not going to be many bugfixes required

Indeed, but I'd rather avoid betting on chance if possible. What if a bugfix impacts us in v3 and is fixed in v4? That's basically why everyone wants to be on maintained versions (not sure why this is a subject worth discussing tbh, staying on the maintained version is just common sense right? 🤷)

All I'm saying basically is that when going for a 4.0, we should not restrict ourselves to any old PHP version.

Understood. I'm asking for the opposite :)

I.e. "when going for 4.0, let's try to keep compatibility with PHP 8.0 unless there is value in not doing so."

My argument (to try and convince all of you) is: if we don't gain anything by dropping 8.0, we are hurting downstream users unnecessarily. So if all things are equal, let's go the "safe" route?

[sanderdlm]

My 2c would be that 8.1 gives us things like enums (which are currently implemented in this library as abstract classes with constants: https://github.com/hollodotme/fast-cgi-client/tree/master/src/Constants) and readonly properties (there are a lot of simple getters that could be eliminated in this lib by public readonly properties). Both of those are nice-to-have's, but not necessary at all. Union and intersection types are nice as well. As is constructor promotion.

The biggest improvement from bumping versions for this lib would be the drastically simplified testing & tooling workflow.

[hollodotme]

Looking at the current packagist stats of the library, around 13% of users are still on PHP < 8.0; PHP 7.4 (10.8%) or even 7.3 (2.3%); 21.2% are on PHP 8.0, 50% on PHP 8.1 and 14% on PHP 8.2.

My thoughts to this:

• Keeping the library code backwards compatible is not a problem at all. As @theseer noted there were no actual code changes necessary since 2017.
• Most changes since 2017 are related to the test pipeline. Currently 3 different PHPUnit versions are needed to run the test suites from 7.1 to 8.1 and latest PHPUnit version 10 is not even included, yet. (see: Makefile) — One of my main goals for the 4.x version was to consolidate, improve and slim down the test suites, preferably to 1 or 2 PHPUnit versions, bacause there are already tests which are not compatible with all PHPUnit versions. And maybe also add other test runners that make it easier to test processes/streams and their edge cases. PHPUnit is not ideal for this.
• I agree with @dreadnip that the changes we could do with a higher PHP minimum version are nice to have, but not necessary at all. Also the outlined new features do not need any of the new language features.
• I agree with @mnapoli that users will jump on the "maintained" or latest major version. Even though I did backport bugfixes and features to previous major version in the past to support users on older systems. And given the number of reported bugs in the last 6 years, I would continue to do so. (As I already said in my comment above in 2020.)
• From test suite maintenance view it would not make a diffrence to use PHP 7.4 or PHP 8.0 as minimum PHP version - Solely PHPUnit 9 would have to be used as the main test suite runner. I don't know yet if PHPUnit 9 would cause problems when runnig the test suite on PHP 8.2 or above. So maybe PHPUnit 10 must be used as a second test runner.
• PHP 7.4 is EOL and of course I also want to encourage users to upgrade to maintained PHP versions. So I would still go with the suggested PHP minimum version of 8.0 as stated in the issue body and would also follow the argumentation of @mnapoli.