Knapsack icon indicating copy to clipboard operation
Knapsack copied to clipboard

PHP 8.1 compatibility

Open martinvenus opened this issue 2 years ago • 7 comments

martinvenus avatar Nov 24 '21 12:11 martinvenus

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

BenMorel avatar Dec 04 '21 17:12 BenMorel

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

martinvenus avatar Dec 10 '21 13:12 martinvenus

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

DusanKasan avatar Jan 30 '22 13:01 DusanKasan

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

davidmpaz avatar Feb 11 '22 17:02 davidmpaz

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

Ekman avatar Apr 28 '22 08:04 Ekman

+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)"} []

Radiergummi avatar Jun 25 '22 09:06 Radiergummi

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.

Stollie avatar Jun 29 '23 11:06 Stollie