Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50639 closed defect (bug) (fixed)

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

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile 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:

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)

Attachments (1)

50639-1.patch (4.7 KB) - added by ayeshrajans 4 years ago.

Download all attachments as: .zip

Change History (24)

@ayeshrajans
4 years ago

#1 @ayeshrajans
4 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.6

#3 @jrf
4 years ago

  • Keywords needs-refresh added

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

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

#6 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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

#8 @SergeyBiryukov
4 years ago

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.

#9 @SergeyBiryukov
4 years ago

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.

#10 @SergeyBiryukov
4 years ago

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.

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

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.

Version 3, edited 4 years ago by SergeyBiryukov (previous) (next) (diff)

#12 @SergeyBiryukov
4 years ago

Just noting there is some overlap with #50640 here.

#13 @SergeyBiryukov
4 years ago

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.

#14 @SergeyBiryukov
4 years ago

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.

#15 @SergeyBiryukov
4 years ago

In 49014:

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

Follow-up to [49009].

See #50639, #50640.

#16 @SergeyBiryukov
4 years ago

In 49018:

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

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

See #50639, #50640.

#17 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#18 @SergeyBiryukov
4 years ago

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.

#19 @SergeyBiryukov
4 years ago

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.

#20 @SergeyBiryukov
4 years ago

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.

#21 @SergeyBiryukov
4 years ago

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.

#22 @SergeyBiryukov
4 years ago

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.

#23 @SergeyBiryukov
4 years ago

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.

Note: See TracTickets for help on using tickets.