-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Update to require PHP 7.1+ #175
Update to require PHP 7.1+ #175
Conversation
1043242
to
92d95c7
Compare
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.
Thanks for working on this @WyriHaximus, I added a few remarks down below and I also went through the project and noticed a few things we should add to this PR. Here are my findings and thoughts:
-
First of all, I think my suggested changes will make this PR a lot bigger, so maybe it's a good idea to split the last commit into two. This way the PR is a bit more overseeable. Could be something like:
- Commit 2: "Update PHP language syntax and remove legacy workarounds" (including all the array stuff)
- Commit 3: "Update test suite and remove legacy PHPUnit workarounds" (PHPUnit changes + legacy loop extensions in tests)
-
We have a lot of workaround functions inside the
TestCase.php
that were necessary for PHPUnit < 7.5. Tried this out and I think we can get rid of all of them (regarding thesetExpectedException()
,assertContainsString()
,assertContainsStringIgnoringCase()
andassertSameIgnoringCase()
) -
There are 3 places in our test suite where we skip tests for HHVM, we should also remove them (reagarding the
DuplexResourceStreamTest
,WritableResourceStreamTest
andReadableResourceStreamTest
) -
Inside the
DuplexResourceStream
andReadableResourceStrem
we have legacy PHP 5 workarounds at the bottom which can be removed in my opinion. -
In line 90 inside the
FunctionalInternetTest
class is a comment describing a bug in PHP < 7.1.4 and PHP < 7.0.18. The part for 7.1.4 should stay, but I think we can remove the part for PHP < 7.0.18.
That's it for now 👍
e4313ea
to
1f68392
Compare
f699f29
to
8bade19
Compare
Thanks for the pointers @SimonFrings, updated the PR according to them. |
85b4fe8
to
d23f22a
Compare
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.
Went through everything, looks good to me. Also nice that you caught a few additional things that I didn't even mention 👍
65f7f6a
to
19a7e3d
Compare
Update one small nitpick, and had to bump the macos job a few times until it passed |
5f6dfd1
to
2866134
Compare
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.
@WyriHaximus Thank you for looking into this and for your patience! I've added some minor remarks only, otherwise love the direction!
1d047d5
to
2f70d54
Compare
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.
@WyriHaximus Went over your changes together with @SimonFrings and addressed all outstanding issues, great work, now let's get this shipped!
For reference: Link to diff between original 1d047d5 and new 2f70d54: https://gist.github.com/clue/7e6012fecc995dee201eb3aae600bcfb)
2f70d54
to
1bb9595
Compare
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.
Went over all changes made, looks good to me so let's get this in 👍
This changeset updates the project to require PHP 7.1+ and drop legacy PHP < 7.1 and HHVM as discussed in #173. I'm marking this as a BC break for anybody still stuck on very old PHP versions, but there's little chance this will affect any current projects otherwise.
This PR aims to contain the minimal changeset to update the PHP version requirement only. Follow-up PRs will update our APIs to leverage newer language features.
Builds on top of #172, #174 and others
Refs reactphp/cache#58