PHP 8.1 compatibility

[martinvenus]

No description provided.

[BenMorel]

@DusanKasan Could you please merge this and tag a relase? Thank you!

[martinvenus]

@DusanKasan Is there anything else I can do for this PR to be merged?

[DusanKasan]

This technically fixes compatibility, but there's also different parts that also need fixing and dependencies updated for at least 8.0 before this is merged as we need the tests to pass and everything.

Also, I will check but I vaguely remember not all dependencies being 8.1 compatible in the past. Will double check.

There's a PR for 8.0 compatibility that introduces generics and some more BC stuff. Any reviews are appreciated. #63

[davidmpaz]

Hello All,

may I suggest here that, instead adding the attribute about the future change of type, it is added the return type to make the method compatible with base class public function getIterator(): Traversable; like described in:

https://www.php.net/manual/en/iteratoraggregate.getiterator.php

I did found this trying to fix deprecation notices from phpunit:

Remaining direct deprecation notices (2)

  1x: Return type of DusanKasan\Knapsack\Collection::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

  1x: DusanKasan\Knapsack\Collection implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary)

thanks in advance for taking care,
David

[Ekman]

Just merge and tag this. It breaks absolutely nothing. Don't overthink it.

[Radiergummi]

+1, Please merge the PR. The deprecation notice reported by @davidmpaz is also hugely annoying, this is what my logs look like right now every time a collection is used:

[2022-06-25T09:31:46.998868+00:00] php.INFO: Deprecated: Return type of
DusanKasan\Knapsack\Collection::getIterator() should either be compatible with
IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute
should be used to temporarily suppress the notice {"exception":"[object]
(ErrorException(code: 0): Deprecated: Return type of
DusanKasan\\Knapsack\\Collection::getIterator() should either be compatible with
IteratorAggregate::getIterator(): Traversable, or the #[\\ReturnTypeWillChange] attribute
should be used to temporarily suppress the notice at
/app/vendor/dusank/knapsack/src/Collection.php:98)"} []
[2022-06-25T09:31:46.999002+00:00] php.INFO: Deprecated: DusanKasan\Knapsack\Collection
implements the Serializable interface, which is deprecated. Implement __serialize() and
__unserialize() instead (or in addition, if support for old PHP versions is necessary)
{"exception":"[object] (ErrorException(code: 0): Deprecated:
DusanKasan\\Knapsack\\Collection implements the Serializable interface, which is deprecated.
Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP
versions is necessary) at /app/vendor/dusank/knapsack/src/Collection.php:11)"} []
[2022-06-25T09:31:47.000524+00:00] php.INFO: User Deprecated: Method
"IteratorAggregate::getIterator()" might add "\Traversable" as a native return type
declaration in the future. Do the same in implementation "DusanKasan\Knapsack\Collection"
now to avoid errors or add an explicit @return annotation to suppress this message.
{"exception":"[object] (ErrorException(code: 0): User Deprecated: Method
\"IteratorAggregate::getIterator()\" might add \"\\Traversable\" as a native return type
declaration in the future. Do the same in implementation
\"DusanKasan\\Knapsack\\Collection\" now to avoid errors or add an explicit @return
annotation to suppress this message. at /app/vendor/symfony/error
handler/DebugClassLoader.php:328)"} []

[Stollie]

I quickly ran a unit test with

@martinvenus version with these commits. https://github.com/martinvenus/Knapsack

It fixes some notifications, but this one is still left

  1x: Method "IteratorAggregate::getIterator()" might add "\Traversable" as a native return type declaration in the future. Do the same in implementation "DusanKasan\Knapsack\Collection" now to avoid errors or add an explicit @return annotation to suppress this message.

Would be nice to see a release with the notices fixes. Thanks.

[davidmpaz]

@martinvenus Could we keep please the original author name?

In my humble opinion, unless this package is declared as abandoned or archived, Information like this should be kept the same.

[martinvenus]

I have lost hope that this will be merged someday and have made other changes to my repository. It's been a few years since I did this PR.

[davidmpaz]

I agree with you. But maybe this changed line is preventing the author to merge it, we do not know :)

In your fork main branch, you can change the name if desired, if a Pull Request is made against this repository which is considered still mainstream, I would advise otherwise. I guess at some point people will start using your fork if no reaction is noticed, like normally happened with many other open source projects that gets unattended in time.

Thanks for answering and regards!
David