[PHP 8] Add @requires annotations for PHPUnit tests that assert engine-enforced constraints

[ayeshrajans]

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 requires gd 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)

[jrf]

Hi @ayeshrajans Thanks for opening this ticket. I've had a look at this ticket and got some feedback for you to consider.

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

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.

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.

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.

Adds @requires extension gd to mark that the test requires gd 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.

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:

<?php
        function test_make_intermediate_size_width() {
                if ( ! function_exists( 'imagejpeg' ) ) {
                        $this->fail( 'jpeg support unavailable' );
                }

                $image = image_make_intermediate_size( DIR_TESTDATA . '/images/a2-small.jpg', 100, 0, false );

                $this->assertInternalType( 'array', $image );
        }

As @requires annotations are supported in all used PHPUnit versions and the @requires annotation supports functions, I'd suggest changing this to:

<?php
        /**
         * @requires function imagejpeg
         */
        function test_make_intermediate_size_width() {
                $image = image_make_intermediate_size( DIR_TESTDATA . '/images/a2-small.jpg', 100, 0, false );

                $this->assertInternalType( 'array', $image );
        }

Reasoning:

1. Test skipping will be consistent across PHP/PHPUnit versions.
2. Optimal use of PHPUnit native functionality.
3. Only skipped when the most specific requirement is met, i.e., the function doesn't exist rather than the extension isn't loaded.

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 ;-)

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 48979:

Tests: Revert [48973].

These tests ensure that a _doing_it_wrong() notice is thrown when wpdb::prepare() is called incorrectly, but also that the function will still handle the provided input as correctly as possible.

Disabling these 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.

That is a backward compatibility break, and wpdb::prepare() should be updated instead to maintain the original behaviour on PHP 8.

Props jrf, ayeshrajans.
See #50913, #50639.

[SergeyBiryukov]

In 48980:

Code Modernization: Return an empty string from wpdb::_real_escape() if a non-scalar value is passed.

This avoids a fatal error on PHP 8 caused by passing a non-string value to mysqli_real_escape_string(), and maintains the current behaviour.

See #50913, #50639.

[SergeyBiryukov]

In 48981:

Code Modernization: Return an empty string from wpdb::prepare() if there are not enough arguments to match the placeholders.

This avoids a fatal error on PHP 8 caused by passing mismatched arguments to vsprintf(), and maintains the current behaviour.

Follow-up to [48979], [48980].

See #50913, #50639.

[SergeyBiryukov]

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. So consistently requiring GD functions where needed seems like the next step here.

[SergeyBiryukov]

Just noting there is some overlap with #50640 here.

[SergeyBiryukov]

In 49009:

Tests: Skip some image tests if neither GD nor Imagick image editor engines are supported on the system.

The explicit message brings some consistency with other image editor tests, specifically the ones using the WP_Image_UnitTestCase class.

Previously, the tests were marked as "risky" in that scenario, due to performing no assertions.

See #50639, #50640.

[SergeyBiryukov]

In 49010:

Tests: Consistently require imagejpeg() function in image_make_intermediate_size() tests.

This outputs a proper message if the requirement is not met, instead of an obscure PHP error further in the test.

See #50639, #50640.

[SergeyBiryukov]

In 49014:

Tests: Optimize some image tests to avoid checking for image editor engines availability twice.

Follow-up to [49009].

See #50639, #50640.

[SergeyBiryukov]

In 49018:

Tests: Correctly unset non-supported image editor engines in some image tests.

Follow-up to [49009], [49014].

See #50639, #50640.

[SergeyBiryukov]

In 49024:

Tests: Convert the checks for imagejpeg() function availability to use the @requires annotation.

This better utilizes the PHPUnit native functionality.

Props ayeshrajans, jrf, johnbillion.
Fixes #50639. See #50640.

[SergeyBiryukov]

In 49025:

Tests: Convert a few more function_exists() and extension_loaded() checks to @requires annotations.

This better utilizes the PHPUnit native functionality.

Follow-up to [49024].

See #50639, #50640.

[SergeyBiryukov]

In 49045:

Tests: Require imagejpeg() function in some more media tests.

This outputs a proper message if the requirement is not met, instead of an obscure PHP error further in the test.

These tests rely on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025].

See #50639, #50640.

[SergeyBiryukov]

In 49047:

Tests: Require imagejpeg() function in some REST API attachments controller tests.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

These tests rely on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045].

See #50639, #50640.

[SergeyBiryukov]

In 49050:

Tests: Require imagejpeg() function in one more media test.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

This test relies on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045].

See #50639, #50640.

[SergeyBiryukov]

In 49069:

Tests: Require imagejpeg() function in Ajax media editing tests.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

These tests rely on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045], [49050], [49052].

See #50639, #50640.

[SergeyBiryukov]

In 49072:

Code Modernization: Return false from wpdb::query() if the query was filtered to an empty string using the query filter.

This avoids a fatal error on PHP 8 caused by passing an empty string to mysqli_query(), and maintains the current behaviour.

Follow-up to [48980], [48981].

See #50913, #50639.