#50639 closed defect (bug) (fixed)
[PHP 8] Add @requires annotations for PHPUnit tests that assert engine-enforced constraints
Reported by: | ayeshrajans | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | php8 has-patch needs-refresh |
Focuses: | Cc: |
Description
PHP 8.0 throws errors when certain build-in functions encounter unexpected values.
Ref:
- https://php.watch/versions/8.0/internal-function-exceptions
- https://php.watch/versions/8.0/ValueError
In current PHP 8 builds, there are few test failures on tests that are supposed to test how functions behave when they encounter unexpected values. In PHP 8, it is not necessary to have these tests because they are validated internally by the engine now.
We still need to keep these assertions for older PHP 8 versions.
- Adds
@requires PHP < 8
annotation to prevent PHPUnit from running the test on PHP 8 and higher. - Adds
@requires extension gd
to mark that the test requiresgd
extension. This is to prevent "jpeg support required" test failures on current PHP nightly builds. I think failing a test when the test environment does not support the extension is not appropriate and we should ideally skip that test instead. Fixing test-failures to mark tests skipped is out of this ticket.
Note that PHPUnit 7.5 will be used for PHP 8 builds. @required
annotations are not supported in earlier versions such as 5.x and 6.x, which WordPress still uses for older PHP versions. For PHP 8, PHPUnit 7.5 will be used, so using @required
annotations is a progressive update.
It is also acknowledged that this is an early ticket for PHP 8. Practically speaking, we are looking to target WordPress 5.6 for this ticket.
With this ticket, we change the tests from:
Tests: 10860, Assertions: 52721, Errors: 37, Failures: 20, Skipped: 97, Risky: 3.
(https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/360267882#L3836)
To:
Tests: 10860, Assertions: 52679, Errors: 30, Failures: 10, Skipped: 121, Risky: 3
(https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/360274089#L3808)
Attachments (1)
Change History (24)
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
4 years ago
#7
@
4 years ago
Looks like the wpdb
part of 50639-1.patch was committed separately in [48973] / #50913.
Per comment:3, that appears to be not a good idea:
The tests are verifying that a "doing it wrong" notice is being thrown when the function is called incorrectly, but that the function will still handle the provided input as correctly as possible.
Disabling those tests on PHP 8 hides a problem, i.e. the function will no longer throw a notice and handle things correctly, it will now cause a white screen of death due to a fatal error.
This is a backward-compatibility break and WP will either need to put code in place to maintain the original behaviour of the function or document that the behaviour in certain circumstances is different on PHP 8.
So I guess [48973] should be reverted for now, and we should look into preserving the current behavior: a _doing_it_wrong()
notice on PHP 8, rather than a fatal error.
#11
@
4 years ago
Looking at the media test failures, they appear to be caused by missing 'gd' extension in the WP Docker image. That includes tests explicitly checking for imagejpeg()
and failing with "jpeg support unavailable", as well as tests implicitly relying on GD availability without specifically checking for it, causing more obscure failures.
The Docker image should probably be updated to provide the same list of extensions for PHP 8 that we currently have for PHP 7.4 and older: gd
, opcache
, mysqli
, zip
, exif
, intl
, mbstring
, xml
, xsl
. If anyone is able to help with that and open a PR, please do :)
Still, explicitly requiring imagejpeg()
and related functions as outlined in comment:3 would be better than the current obscure failures.
Hi @ayeshrajans Thanks for opening this ticket. I've had a look at this ticket and got some feedback for you to consider.
I don't know where you got this information from, but it's incorrect.
The
@requires
annotations have been supported for the longest time, including in PHPUnit 5.x and 6.x and even in PHPUnit 4.x.The only thing which I can think of regarding
@requires
annotations which may not have been supported before PHPUnit 7.x, is the use of the<
operator.This would need some quick testing with older PHPUnit versions to make sure they completely ignore a
@requires
annotation when they can't interpret it (in contrast to skipping the test when they can't interpret the@requires
).I trust that will be fine, but would like to see it confirmed.
I don't think this is correct.
The tests are verifying that a "doing it wrong" notice is being thrown when the function is called incorrectly, but that the function will still handle the provided input as correctly as possible.
Disabling those tests on PHP 8 hides a problem, i.e. the function will no longer throw a notice and handle things correctly, it will now cause a white screen of death due to a fatal error.
This is a backward-compatibility break and WP will either need to put code in place to maintain the original behaviour of the function or document that the behaviour in certain circumstances is different on PHP 8.
I 100% agree with the reasoning about skipping rather than failing tests and am seriously wondering why this was never changed before now.
Regarding the implementation though, I would suggest an alternative route:
Consider this test in
tests/image/intermediateSize.php
:As
@requires
annotations are supported in all used PHPUnit versions and the@requires
annotation supports functions, I'd suggest changing this to:Reasoning:
I also believe this could/should be applied in more places as a quick search of the test code base shows quite a number of places where either
$this->fail()
or$this->markTestSkipped()
is used in combination with an extension or function check.Hope this helps ;-)