Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Enhancement: Add support for PHP 8.0 #2063

Closed
wants to merge 3 commits into from
Closed

Enhancement: Add support for PHP 8.0 #2063

wants to merge 3 commits into from

Conversation

chris-doehring
Copy link
Contributor

@chris-doehring chris-doehring commented Oct 2, 2020

ThiHey there,

as you may know, php 8 will be released on the 26th of November 2020. Unfortunately, this package does not allow php 8, so I simply tried to change the composer version constraint to >=5.3.3, but then no unit tests would work under php 8, as only phpunit ^9.0 would allow php 8. Setting something like ^5.3.3 || ^7.0 || ^8.0 is no option either, as phpunit 8 introduced some backward incompatible changes which would require different tests for different phpunit and php versions.

So the logical conclusion for me was to set the same version constraint as in phpunit 9 (>=7.3). This also means, that a new major release of Faker (as of now v2.0.0) would be required, as it now drops support for older php versions. I know that's a hard step to take, and I know that there is indeed an ongoing planning and working going on for the "next Faker version", but as far as I can tell, these works are not going to be finished before php 8 has been released. And this package is included in many projects require-dev section. It is required for those projects to have a php 8 compatible Faker version in order to upgrade to php 8. This is the reason why I created this pull request for the master branch, instead of the "next" release branch.

I was able to get everything running for php 7.3, 7.4 and 8.0. What’s your thought on this?

@codecov-commenter
Copy link

Codecov Report

Merging #2063 into master will decrease coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2063      +/-   ##
============================================
- Coverage     56.56%   56.10%   -0.47%     
+ Complexity     2068     1966     -102     
============================================
  Files           306      306              
  Lines          4849     4880      +31     
============================================
- Hits           2743     2738       -5     
- Misses         2106     2142      +36     
Impacted Files Coverage Δ Complexity Δ
src/autoload.php 0.00% <0.00%> (-75.00%) 0.00% <0.00%> (ø%)
src/Faker/Provider/ne_NP/Person.php 0.00% <0.00%> (-50.00%) 2.00% <0.00%> (ø%)
src/Faker/Provider/es_AR/Address.php 25.00% <0.00%> (-50.00%) 4.00% <0.00%> (ø%)
src/Faker/Provider/id_ID/Company.php 50.00% <0.00%> (-50.00%) 2.00% <0.00%> (ø%)
src/Faker/Provider/ko_KR/Address.php 60.00% <0.00%> (-40.00%) 5.00% <0.00%> (ø%)
src/Faker/Provider/sr_Cyrl_RS/Address.php 66.66% <0.00%> (-33.34%) 3.00% <0.00%> (ø%)
src/Faker/Provider/ka_GE/Address.php 57.14% <0.00%> (-28.58%) 7.00% <0.00%> (ø%)
src/Faker/Provider/uk_UA/Company.php 28.57% <0.00%> (-28.58%) 3.00% <0.00%> (ø%)
src/Faker/Provider/en_AU/Address.php 75.00% <0.00%> (-25.00%) 4.00% <0.00%> (ø%)
src/Faker/Provider/en_ZA/Address.php 50.00% <0.00%> (-25.00%) 4.00% <0.00%> (ø%)
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5337ce5...9543b24. Read the comment docs.

@chris-doehring chris-doehring marked this pull request as ready for review October 2, 2020 14:50
@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #2063 into next will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               next    #2063      +/-   ##
============================================
+ Coverage     70.51%   70.69%   +0.18%     
  Complexity     1599     1599              
============================================
  Files           287      287              
  Lines          3863     3863              
============================================
+ Hits           2724     2731       +7     
+ Misses         1139     1132       -7     
Impacted Files Coverage Δ Complexity Δ
src/Faker/Provider/de_AT/Person.php 0.00% <0.00%> (-100.00%) 1.00% <0.00%> (ø%)
src/Faker/Provider/es_PE/Person.php 50.00% <0.00%> (-50.00%) 2.00% <0.00%> (ø%)
src/Faker/Provider/ko_KR/Company.php 50.00% <0.00%> (-50.00%) 2.00% <0.00%> (ø%)
src/Faker/Provider/th_TH/Address.php 33.33% <0.00%> (-33.34%) 3.00% <0.00%> (ø%)
src/Faker/Provider/sr_Latn_RS/Address.php 66.66% <0.00%> (-33.34%) 3.00% <0.00%> (ø%)
src/Faker/Provider/hu_HU/Address.php 30.76% <0.00%> (-30.77%) 6.00% <0.00%> (ø%)
src/Faker/Provider/es_VE/Person.php 57.14% <0.00%> (-28.58%) 3.00% <0.00%> (ø%)
src/Faker/Provider/vi_VN/Address.php 73.68% <0.00%> (-26.32%) 8.00% <0.00%> (ø%)
src/Faker/Provider/nb_NO/Address.php 50.00% <0.00%> (-25.00%) 4.00% <0.00%> (ø%)
src/Faker/Provider/en_HK/Address.php 55.55% <0.00%> (-22.23%) 9.00% <0.00%> (ø%)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb0a6d4...69d222c. Read the comment docs.

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Oct 10, 2020

Laravel 6 LTS needs PHP 7.2 support to stay around. You should be able to support PHPUnit 8.5 in parallel with 9.x.

@GrahamCampbell
Copy link
Contributor

Let me know if you need a hand. Also, dropping old PHP is not a BC break, and could be done as 1.10.0.

@chris-doehring
Copy link
Contributor Author

Thanks for your feedback, @GrahamCampbell! I changed it locally and it looks good, but I have one problem to solve: there are a lot of regex assertions in the tests and until now assertRegExp has been used, which also exists in phpunit ^9.0 but throws deprecations as warnings as it has been replaced by assertMatchesRegularExpression, but this method does not exist in phpunit ^8.0. I will try to create a custom TestCase class which is overloading the original phpunit class where I will solve that compatibility issue. I will leave feedback here as soon as I am done.

@chris-doehring
Copy link
Contributor Author

I was able to change it as described and I was even able to support php 7.1 as well, but I can't get lower than this. Regarding what the new version will be: I guess that’s up to the maintainer to decide. I just hope that there will be a php 8 compatible Faker version soon.

@GrahamCampbell
Copy link
Contributor

In the event of the maintainer being long-term unavailable, I think it's worth considering making an official fork of this package to allow the package to continue into 2021 and beyond.

@pimjansen
Copy link
Contributor

What is the usecase of dropping old php versions? I do not think it is needed right?

@chris-doehring
Copy link
Contributor Author

It's not really a feature or my intend to drop the support for php >=5.3.3 <7.1 but a necessity in order to use phpunit >=8 and therefor php 8, as those setUp and tearDown methods now need to return void, which is a php 7.1 feature. Otherwise the most tests are not compatible. In order to keep the php versions >=5.3.3 <7.1 supported, many tests need to be rewritten in order to use different approaches so that it's compatible to those older phpunit versions. But to be honest: I have no clue how to do that without creating some messy tests.

@chris-doehring
Copy link
Contributor Author

Maybe I have an idea how to do it, but again, thats usually what one shouldn't do when creating unit tests: I will try to rename all setUp/tearDown methods to something different and make my new TestCase magically call them in the original setUp/tearDown. It won't look nice, but that way we could support php >=5.3.3

@GrahamCampbell
Copy link
Contributor

I don't think dropping old PHP should be an issue. People can still use an older version of faker.

@pimjansen
Copy link
Contributor

I don't think dropping old PHP should be an issue. People can still use an older version of faker.

Dont think @fzaninotto agrees on that statement though

@chris-doehring
Copy link
Contributor Author

@pimjansen That's a shame though. The code base regarding testing will be a mess. I found a way how to support all php versions, but I have to create some backward compatible custom methods in my base test class. I'm in progress of implementing that right now.

@pimjansen
Copy link
Contributor

Yeah lets wait for his reply in that case since i agree that it should be php 8 compatible once its released

@ollieread
Copy link

Any news on this? Anything I can do to help speed things up?

@chris-doehring
Copy link
Contributor Author

We are waiting for feedback of @fzaninotto on this. @GrahamCampbell was so kind and wrote him a mail about the future of this project. So we either get a response or the other possibility still remains: Creating a new forked project with new maintainers.

@ollieread
Copy link

We are waiting for feedback of @fzaninotto on this. @GrahamCampbell was so kind and wrote him a mail about the future of this project. So we either get a response or the other possibility still remains: Creating a new forked project with new maintainers.

Ah fair enough. Should such an effort be required id be happy to help maintain/contribute.

@GrahamCampbell
Copy link
Contributor

FYI, this was the content of the email:

image

@dunglas
Copy link
Contributor

dunglas commented Oct 20, 2020

Regarding the issue with PHPUnit versions, you can use the Symfony PHPUnit Bridge, which handles automatically compatibility with most PHPUnit versions (using black magic). Basically, replace phpunit/phpunit by symfony/phpunit-bridge in composer.json and update the CI to run vendor/bin/simple-phpunit instead of phpunit and this should do the trick.

We've done this recently to support PHP 8 in API Platform and the Negotiation library and it worked like a charm. The tool provides a polyfill for new PHPUnit features, so you can use the new assertions even with old PHPunit versions, no need for backward compatibility hacks in the tests.

@chris-doehring
Copy link
Contributor Author

That was a great hint @dunglas! Thanks a lot! It's indeed black magic, but really nice as it saves a lot of time 😄

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Oct 20, 2020

I see no reason to change this PR. PHP 5 is totally dead now, and we don't need to make any effort to support it. A minimum version of PHP 7.1, going forward, is totally fine.

.gitignore Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@pimjansen
Copy link
Contributor

Can we ensure that the existing php versions stay active? The change as is means it is a breaking change for older users on which we should release a major.

We will do this for future release but not now to make it php 8 compatible.

@driesvints
Copy link

I'd personally drop anything below ^7.2.5. PHP 7.2 is almost EOL: https://www.php.net/supported-versions.php

@GrahamCampbell
Copy link
Contributor

Dropping PHP 5 is not a breaking change, and keeping at ^7.1 || ^8.0 is fine for now. If we want to drop 7.1 to use features from 7.2, then we shouldn't hesitate, but no point in dropping for no reason, when everything works. I am happy with this PR exactly in it's current state.

@negoziator
Copy link

I want to thank you very much for all the work you've done.
I've been using faker heavily through the years.

Thank you! 😎

@GrahamCampbell
Copy link
Contributor

https://github.com/FakerPHP

@ssmusoke
Copy link

@fzaninotto Faker has been invaluable to the community thank you for all you have done.

Your approach to sunsetting the project has also opened a path for handling of open source projects too, thank you for pioneering this too

@hotmeteor
Copy link

Thanks for all your work on this great package, @fzaninotto

@Cannonb4ll
Copy link

Understandable, thank you for your work on this @fzaninotto 💪

@techguydev
Copy link

Thanks a lot for your effort and great work!, thanks for sharing this project with all of us. Best wishes in your endeavors.

@sjelfull
Copy link

Thank you for all the hard work, and good luck!

@taytus
Copy link

taytus commented Oct 27, 2020

Francois, thank you so much for your work.

@lostdesign
Copy link

Even if most of the time I've been actually typing or filling fields in the DB by hand, Faker saved me a lot of time when I used it. Been such a lovely package, thanks for all your effort and love that went into this.

@amitavroy
Copy link

Thank you for your effort... @fzaninotto
I would like to tell you that you package has helped me a lot.
As a member of the community, I guess I respect your decision and wish you luck.
I hope, someone picks it up and continue this wonderful package.

@thijsvdanker
Copy link

Thank you @fzaninotto for all your hard work on this great package 👏👏👏

@lukio3
Copy link

lukio3 commented Oct 27, 2020

Thank you so much @fzaninotto for your work on getting faker to where it is today.

@luigircruz
Copy link

Thank you for the years of hard work that you've put on this package @fzaninotto! You've created a tool that is so beneficial to the PHP community.

@SeoFood
Copy link

SeoFood commented Oct 27, 2020

WoW thanks man for that amazing package 💕

@fgilio
Copy link

fgilio commented Oct 27, 2020

Thanks for all your work, it's been immensely helpful 🙌

@cyrildewit
Copy link

@fzaninotto Thank you for your work!

@Hasnayeen
Copy link

@fzaninotto thank you for all your work!

@Ali-Shaikh
Copy link

@fzaninotto Thank you for your hard work. Wish you the best in your future endeavours 👍

@rogervila
Copy link

Thank you @fzaninotto :)

@next65
Copy link

next65 commented Oct 27, 2020

Thank you @fzaninotto

@fabioi
Copy link

fabioi commented Oct 27, 2020

Thank you a lot, this package helped a lot of people! 🙏

@vikas5914
Copy link

Thank you @fzaninotto

@stelgenhof
Copy link
Contributor

Thank you so much for this great project @fzaninotto !

@damiani
Copy link

damiani commented Oct 27, 2020

Thanks for all your hard work on this @fzaninotto!

@mypromo-ml
Copy link

Thank you for all this @fzaninotto ! All the best for you :-)

@rnleal
Copy link

rnleal commented Oct 27, 2020

Thank you @fzaninotto

@austintoddj
Copy link

Thanks for all the hard work @fzaninotto!

@ceejayoz
Copy link

Faker has been very useful to me and I just wanted to chime in with thanks as well.

@rslhdyt
Copy link

rslhdyt commented Oct 27, 2020

Thanks for all the hard work @fzaninotto

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.