Get ready for PHP 8.4 / drop support for PHP < 7.1 ?

[jrfnl]

I've just manually triggered a test run and it shows that, as things are, the tests are failing badly (100% failure rate) when running on PHP 8.4.

While there may be more deprecations impacting Patchwork, the biggest and first problem which needs to be overcome is the deprecation of implicitly nullable parameters, i.e. typed parameter with a null default value, which are not explicitly declared as nullable.

Now, the typical fix for these is to make the parameter explicitly nullable, like so:

-function relay(array $args = null)
+function relay(?array $args = null)

... however, this codebase currently still has a minimum supported PHP version of PHP 5.4, which makes that impossible as the nullable parameter syntax is only available as of PHP 7.1.

Okay, so the first thing to do is decide how to solve this.

The options are:

1. Remove type declarations from optional parameters which have a null default value and do in-function type validation instead.
   This is a BC-break for any overloadable methods (non-final class, non-private method) as it effectively widens the type and can cause signature mismatches for the overloaded methods.
2. Remove the null default value from parameters.
   Well, this is a 100% BC break and not a good idea. The only case where this option would be valid is required parameters with a null default, but even then, it would be a BC-break for overloaded methods.
3. Raise the minimum supported PHP version to PHP 7.1 and make the type declarations nullable.
   Note: this is not a BC-break for overloaded methods as parameters with a null default are already implicitly nullable, so this will not trigger a signature mismatch.

I would personally recommend option 3 and releasing this in a new minor.
Composer can do the version negotiations and install the last 2.1.x version for users still on PHP < 7.1 and the latest 2.2.x (or higher) version for PHP 7.1+.

If I look at the Packagist PHP version stats for Patchwork, PHP 5.4 - 7.0 currently translates to 6.2% of the usage of Patchwork, with the bulk of this (5.8%) being PHP 7.0, so the fast majority of users could move onto 2.2.x without issue and only a small percentage of users would need 2.1 + 2.2.

@antecedent @anomiex Care to give me your opinion ?

If support for PHP < 7.1 is dropped in the 2.2.0 release, a decision would also be needed about whether or not the 2.1 branch will still be supported for pertinent bug fixes or not.

Note: I'm willing to put in the work to create an initial patch to drop support for PHP < 7.1 and fix the implicitly nullable parameters, but would like a "go ahead" before investing the time.

Also note that this initial patch will only do the version drop and PHP 8.4 compat fixes. Other modernizations of the codebase/cleaning up of code which becomes redundant after the version drop, may need follow-up PRs.

[anomiex]

I don't really like it, but I agree with your reasoning.

My own use cases for the package will be part of the percentage that need 7.0 support until November. That's not a problem for me though.

I would personally recommend option 3 and releasing this in a new minor.

If we're doing semver, dropping support for old PHP versions should be a major version bump. So 3.0.0, not 2.2.0.

[jrfnl]

I would personally recommend option 3 and releasing this in a new minor.

If we're doing semver, dropping support for old PHP versions should be a major version bump. So 3.0.0, not 2.2.0.

The prevailing reasoning in the PHP world is that, no, a PHP version drop does not need to be in a major if the package is (predominantly) managed via the Composer package manager as Composer will just serve a version compatible with the PHP runtime anyway.

Some articles about this (though there should be plenty more which my search-fu didn't find):

• https://sebastiandedeyne.com/composer-semver-and-underlying-dependency-changes/
• https://medium.com/@sampart/semantic-versioning-when-you-change-the-required-programming-language-version-16a3a3555c95

The only reason for doing it in a major for Patchwork could be that Patchwork is also released as a PHAR file, though it's unclear to me whether people actually use it via the PHAR.

[jrfnl]

@antecedent Care to give your opinion ?

I'm perfectly fine with doing this as a new major instead of a minor if so desired, but the main thing is that something needs to be done reasonably soon before end-users trying to make their code compatible with PHP 8.4 and using Patchwork as part of their project/tests, are impacted.

[jrfnl]

@antecedent Ping ? I'd love to move this forward to unblock users of Patchwork who want to get ready for PHP 8.4 before its release.

[antecedent]

Yes, I am in favor of option 3 too, as well as of making it a minor version increment (2.2.0). Thank you for your putting all the care into this, and really sorry for being the one blocking this entire thing again.

I guess that alternatively, one could consider an extra preprocessing step for whenever Patchwork's internals are included, just to scrap the ?s if the PHP version is < 7.1. But I am quite unsure if that would be worth it; also, it is something that can be done retroactively after going with option 3 — as in 2.3.x recovering the compatibility for PHP < 7.1 again.

So once again, in favor of option 3 and a minor version increment.

[jrfnl]

@antecedent Thanks for getting back to me. I have a commit ready for the version drop, but it would conflict with open PR #158 (dropping HHVM support).

Would you prefer I pull the PR anyway and resolve the conflicts with PR #158 afterwards or is PR #158 acceptable to go in for the 2.2.0 release (as HHVM has effectively not been supported since they dropped PHP support anyhow) ? In which case, it would be more straight forward to merge that one first and for me to pull the PHP < 7.1 version drop after.

[jrfnl]

Regarding the PHAR file usage - I just came across this site which can give us stats about this: https://somsubhra.github.io/github-release-stats/?username=antecedent&repository=patchwork&page=1&per_page=30

By the looks of it, the number of downloads for the PHAR are negligible and we may even want to consider dropping support for releasing via a PHAR completely (in a future release).

[antecedent]

To my judgment, which eventually is arbitrary, let's put PR #158 under 2.2.0 as well, even if against the spirit of strict semver.

As for doing away with the PHARs, I would also be fine with that, given the stats that you brought up.

[jrfnl]

@antecedent Looks like we're all in agreement. I've opened up issue #162 as a reminder to drop support for PHAR files in the future.

So the only question I now still have is one of procedure. I can see you've both approved PR #158 (HHVM drop). Should I merge it ? (I'm not used to merging my own PRs and kind of expected it to be merged once the second approval came in).

[antecedent]

@jrfnl, please do. I am just unaware of the norms around this.

[jrfnl]

I've merged #158 (HHVM) now and opened the follow-up PR #163 to drop support for PHP < 7.1. Once that one is merged, there is one more PR I need to open (aside from the two open already) related to PHP 8.4 supported, Then once those three are merged, the build should be able to pass on PHP 8.4.

I am just unaware of the norms around this.

Norms for this are different per repo and also largely depend on whether there is a sole maintainer who can merge or multiple maintainers who can.

For me, when I'm the sole maintainer, I will still use PRs, and will then merge my own PRs, though depending on what it is/which repo, I may leave the PR open for a few days to give others (watchers) a chance to review my work.

When I'm not the sole maintainer (or when I'm a "drive-by/occasional contributor"), I generally don't merge my own PRs. For transparency, it generally always seemed to make more sense to me to have someone else merge those PRs, as it is a clear signal that the PR has been reviewed and approved by a second person before merge.

As for this repo... it's really up to you (or the three of us together) to decide what should be the procedural norm. Might be good to set one ?

[jrfnl]

FYI: I've moved whatever was still left in the 2.1 milestone to a 2.2.x milestone.

[antecedent]

Thank you, and I find it really helpful that you explained the procedural thing in detail. In that case, I agree that we could keep up the "someone else initiates the merge" norm, at least for the time being. It does sound a bit redundant at first, but I see how this could be the good kind of redundant.

[jrfnl]

Now PR #163 has been merged, I've opened the last two PHP 8.4 PRs - once all four of the PHP 8.4 related PRs have been merged, the build should start passing again.

• PHP 8.4 | Console: remove use of E_STRICT #160
• PHP 8.4 | Silence deprecation for trigger_error() #161
• PHP 8.4 | Fix implicitly nullable parameters #165
• PHP 8.4 | Remove use of E_STRICT from the tests #166

[anomiex]

When I'm not the sole maintainer (or when I'm a "drive-by/occasional contributor"), I generally don't merge my own PRs. For transparency, it generally always seemed to make more sense to me to have someone else merge those PRs, as it is a clear signal that the PR has been reviewed and approved by a second person before merge.

Another convention for a multiple-maintainer situation, particularly when there are few non-maintainers contributing, is for the reviewer to use the "approve" feature to indicate approval, then the PR author self-merges when they're ready.

In that case, I agree that we could keep up the "someone else initiates the merge" norm, at least for the time being.

👍

[jrfnl]

🎉 We have a passing build on PHP 8.4: https://github.com/antecedent/patchwork/actions/runs/11074672941/job/30773992717

@antecedent As far as I'm concerned, all that's left to do for this issue is to tag a release ;-)

[antecedent]

Hooray! Well done, @jrfnl and @anomiex. Thanks so much again!

[jrfnl]

Thanks @antecedent and @anomiex for the collaboration to get this done and out in the world!