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

composer: allow php ^8.0 #23

Merged
merged 6 commits into from
Nov 30, 2020
Merged

composer: allow php ^8.0 #23

merged 6 commits into from
Nov 30, 2020

Conversation

solcik
Copy link
Contributor

@solcik solcik commented Nov 27, 2020

closes #22

Should I send also migration from Travis to GitHub actions? Since Travis is ultra-slow these days and obviously does not have PHP 8.0 ready, GitHub actions are far better nowadays.

@solcik
Copy link
Contributor Author

solcik commented Nov 28, 2020

@oscarotero this one is also ready

You can see it here: https://github.com/solcik/utils/runs/1467297889

It is also prepared for https://coveralls.io/

  • github actions
  • phpunit upgrade
  • phpstan added
  • dependencies fixed lowest
  • PHP ^7.3 || ^8.0

@solcik
Copy link
Contributor Author

solcik commented Nov 29, 2020

@oscarotero @shadowhand @ecolinet @eusonlito

I am ready here for now.
I am using these middlewares mentioned in this PR.
I hope you are going to like my work done here.
I will be glad for any feedback given.
I might find more time - to do the others also.

@franzliedke
Copy link

Why does PHP 7.2 support need to be dropped in order to support PHP 8?

@solcik
Copy link
Contributor Author

solcik commented Nov 29, 2020

It is up to maintainers of course, but from my point of view 7.2 is in EOL. Current versions were used a long time without any update, so they are stable and can be used with 7.2 still.

@franzliedke
Copy link

This is a low-level library, used by many other libraries and applications. This change would prohibit these users from either

  • supporting PHP 8 alongside their currently supported versions (if those include 7.2), or
  • building on top of this library.

Supporting PHP 8 is great and obviously necessary. But let's not couple that with other, unrelated changes, IMHO!?

@oscarotero
Copy link
Member

@solcik Thanks for this huge contribution. I have some comments:

Removing support for 7.2 is a breaking change, and it requires to be released as a major version. I don't want to do this for now (and I don't see any reason to remove 7.2), so I think we should maintain 7.2.

I also see a lot of stuff in the GA workflow (qa, test, coverage...) I don't now if we need this amount of tooling, I'd like to keep things simple. Anyway, it's a great contribution, I can modify it and reuse for the other middlewares packages.

There are other unrelated changes like replacing $this-> with self:: in the test cases that I don't know what for.

@solcik
Copy link
Contributor Author

solcik commented Nov 29, 2020

@oscarotero it is up to you, if you want, I am going to revert it to 7.2. (But it is not a breaking change, since composer would not install it as soon as somebody is on 7.2)

https://www.php.net/supported-versions.php

I think that tooling (phpstan, phpunit, coverage) is simple enough and is there for contributions - best UX. These are dev dependencies.

Upgraded phpunit - these methods are static. Therefore self instead of this.

@oscarotero oscarotero merged commit ec61a52 into middlewares:master Nov 30, 2020
@oscarotero
Copy link
Member

Ok, thanks for your great contribution. I'll recover the 7.2 support and release a new version.

@solcik solcik deleted the patch-1 branch November 30, 2020 19:49
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.

Support PHP 8
3 participants