Remove illegal annotation

[XedinUnknown]

The @expectedException annotation is deprecated, and removed in PHPUnit 9. When testing with PHPUnit 8, the following warning appears for every time a logger test is run:

The @ExpectedException, @expectedExceptionCode, @expectedExceptionMessage, and @expectedExceptionMessageRegExp annotations are deprecated. They will be removed in PHPUnit 9. Refactor your test to use expectException(), expectExceptionCode(), expectExceptionMessage(), or expectExceptionMessageMatches() instead.

With PHPUnit 9.5.4, the tests fail with the following error:

Inpsyde\WooCommerceLogging\Tests\Functional\AbstractLoggerTest::testThrowsOnInvalidLevel
Psr\Log\InvalidArgumentException: Unknown log level "invalid level"

/home/runner/work/product-woocommerce-logging/product-woocommerce-logging/src/AbstractLogger.php:42
/home/runner/work/product-woocommerce-logging/product-woocommerce-logging/vendor/psr/log/Psr/Log/Test/LoggerInterfaceTest.php:74

[Jean85]

This is why we should split this stuff outside in an utils package, so we can properly version it against the related tools like PHPUnit.

@Seldaek we could do that with the new versioning flow for PSR, and @Crell already invited you to proceed with that: https://groups.google.com/g/php-fig/c/Ms_iGZuUP6Y
If you want help on how to proceed, feel free to ping us! You can chat with us on Discord: https://discord.gg/php-fig

[Seldaek]

Yeah I'm not against doing this, i just don't have time.. If someone does feel free to take over.

[XedinUnknown]

Would it make sense to merge this while the more long-term solution is being considered? 😃

[Jean85]

IMHO no because it would break for anyone with old PHPUnit versions.

[Crell]

I have PRs up now for typed v2 and v2 versions that remove the test classes entirely, as they have no business being in this package to begin with. See: php-fig/fig-standards#1231

If someone wants the test classes, they belong in a util package instead.

[XedinUnknown]

I see. I guess I am coming more from the following point of view.

Right now, otherwise legal tests are failing due to the deprecated annotation. The fix is to remove the annotation, in favour of what is suggested, reasonable, and practical: explicit method call. If someone has not migrated their tests to PHPUnit 9, and they want to build on PHP 8, then it's not possible - some things just aren't - and it's their choice what they want to sacrifice (probably in terms of PHP version support).

Of course, migrating your tests would mean probably dropping support for PHP version for which there isn't a PHPUnit that supports that annotation. That decision, as well as how to overcome the problem, is of course entirely up to you. I just hope it is made sooner rather than later. 😊

[Crell]

According to https://packagist.org/packages/psr/log/php-stats, PHP 5 usage is around 3%. 7.2 and up accounts for the vast majority of uses. I'm not sure how that corresponds to PHPUnit versions, though.

Depending on the version matching it's possible we could switch the tests to use the updated PHPUnit approach in a 1.2 release, and then let them get dropped entirely in the 2.0/3.0 releases. However, that could also be a BC break for someone using these tests, depending on their PHPunit version.

We also have no idea how many people even use these test classes to begin with. I still think just killing them ASAP is the best approach.

[XedinUnknown]

PHPUnit 4 is the latest version that supports PHP 5 and not PHP 7. PHPUnit 5 is the latest that supports PHP 5 at all. Since then, there have been many deprecations and BC breaks. I know that BC break is an important concern, especially for an interop package. However, working for a WP agency, I know that even in WP, which is probably the area with most legacy code of all, only 14% of sites are on PHP 5, and those are speculated to probably be mostly abandoned installations. Besides this, although this change might break builds for a small percentage of users, BC breaks in dependencies do not constitute a BC break in dependants, strictly speaking. But I'm sure none of this is news for any of you 🙂; it's just my 5c.

Also, I haven't tried, but it might be I tried it and it is possible to use both the annotation and the call (see next comment), which should mean that you can get away with just a few deprecation notices instead of an outright failure.

[XedinUnknown]

So, I tried it in annotation-and-call branch. The build reveals the following:

1. Having both the illegal annotation and the method call does not cause an error in PHPUnit 9.
2. Tests in this package prevent it and its descendants from building on some of the target PHP versions.

Since point 2 prevents builds on PHP 5.3 and 5.4 anyway due to incompatible syntax (although it can be fixed, but there are more occurrences), this suggests that dropping support for those versions of PHP is not that bad of a break.

[Crell]

I think this is no longer relevant with the v2 and v3 releases, especially as there's now a util package for the tests instead. Let's continue the test discussion over there.