-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP 8.1 compatibility #75
base: master
Are you sure you want to change the base?
Conversation
@DusanKasan Could you please merge this and tag a relase? Thank you! |
@DusanKasan Is there anything else I can do for this PR to be merged? |
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 |
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 https://www.php.net/manual/en/iteratoraggregate.getiterator.php I did found this trying to fix deprecation notices from phpunit:
thanks in advance for taking care, |
Just merge and tag this. It breaks absolutely nothing. Don't overthink it. |
+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:
|
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
Would be nice to see a release with the notices fixes. Thanks. |
@@ -1,5 +1,5 @@ | |||
{ | |||
"name": "dusank/knapsack", | |||
"name": "martinvenus/knapsack", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
No description provided.