-
Notifications
You must be signed in to change notification settings - Fork 45
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
Support PHP 8 #169
Merged
Merged
Support PHP 8 #169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edudobay
force-pushed
the
php8
branch
2 times, most recently
from
November 1, 2020 20:20
7c10414
to
491318f
Compare
- PHPUnit 9.3 is required to support PHP 8. PHPUnit 8.x will not be updated to support PHP 8 and will become unsupported in February 2021. - Methods that were removed from 9.x were migrated. - Prophecy will no longer be bundled with PHPUnit, but the replacement phpspec/prophecy-phpunit required PHP 7.3. To keep PHP 7.2 compatibility, weirdan/prophecy-shim is used as a "migration layer". Note: 2.0.1 was tagged incorrectly, hence it is avoided in the version constraint. - http-factory-tests upgraded to 0.7 (allows installing on PHP 8) - In PHP 8, fopen throws ValueError instead of triggering an error. In PHP 7, that exception does not exist and PHPStan fails. Hence the symfony/polyfill-php80 dependency was brought. - Some dependencies cannot be installed on PHP 8 yet (namely, fig/http-message-util and php-http/psr7-integration-tests). "--ignore-platform-reqs" was temporarily added to the CI build with PHP 'nightly'.
All set for this one as well! |
Fantastic work again @edudobay! Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi! I'm interested in contributing to make Slim 4 compatible with PHP 8. I've written some for the
Slim
core repo but a big part of this is about dependencies, so I figured I'd start from the "subcomponents" like this one.Blockers:all set!As I'm new to open-source contributions and I've touched a lot of points that are not purely technical but have a significant 'policy/social' side, I'd like to know if the decisions I've taken so far are aligned with the views of the Slim maintainers — mostly regarding dependencies and test warnings.
Coverage: is it OK to use coverage ignore annotations for the catch-block that only gets executed in PHP 8? I can't figure out a way to solve this because different code paths will be executed in PHP 7/8 and I see no way to annotate this specifically for coverage analysis.
About new dependencies: I don't know if I've reached a good balance in the "minimizing dependencies" vs. "not reinventing the wheel" tradeoff. I upgraded one dependency and pulled two new ones:
symfony/polyfill-php80
. Is this ok?Test suite warnings: php-http/psr7-integration-tests uses assertions (
assertRegExp
andassertFileNotExists
) deprecated in PHPUnit 9.1 but unavailable on PHPUnit 8 (still required for PHP 7.2 support). Do we need to fix that or is it acceptable to leave these warnings for now (11 warnings)? Of course I would prefer to remove the warnings, but this depends on other projects — if this is important, I'll see what I can do there too.