PHP 8.1 Compatibility

[ksassnowski]

PHP 8.1 introduced return type declarations for most internal methods. Several classes in this library implement various built-in interfaces such as Iterator and ArrayAccess. These classes currently cause deprecation warnings since they don't provide the proper return type hints.

There are two ways to approach this problem.

Approach 1: Add #[ReturnTypeWillChange] Attribute

PHP 8.1 introduces the #[ReturnTypeWillChange] attribute which can be used to temporarily suppress the deprecation notice until PHP 9. Since the attribute syntax is backwards compatible (older versions will treat it as a code comment), this would not break backwards compatibility. It is, however, not a real solution as it simply hides the issue.

Approach 2: Adding the missing type hints

The "correct" solution would be to add the missing type hints to the various interface methods. This would of course be a breaking change as return types are only supported since PHP 7.0.

That being said, this project currently supports PHP versions all the way down to PHP 5.6, which as been EOL for close to 3 years. I think it's reasonable to tag a new version which fixes the deprecation notices in PHP 8.1 and drops support for all PHP versions that don't receive security fixes anymore (< PHP 7.4).

I'd be happy to provide the necessary pull requests.

[btry]

Hi

I fully agree with your point of view to prefer approach 2 and drop PHP 5.6 support. If you want to do a pull request, go ahead. I'll prepare a new major version with this change.

[ksassnowski]

Alright, sweet. So to some up, I will:

• add the missing type hints for the Iterator and ArrayAccess methods
• require at least PHP 7.4
• possibly bump the required versions of certain dependencies (like phpunit)

Sound good?

[btry]

Yes, it looks good to me.

[ksassnowski]

Great. Slightly off topic. Would you be open for a separate PR that adds a Github action to run the test suite against different PHP versions? I found this issue (#44) but it's from 2018 and there doesn't seem to have been any activity on it since then.

[btry]

Yes : let's drop travis and use github actions.

[ksassnowski]

I've run into a slight issue while adding the return types. The mixed type has only been added in PHP 8.0, so it's not possible to add it as a typehint without having to drop support for PHP 7 completely. Not adding the mixed type hint still causes a deprecation warning, unfortunately.

So I guess there are two options:

1. Use as many typehints as possible and fall back to #[ReturnTypeWillChange] where mixed would be required. Feels kind of icky since we're back to square one, more or less.
2. Require at least PHP 8.0

I definitely favor option two (especially as a maintainer), but completely dropping PHP 7 is definitely a bigger change than dropping PHP 5.6.

That being said, since we wouldn't be adding any features, it's not like people who don't use PHP 8 and up would have any reason to update anyways.

[btry]

https://www.php.net/supported-versions.php

I think It is too early to drop PHP 7.x support. Maybe it is time to have a dedicated branch for each major version (current and curent +1) to permit maintenance of both for some time.

[ksassnowski]

In that case I would tend towards using the #[ReturnTypeWillChange] attribute instead of mixed and keeping everything on one branch. The branches would be literally identical in features, they would only differ in some of their method declarations. I don't think this is worth adding the maintenance overhead of two separate versions.

But it's your decision, of course.

[btry]

The branches would be literally identical in features, they would only differ in some of their method declarations

Yes, but semver says a new major version should be created when there is a breaking change : https://semver.org/

8. Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

Then maybe this would not be a problem to create a branch for 2.0.x . Also, there is few activity on this prroject, then there should not be much overhead to have a new branch and still maintain 1.1.x at least 1 or 2 years.

[ksassnowski]

That still leaves the question about whether the 2.0 branch should require PHP 8 and up or not. Otherwise we'd still have to fall back to using #[ReturnTypeWillChange].

[btry]

I would propose to create a branch support/2.0 for a PHP 8+ version and an other branch support/1.0 for 1.X versions.

The names are from Git flow AVH branching model. Without strictly apply its rules, the branch names are understandable.

EDIT : I noticed cross post. Version 2 would support PHP 8 and above only, and version 1.x would still support PHP 7.x. This version could support #[ReturnTypeWillChange] without having issue until PHP 9. By security, we could set a high php version limit in composer.json.

[ksassnowski]

Ok, then I will prepare two PRs, one for each new major version 👍

[btry]

thank you very much for your contribution

[btry]

Branches support/1.0 and support/2.0 created. You may base your branches onto them.

[ksassnowski]

Opened a PR for version 1.0 (#70).

I'm honestly not sure if creating a separate branch for PHP 8 and up is worth it. The 1.0 branch would already work for PHP 8.1. So maybe just keep this one branch around and release a new major version in a year after PHP 7.4 is EOL? Then we could simply remove the #[ReturnTypeWillChange] attribute and use the mixed type hint instead.

[btry]

I think this is necessary to follow semver recommendation : dropping support for some PHP versons is a breaking change, then the rule is clear, even if this is overkill here. The pro is that you can avoid #[ReturnTypeWillChange] then there is no cleanup for this temporary lines of code (in a few months or years). Also, maybe this gives the opportunity to add more type hinting elsewhere in the code (among others)

I'll study soon the release of the next 1.x version.

[ksassnowski]

I don't believe that dropping support for a PHP version is even a breaking change. Composer will ensure that the new version simply won't get installed is the PHP version is not satisfied. So there's no danger of a composer update breaking any projects.

[ksassnowski]

Some relevant links:

• https://github.com/semver/semver/blob/d460299638d3d2af6fcf809285dc6a469e15742a/semver.md#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api
• https://sebastiandedeyne.com/composer-semver-and-underlying-dependency-changes/

In short, if my application requires PHP 7.3, it will simply not install a newer version of this package that requires PHP 7.4 until my own application becomes compatible.

[btry]

Hi

Ok, agreed : so no new major version needed. By the way, I forgot that releases 2.x were created after I splitted all classes in their own file. Therefore the branch support/1.0 is useless. My bad. I'll rebase your PR onto develop before merge, and both support/* branches being now useless, i'll delete them.

[ksassnowski]

Sounds good. Thanks!

[btry]

Hi

Tag v2.0.3 created with your contribution.

Be aware that I will probably remove the v prefix from all 2.x tags . I don't have enough time these days to do it right now. I hope the release will help you for your project.

[ksassnowski]

The tag doesn't seem to be available on Packagist yet. This looks related to an old issue of mine (#65) where the package doesn't seem to auto update on Packagist.

https://packagist.org/packages/rodneyrehm/plist

[btry]

Hi

Thanks you for the feedback; i'll investigate this week