Skip to content
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

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Feb 21, 2024

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

@WyriHaximus WyriHaximus added this to the v3.0.0 milestone Feb 21, 2024
@WyriHaximus WyriHaximus marked this pull request as ready for review February 21, 2024 11:46
@WyriHaximus WyriHaximus force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch from 1043242 to 92d95c7 Compare February 21, 2024 15:42
@WyriHaximus WyriHaximus changed the title Update to require PHP 7.1 Update to require PHP 7.1+ Feb 22, 2024
Copy link
Member

@SimonFrings SimonFrings left a 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:

  1. 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)
  2. 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 the setExpectedException(), assertContainsString(), assertContainsStringIgnoringCase() and assertSameIgnoringCase())

  3. There are 3 places in our test suite where we skip tests for HHVM, we should also remove them (reagarding the DuplexResourceStreamTest, WritableResourceStreamTest and ReadableResourceStreamTest)

  4. Inside the DuplexResourceStream and ReadableResourceStrem we have legacy PHP 5 workarounds at the bottom which can be removed in my opinion.

  5. 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 👍

README.md Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
tests/DuplexResourceStreamIntegrationTest.php Outdated Show resolved Hide resolved
tests/DuplexResourceStreamIntegrationTest.php Outdated Show resolved Hide resolved
@WyriHaximus WyriHaximus force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch 2 times, most recently from e4313ea to 1f68392 Compare February 23, 2024 21:06
bpolaszek added a commit to bpolaszek/stream that referenced this pull request Feb 24, 2024
@WyriHaximus WyriHaximus force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch from f699f29 to 8bade19 Compare February 24, 2024 13:46
@WyriHaximus
Copy link
Member Author

Thanks for the pointers @SimonFrings, updated the PR according to them.

@WyriHaximus WyriHaximus force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch 2 times, most recently from 85b4fe8 to d23f22a Compare February 24, 2024 15:16
Copy link
Member

@SimonFrings SimonFrings left a 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 👍

@WyriHaximus
Copy link
Member Author

Update one small nitpick, and had to bump the macos job a few times until it passed

@WyriHaximus WyriHaximus force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch 3 times, most recently from 5f6dfd1 to 2866134 Compare February 28, 2024 06:07
Copy link
Member

@clue clue left a 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!

src/DuplexResourceStream.php Outdated Show resolved Hide resolved
src/DuplexResourceStream.php Outdated Show resolved Hide resolved
src/DuplexResourceStream.php Outdated Show resolved Hide resolved
src/ReadableResourceStream.php Outdated Show resolved Hide resolved
src/ReadableResourceStream.php Outdated Show resolved Hide resolved
tests/ReadableResourceStreamTest.php Outdated Show resolved Hide resolved
tests/ReadableResourceStreamTest.php Outdated Show resolved Hide resolved
tests/ReadableResourceStreamTest.php Outdated Show resolved Hide resolved
tests/ReadableResourceStreamTest.php Outdated Show resolved Hide resolved
tests/ReadableResourceStreamTest.php Outdated Show resolved Hide resolved
@clue clue force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch from 1d047d5 to 2f70d54 Compare May 15, 2024 13:39
Copy link
Member

@clue clue left a 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! :shipit: :shipit: :shipit:

For reference: Link to diff between original 1d047d5 and new 2f70d54: https://gist.github.com/clue/7e6012fecc995dee201eb3aae600bcfb)

@clue clue requested a review from SimonFrings May 15, 2024 13:44
@clue clue force-pushed the 3.x-raise-minimum-php-version-to-7.1PLUS branch from 2f70d54 to 1bb9595 Compare May 15, 2024 14:20
Copy link
Member

@SimonFrings SimonFrings left a 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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants