DusanKasan / Knapsack

Collection pipeline library for PHP
http://dusankasan.github.io/Knapsack/
MIT License
536 stars 56 forks source link

PHP 8.1 compatibility #75

Open martinvenus opened 2 years ago

BenMorel commented 2 years ago

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

martinvenus commented 2 years ago

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

DusanKasan commented 2 years ago

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. https://github.com/DusanKasan/Knapsack/pull/63

davidmpaz commented 2 years ago

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 commented 2 years ago

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

Radiergummi commented 2 years ago

+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 commented 1 year ago

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.