Update LoggerInterfaceTest to be phpunit 5.7+ compatible#52
Conversation
| * as part of their test suite. | ||
| */ | ||
| abstract class LoggerInterfaceTest extends \PHPUnit_Framework_TestCase | ||
| abstract class LoggerInterfaceTest extends \PHPUnit\Framework\TestCase |
There was a problem hiding this comment.
you should add a use statement
|
@stof I made the change. |
|
Please merge |
|
@stof: Could you please merge this change? It makes the work with PHPUnit easier :) |
|
@MyIgel I cannot. I don't have write access to this repo (I'm not a member of FIG btw, as I'm not the one representing the Symfony core team there) |
|
@stof Hm, ok. And who is able to merge this? |
|
https://github.com/orgs/php-fig/people is the list of people making it public that they are member of the organization. |
|
i have no commit rights on the standards, i am just part of the working group for the http client specification. the problem i see is that this breaks with very old versions of phpunit that do not provide the namespaced TestCase. i have seen workaround code for that in some place that uses the class_alias method when the namespaced version does not exist. (would link it, but can't find the example anymore, basically its what does @Seldaek think about this change? maybe @michaelcullum can help? |
|
This is a major breaking change isn't it, since this test class actually does constitute publicly consumable code? |
|
I'd question why this even exists in the first place. |
|
@gsdevme it's explained in the comments of the base test class. |
|
btw, no, we can't merge this because it's a BC break. In hindsight, things like the base test case, the trait, and anything that is not the standard interface itself should've gone in a different |
|
If we minimum require PHP 5.3.3 and PHPUnit 4.8.35, there’s no BC and everyone would be benefited 😁 |
|
another workaround would be to create an alias if the new class does not exist. that should be BC, as nothing here relies on features and its only about the class name (if i understand the problem correctly) |
|
As the namespaced TestCase has been backported to PHPUnit 4.8 (and PHPUnit 4.x and 5.x are EOLed), I'm fine with dropping support for PHPUnit < 4.8.35 (people still using 4.x, for instance to run tests on PHP 5.3, should at least use the latest version of it). |
|
I wasn't aware of the backporting, which improves things certainly, but still, the problem is that this package doesn't even declare a dependency on phpunit (which has been a mistake, in hindsight), so upgrading The point is this package is broken: it uses code of another package without explicitly declaring the dependency, and this PR aggravates the problem. We need to fix the underlying problem first, then safely upgrade the dependency. |
|
Requiring PHPUnit is not feasible though: that way, you would have it as a non-dev requirement just because you need this interface. That's not doable. We could add a suggest in the composer.json, but it's weak and noisy; IMHO we could just add a simple |
I never said it should be a normal req, it should be a dev req, of course, but it's not even that at the moment! |
It's not there because there's no test suite and no CI. Having as a dev requirement would be not meaningful for end users too IMHO... |
Exactly, we can't make clear that this package provides code that can only function with a certain version of a certain dependency, which happens to be something that people would only require for development purposes. This is symptomatic that we're mixing things up. |
|
The original sin was putting tests into this package in the first place. That should not have happened since the tests are not defined by PSR-3. (Even the traits, in hindsight, shouldn't have been part of this package.) If there's no declared dependency anyway, let's just go ahead and move the tests out to a separate package with its own versioning that can be updated/maintained separately. A couple of other specs already have The chances of anyone depending on the tests are small, and in a production situation are basically zero. And the fix for it would be trivial: Add this other package as a require-dev, kxthxbye. Rip that bandaid! |
|
oh wait, that's a testcase for userland implementations or a logger, not for the code in this repo. This should indeed go to a separate package (similar to how we have a shared testsuite for PSR-6 and PSr-16 maintained by some guys). |
|
I agree with @stof and @Crell that there should be a separate repository for the tests, but this would cause B.C. issues. Just like the current pull request btw. #55 Looks like an elegant solution to solve the PHPUnit compatibility issue and moving the tests to a separate repository should be a major update (v2.0), right? |
|
Hi all. So please feel free to clone, copy or use it as a reference. |
|
能打中文 |
|
Reading all this thread and @Crell comment I have the feeling that I made things worse by adding the |
Introducing facilities for a specific test-framework was an extremely opinionated move - this does not belong in an package with interfaces designed to bridge projects and provide interoperability between different frameworks. (PHPUnit being a test-framework.) But as everyone seems to have concluded, the test-helper shouldn't have been there in the first place.
Yes, that's a breaking change - but they shouldn't have been there in the first place, so hopefully folks out there are using
That would be extremely problematic. PSRs are immutable - they don't have breaking changes, that's sort of the whole point. And the interfaces haven't changed - that is, the standard hasn't changed, and so the version number definitely shouldn't increase to a new major version. The good news is these are test-facilities only, so removing them won't break production code - it's very unlikely to break anything other than automated tests (assuming those run with Introducing these facilities in the first place was a mistake. The damage is done. Remove them and version this as a bugfix. |
Maybe tag a minor version first that raises a deprecation error in order to minimize the damage?
Just a clarification I was talking about this TestLogger.. do you believe that this also does not belong in this repo? |
|
@gmponos Nothing belongs in this repo except what was included in the text of the specification as passed. Anything else belongs in a -util or -tests repo. |
\PHPUnit_Framework_TestCase does not exist in phpunit > 6
For those who want a quick fix here is how you can do it: https://github.com/vanilla/vanilla/blob/19355f36e029cd26c01dfbc5823516ee537578a4/tests/phpunit.php#L5-L13