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 #102

Merged
merged 3 commits into from Dec 10, 2020
Merged

Support PHP 8 #102

merged 3 commits into from Dec 10, 2020

Conversation

edudobay
Copy link
Contributor

@edudobay edudobay commented Dec 5, 2020

Fixes the issues raised in #101.

There's one issue about dropping PHP 7.0 support:

  • Add void return type to PHPUnit setUp methods: this makes the syntax incompatible with PHP 7.0

@edudobay edudobay mentioned this pull request Dec 5, 2020
composer.json Outdated
"psr/log": "~1.0",
"react/event-loop": "^1.0 || ^0.5 || ^0.4",
"react/promise": "~2.2"
},
"require-dev": {
"phpunit/phpunit": "~6.5"
"phpunit/phpunit": "^6.5 || ^7.5 || ^8.5 || ^9.3"
Copy link
Contributor

@standa standa Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"phpunit/phpunit": "^6.5 || ^7.5 || ^8.5 || ^9.3"
"phpunit/phpunit": "^8.5 || ^9.3"

@edudobay I think only these two make sense for "php": "^7.1 || ^8.0",: https://phpunit.de/supported-versions.html (or in other words, if this is to support PHP 8)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@standa If PHP 7.1 is still to be supported, I think we need PHPUnit 7 too; Composer won't let us install PHPUnit 8 with PHP 7.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've dropped PHPUnit 6 and pushed a new commit to fix PHP 8 build. What do you think @standa?

@edudobay
Copy link
Contributor Author

edudobay commented Dec 5, 2020

Trying to fix these SSL issues here! It's hard to know what exactly went wrong, so I'm working on making local SSL testing easier as well — I had a hard time getting the SSL tests to run locally.

@standa
Copy link
Contributor

standa commented Dec 7, 2020

@edudobay I tested this PR on our production code with php 8.0 today and it works like a charm!

@edudobay
Copy link
Contributor Author

edudobay commented Dec 7, 2020

Nice to hear that, @standa! I just have to solve this SSL issue in the CI builds — I'll try to reproduce the CI environment on the next few days to see how I can fix this.

@WyriHaximus
Copy link
Collaborator

@standa awesome!

Planning to release v0.4.4 to tomorrow

@edudobay let me know when this is ready, it looks like a breaking change to assigned v0.5.0 to this PR

- Fix method signatures — required parameters after optional arguments
  are deprecated in PHP 8.

- Upgrade PHPUnit, adding void return type to setUp methods; this makes
  the syntax incompatible with PHP 7.0.

- Drop PHP 7.0 and PHPUnit 6 support - PHPUnit 6 support ended on
  2019-02-01 and PHP 7.0 end-of-life was almost at the same time. It
  becomes harder to support PHP 7.0 syntax at the same time as PHP 8.0.

- rabbitmq-server package is now needed for Travis CI, starting with
  Ubuntu Xenial. PHP 8.0 is not available for Ubuntu Trusty in Travis
  CI. https://docs.travis-ci.com/user/database-setup/#rabbitmq

- Fix RabbitMQ config files and permissions for SSL tests in CI
  (behavior seems to have changed across Ubuntu versions).
@edudobay
Copy link
Contributor Author

edudobay commented Dec 8, 2020

@WyriHaximus Yes, now this is ready :) And in fact there's a breaking change, so it also seems to me that it would be better suited for the 0.5.0 release.

@WyriHaximus
Copy link
Collaborator

Ok just release v0.4.4, going to merge this soon and then add some additional workflows before tagging v0.5.0. Want to start keeping a better release message from here on out.

Copy link
Collaborator

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏 👏 👏 Well done! And thank you for putting the time into this 👍

@WyriHaximus WyriHaximus merged commit 9d6be4a into jakubkulhan:master Dec 10, 2020
@edudobay edudobay deleted the php8 branch December 11, 2020 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants