chris-doehring commented 4 years ago

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 commented 4 years ago

Codecov Report

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

@@             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.

codecov-io commented 4 years ago

Codecov Report

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

@@             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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

chris-doehring commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

pimjansen commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

ollieread commented 4 years ago

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

chris-doehring commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

FYI, this was the content of the email:

dunglas commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

pimjansen commented 4 years ago

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 commented 4 years ago

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

GrahamCampbell commented 4 years ago

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.

driesvints commented 4 years ago

@GrahamCampbell sure

pimjansen commented 4 years ago

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.

That is how you look at it. If i am an user with an app on php 5 and i target the lib at 1.9 i will receive 1.10 and thus will break my installation.

So therefor it is a breaking change and we should keep supporting it.

GrahamCampbell commented 4 years ago

That is how you look at it. If i am an user with an app on php 5 and i target the lib at 1.9 i will receive 1.10 and thus will break my installation.

No you won't. Composer knows that your old PHP version cannot use a package marked as not compatible with it. Composer will install 1.9.x for you instead.

driesvints commented 4 years ago

@pimjansen updating dependencies in stable releases isn't a breaking change: https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

pimjansen commented 4 years ago

Ok missed that one though thanks. Let me review it tomorrow to see how we can ensure it is release before the actual PHP release itself as 1.10

fzaninotto commented 4 years ago

Hi there! I'm not sure I understand where we are on this PR.

I understood that phpUnit was the main blocker for supporting php8

I understood that @dunglas' trick allows us to use any phpUnit version with Faker.

So I understand that we can make this lib compatible with php8 without dropping support for php5.

Or am I missing something?

chris-doehring commented 4 years ago

In case you want to support php 5, 7 and 8 at the same time, you must drop support for php 5.3. phpunit-bridge uses traits to fake the void return type for those setUp and tearDown methods, which has been introduced in phpunit 8. But that is only a theoretical assumption. I have to change all tests in order to see if everything really works for >=5.4.

What is your point of view regarding dropping the support for php 5, taking the previous comments of different persons into account?

pimjansen commented 4 years ago

@localheinz can you also have a look to push this for 1.10. If not we will not have proper PHP8 support and an huge inflow on bugs.

localheinz commented 4 years ago

@pimjansen

I wouldn't rush support for PHP 8.0, especially taking into account things like named arguments.

pimjansen commented 4 years ago

I wouldn't rush support for PHP 8.0, especially taking into account things like named arguments.

I don't really agree though. The named arguments do not change how the application works so its an easy win. For the next major we can use all the benefits of PHP 7.3 though

localheinz commented 4 years ago

@pimjansen

I don't really agree though. The named arguments do not change how the application works so its an easy win. For the next major we can use all the benefits of PHP 7.3 though

Named arguments prevent us from renaming arguments without a new major release.

fzaninotto commented 4 years ago

So it seems there are 2 PRs in this one:

makes Faker compatible with php8 by changing how phpUnit works. This requires increasing the minimal PHP version to PHP 5.4. I think we can live with that in master
Increases the minimal PHP version to 7.X. This will enable new features, but possibly in a non backwards compatible way. Let's do that in next

driesvints commented 4 years ago

I think we can live with just PHP 8 support in the current stable release for Laravel which is the most important right now. Preferable in Faker 1.10 if that seems reasonable to you all. PHP 8 is fast approaching now and more people are beginning to test on it.

chris-doehring commented 4 years ago

I personally disagree with the choice of having to support three php versions at once, but that is up to the maintainer to decide. I will do my best in making it compatible to php ^5.4 || ^7.0 || ^8.0, so I will change this PR according to the first point @fzaninotto suggested. I will keep you updated on the progress.

I hope that we will get a faker with php 8 support before the php GA release. Faker is a testing tool and there many projects depending on it which already want to upgrade in preparation for the release.

localheinz commented 4 years ago

@chris-doehring

Can you point this against the next branch, please?

chris-doehring commented 4 years ago

@localheinz and what about the php 8 support in master? This branch is currently confliced to 40 files in next. In my opinion it makes more sense to continue to work on this branch, make it work with php 5 and then create a new PR with new branch based on next.

localheinz commented 4 years ago

@chris-doehring

If you do not want to do it, I will.

chris-doehring commented 4 years ago

@localheinz I just don't understand what you want to achieve by doing that. It would be better if you could explain what your plan is. At the end I'm only an author of a PR trying to help this project. Feel free to do it.

localheinz commented 4 years ago

@chris-doehring @GrahamCampbell @driesvints @dunglas @fzaninotto @ollieread @pimjansen

Since I changed the target branch from master to next and applied a few changes, would you like to take another look?

As far as I am concerned, I wouldn't mind cutting a 1.10.0 release from the next branch.

What do you think? Will this work for the Laravel community and everyone else interested in a timely release for PHP 8.0?

pimjansen commented 4 years ago

@localheinz what will we do with the fixes in master that are done for 1.9.2? Can we try to merge those in vnext as well?

localheinz commented 4 years ago

@pimjansen

@localheinz what will we do with the fixes in master that are done for 1.9.2? Can we try to merge those in vnext as well?

Definitely out of scope for this pull request!

We can sort this out later. 👍

pimjansen commented 4 years ago

@pimjansen

@localheinz what will we do with the fixes in master that are done for 1.9.2? Can we try to merge those in vnext as well?

Definitely out of scope for this pull request!

We can sort this out later. 👍

Totally right, was just figuring out the wat to go with the vnext branch thats why

fzaninotto commented 4 years ago

@localheinz if you disagree with the OP, discuss in the PR, but don't push on their PR without their consent, this is rude.

Besides, you an I posted contradictory messages in this discussion regarding the base branch. Before acting, we must agree on the path to follow.

localheinz commented 4 years ago

@fzaninotto

My intention was to help move forward with allowing the installation of fzaninotto/faker on PHP 8.0.

Option A

As far as I can see, if we want to allow the installation of fzaninotto/faker on PHP 8.0 while running tests on PHP 8.0 as well, we need to drop support for

PHP 5.3
PHP 5.4
PHP 5.5
PHP 5.6
PHP 7.0
PHP 7.1

That means that we will end up with the same changes in master that we have already made in next.

Option B

Alternatively, we could run

$ composer require php:"^5.3 || ^7.0 || ^8.0"

on current master, and never run a test on PHP 8.0. If that's ok for everyone involved, that certainly is a viable option.

Opinion

I don't see any value in supporting otherwise unsupported PHP versions.

If anyone wants to use fzaninotto/faker on

PHP 5.3
PHP 5.4
PHP 5.5
PHP 5.6
PHP 7.0
PHP 7.1

they can do so with any of the versions that are currently available.

Here's what I would have done, if I had full control over the repository:

create a 1.9 branch off 1.9.1and keep it around for bug fixes only (this is what we have done, it is our current master)
create a master branch at the same time, make it the default branch, and start modernizing it aggressively (this is what we have done, it is our current next), while keeping backwards-incompatible changes as little as possible

I don't think it makes sense to provide support for PHP 8.0 in master (i.e., 1.9) with Option A.

localheinz commented 4 years ago

@fzaninotto

What I mean is this: we can not move forward without making any changes. So either we make the changes and move forward, or we keep everything as it is. Then either someone else will fork the repository and make the changes there, or after enough pressure has built up to hand over the repository, then someone else will make the changes.

Why not make the changes here?

driesvints commented 4 years ago

What do you think? Will this work for the Laravel community and everyone else interested in a timely release for PHP 8.0?

For Laravel we'll need Faker to run on PHP 7.2, 7.3, 7.4 & 8.0 (preferable in Faker v1.10 f that's possible). So I think the current state of the PR reflects that? /cc @GrahamCampbell

fzaninotto / Faker

Enhancement: Add support for PHP 8.0 #2063

Codecov Report

Codecov Report

Option A

Option B

Opinion