search

php-vcr / phpunit-testlistener-vcr

Integrates PHPUnit with PHP-VCR.
MIT License
61 stars 50 forks source link

PHPUnit as a dependency #31

Open uuf6429 opened 6 years ago

uuf6429 commented 6 years ago

Since this project is for PHPUnit and uses PHPUnit classes, doesn't it explicitly depend on PHPUnit? In particular, shouldn't it depend on a specific PHPUnit version that is compatible?

This causes an issue when trying to support multiple versions of PHPUnit. This package does not downgrade to the version that works with PHPUnit.

renatomefi commented 6 years ago

Hello @uuf6429, I took the freedom to answer directly the mentioned issue! Hope it helps

uuf6429 commented 6 years ago

Thanks! 3.1 would work, but only because of the common dependency on PHP. We can't use 3.0 for PHP 7.0 since it doesn't require it.

renatomefi commented 6 years ago

If you have ^3.0 it will automatically solve it, by matching all the dependent versions

uuf6429 commented 6 years ago

Sorry to persist, but ^3.0 doesn't have any requirements... which is incorrect (same with any version, actually). As it is right now, this package claims to be compatible with any PHPUnit version, even unreleased future ones...

renatomefi commented 6 years ago

This library only depends on TestListener (https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/TestListener.php) and indeed "saying" it's compatible with all versions. Which can be improved for sure. The real phpunit version limitations are coming from php-vcr/php-vcr and not here.

I'd say we could add a constraint saying it's compatible with php unit 5,6,7. But in the end you'll be limited to the php-vcr one! Is that what you're looking for?

uuf6429 commented 6 years ago

This library only depends on TestListener...

To me that's a clear dependency. If someone installs this project but forgets to install PHPUnit, s/he ends up with a class extending and code referencing non-existent classes.

The real phpunit version limitations are coming from php-vcr/php-vcr and not here.

I do not understand your point here... php-vcr does not depend on phpunit to function, it uses phpunit only for tests. Right now the only PHPUnit version constraint comes from the user.

I'd say we could add a constraint saying it's compatible with php unit 5,6,7.

That's the best case scenario. Right now, it doesn't even suggest that you should have PHPUnit installed, nor does it say that it conflicts with older PHPUnit.

^3.1 works somewhat by coincidence: it relies on the fact that both PHPUnit, this project and the linked project depend on a specific PHP version. To be honest, I'd say this project shouldn't even care about PHP version... it should follow the same PHP version as the one PHPUnit and php-vcr (which you want to target) follow.

renatomefi commented 6 years ago

Yes you're right! php-vcr doesn't depend on it :man_facepalming:

The 3.1 was done on purpose to provide an easy update path for people using ^3.0, I also agree it isn't the best approach, check more here: https://github.com/php-vcr/phpunit-testlistener-vcr/pull/26

Please feel free to open a PR providing the phpunit dependency, luckily there's no conflicts so far since the mentioned interface remains the same for a long time

uuf6429 commented 6 years ago

No problem! I'll see if I can get this right for all major versions. Worst case we might end up with a few patch releases.

Edit: I've checked every notable version for compatibility with PHPUnit and PHP, here's the results:

tag pu4 pu5 pu6 pu7 php56 php70 php71 fix
1.0(.7) ✔️ ✔️ ❌ ❌ ✔️ ✔️ ✔️ too old?
1.1(.7) ✔️ ✔️ ❌ ❌ ✔️ ✔️ ✔️ 1.1.8 -
2.0(.0) ✔️ ✔️ ❌ ❌ ✔️ ✔️ ✔️ 2.0.1 -
3.0(.0) ❌ ❌ ✔️ ❌ ✔️ ✔️ ✔️ 3.0.1 #33
3.1(.0) ❌ ❌ ❌ ✔️ ❌ ✔️ ✔️ 3.1.1 #32
3.2(.1) ❌ ❌ ❌ ✔️ ❌ ✔️ ✔️ 3.2.2 #34

Note that the PHP checks are only about syntax, I didn't actually run the tests.

uuf6429 commented 6 years ago

@renatomefi I'd like to have two branches, one called 1.1 (branching from tag 1.1.7) and one 2.0 (branching from tag 2.0.0). When done, I can create my pull requests for both.

ekeyte commented 6 years ago

@uuf6429 @renatomefi Hey there, just following up on this issue. Any updates here?

uuf6429 commented 6 years ago

@ekeyte I'm still waiting for the PRs (linked above) to be merged+released and for branches to the old versions to be created. I literally can't do anything more from my side.

ekeyte commented 5 years ago

@uuf6429 @renatomefi Hey guys, are there any updates on this? This is holding us back from moving some big projects to 7.2. Anything I can do to help with this?

uuf6429 commented 5 years ago

I literally can't do anything more from my side.

Still applies..

ekeyte commented 5 years ago

I literally can't do anything more from my side.

Still applies..

I know, bud. :(

@renatomefi: At your encouragement, @uuf6429 has opened a PR. Is there something holding you back from merging it that I could assist with? We'd love to see this resolved soon. Many thanks!