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

PHP 8 Support #7

Closed
acelaya opened this issue Nov 30, 2020 · 11 comments · Fixed by #9
Closed

PHP 8 Support #7

acelaya opened this issue Nov 30, 2020 · 11 comments · Fixed by #9

Comments

@acelaya
Copy link
Contributor

acelaya commented Nov 30, 2020

Hey!

I was creating a PR here to add support for PHP 8 on the project, in preparation to do the same on nikolaposa/monolog-factory, but I see this project has a dev dependency on php-cs-fixer, which does not support PHP 8 and doesn't seem to be something close in time.

Since you just seem to be checking styles against PSR-2, would you be open to change it by PHP_CodeSniffer which already supports PHP8?

@nikolaposa
Copy link
Owner

Hm, perhaps I can simply replace php-cs-fixer dependency for a binary. I'll think about it these days, I'll consider PHP_CodeSniffer as well.

@acelaya
Copy link
Contributor Author

acelaya commented Dec 1, 2020

I tried doing an install with --ignore-platform-reqs, but then the tool was still checking the PHP version and failing at runtime, so I don't know if replacing it with a binary would solve the problem.

@nikolaposa
Copy link
Owner

What I meant is:

  1. remove CS Fixer dev dependncy entirely
  2. download binary of a fixed CS Fixer version
  3. put binary under version control
  4. optionally change aliases in composer.json (if they exist) to use this binary

I don't like the fact that this approach implies putting 3rd party tool under version control, but I've seen some projects doing this for PHPUnit as well.

@nikolaposa
Copy link
Owner

I remember this interesting discussion on the subject: https://twitter.com/infection_php/status/969453737428357121.

@acelaya
Copy link
Contributor Author

acelaya commented Dec 1, 2020

Oh, yeah, I think I understood it, but I didn't notice the cs checks are done only in one PHP version, which allows doing it in the smallest supported version only, so that approach would work 🙂

@acelaya
Copy link
Contributor Author

acelaya commented Dec 12, 2020

Hey!

I was revisiting this and I saw you already created this PR to add PHP8 support to another package https://github.com/nikolaposa/version/pull/34/files

Do you want me to contribute pull requests to this project to:

  • Move from Travis to github actions, adding PHP 7.3 and 7.4 support in the process. I can use the workflow from nikolaposa/version as template for this repo.
  • Once that is merged, add PHP 8 support to this project, using the same approach you used on versions one.

I can also do the same on the monolog-factory package, if you are ok with it.

@nikolaposa
Copy link
Owner

nikolaposa commented Dec 12, 2020

Migrating from Travis to GitHub Actions would be amazing. 🙏🏻

As for PHP CS Fixer issues, I'm still having a dilemma what to do about dev dependencies in general, but I think we can now simply update it to a new version, as it seems that they've finally added PHP 8 support: ergebnis/php-cs-fixer-config#265.

I look forward to your contributions, thank you!

@acelaya
Copy link
Contributor Author

acelaya commented Dec 12, 2020

I think we can now simply update it to a new version, as it seems that they've finally added PHP 8 support: ergebnis/php-cs-fixer-config#265.

That simplifies things a lot 🙂

I'll start with the first PR then 💪🏼

@acelaya
Copy link
Contributor Author

acelaya commented Dec 12, 2020

I have just created the PR that adds Github Actions and support for PHP 8.

I have also removed support for PHP 7.0 and 7.1, which is something that we didn't discuss. If you want to keep support for those versions, let me know and I'll try to get it back.

BTW, I'm not sure why, but the workflow is not getting run. Maybe it doesn't work until it is merged? 🤔

@nikolaposa
Copy link
Owner

I'm not sure why, but the workflow is not getting run. Maybe it doesn't work until it is merged?

Yep, I believe that's the case.

@acelaya
Copy link
Contributor Author

acelaya commented Dec 12, 2020

I have just created the second PR that bumps dependencies #9

Having Github Actions simplifies the process 🙂

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 a pull request may close this issue.

2 participants