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

Support PHP 8 #169

Merged
merged 3 commits into from
Nov 28, 2020
Merged

Support PHP 8 #169

merged 3 commits into from
Nov 28, 2020

Conversation

edudobay
Copy link
Contributor

@edudobay edudobay commented Oct 10, 2020

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:

  • It was necessary to polyfill \ValueError to pass the PHPStan checks. I did this with symfony/polyfill-php80. Is this ok?
  • I was having some trouble with PHPUnit 9 deprecating Prophecy from its core until I found a ready solution for that: weirdan/prophecy-shim. Is it OK to pull this dependency (dev-only)?

Test suite warnings: php-http/psr7-integration-tests uses assertions (assertRegExp and assertFileNotExists) 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.

@coveralls
Copy link

coveralls commented Oct 10, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling aaf88e8 on edudobay:php8 into dea7736 on slimphp:master.

@edudobay edudobay mentioned this pull request Nov 1, 2020
5 tasks
@edudobay edudobay mentioned this pull request Nov 1, 2020
1 task
@edudobay edudobay marked this pull request as ready for review November 1, 2020 21:22
@edudobay edudobay changed the title Draft: PHP 8 support PHP 8 support Nov 2, 2020
@edudobay edudobay changed the title PHP 8 support Support PHP 8 Nov 2, 2020
- 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'.
@edudobay
Copy link
Contributor Author

All set for this one as well!

@l0gicgate l0gicgate added this to the 1.3 milestone Nov 28, 2020
@l0gicgate
Copy link
Member

Fantastic work again @edudobay! Thank you!

@l0gicgate l0gicgate merged commit 235d2e5 into slimphp:master Nov 28, 2020
@edudobay edudobay deleted the php8 branch December 1, 2020 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants