Make WordPress Core

Opened 20 months ago

Closed 15 months ago

Last modified 15 months ago

#56817 closed defect (bug) (fixed)

Investigate new test failure in PHP 8.2

Reported by: desrosj's profile desrosj Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: php82 has-patch has-unit-tests
Focuses: Cc:

Description

There's a new unit test failure being reported on PHP 8.2 all of a sudden.

There was 1 failure:

1) Tests_Functions::test_wp_check_filetype_and_ext_with_filtered_woff
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    'ext' => 'woff'
-    'type' => 'application/font-woff'
+    'ext' => false
+    'type' => false
     'proper_filename' => false
 )

/var/www/tests/phpunit/tests/functions.php:1428
/var/www/vendor/bin/phpunit:123

After some investigation, this seems to be caused by the PHP 8.2 Docker container used during testing.

The containers are usually rebuilt weekly on Sunday, ensuring the latest point releases (or beta/RC versions in this case) are used. Unfortunately, GitHub disables workflows that run on schedule when a repository does not have activity for a period of time, so this had not run since August 27th.

After re-enabling the workflow, the containers were rebuilt and pushed. This updated the version of PHP 8.2 from beta 3 to RC3.

The timing also lines up. The first failure occurred approximately 1.5 hours after the containers were updated.

Something changed between 8.2.0beta3 and 8.2.0RC3 that's causing this failure.

Attachments (3)

56817.diff (994 bytes) - added by desrosj 20 months ago.
56817-skip.diff (935 bytes) - added by desrosj 20 months ago.
56817-skip.2.diff (626 bytes) - added by desrosj 20 months ago.

Download all attachments as: .zip

Change History (30)

#1 @desrosj
20 months ago

This is likely somehow being caused by https://github.com/php/php-src/issues/8805, which was included in PHP 8.2.0RC3. But still not sure how.

#2 @oandregal
20 months ago

For cross-reference, I've experienced this bug in this PR meant for WordPress 6.1 RC2, https://github.com/WordPress/wordpress-develop/actions/runs/3241375868/jobs/5314643672

There's also other failures. I'm not sure if all of them stem from the same issue, but wanted to share, so it's documented.

#3 @jrf
20 months ago

As a temporary measure to prevent PRs/patches/commits being blocked, I'd like to suggest adding a selective (PHP 8.2 only) $this->markTestSkipped() at the top of the test with a link to this issue.

This issue can then stay open to investigate this in more depth and determine whether this is a test issue or a src issue.

#4 @jrf
20 months ago

  • Keywords php82 added

@desrosj
20 months ago

#5 @desrosj
20 months ago

56817.diff fixes the test failure in PHP 8.2, but will cause a failure in all other versions. But at least the problem is more clear now.

I think skipping temporarily is fine for now.

@desrosj
20 months ago

#6 @SergeyBiryukov
20 months ago

56817-skip.2.diff looks good to me as a temporary measure 👍

#7 @SergeyBiryukov
20 months ago

Since woff is not an extension supported by core by default, I guess the question for further discussion is, should it be up to plugins to adjust $types['woff'] = 'application/font-woff'; based on the PHP version, or should core provide backward compatibility for that?

#8 @jrf
20 months ago

@desrosj Please add a comment with a link to this ticket to the 56817-skip.2.diff patch. Other than that looks good.

#9 @jrf
20 months ago

Also - I previously identified a similar issue with mimetypes for webp on PHP 5.6 (and 7.0 I think). This was not solved yet and there is a draft PR which has more (debug) info available: https://github.com/WordPress/wordpress-develop/pull/2453

Note: all the test + documentation improvements in that PR have been committed in the mean time, it's only the debug commits which still remain. Hmm.. maybe I should rebase it at some point to make it clearer what I was debugging there.

#10 @desrosj
20 months ago

In 54508:

Tests: Temporarily skip WOFF file test on PHP 8.2.

A recent change to how WOFF files are processed in PHP 8.2 RC3 has caused a new test failure.

This temporarily marks the test_wp_check_filetype_and_ext_with_filtered_woff() test skipped until a deeper analysis can be performed.

Props SergeyBiryukov, jrf, desrosj, oandregal.
See #56817.

#11 @desrosj
20 months ago

  • Milestone changed from 6.1 to 6.2

Moving this to 6.2 as it's not something that will be fixed for 6.1.

#12 @desrosj
20 months ago

In 54509:

Tests: Revert unintentional change in [54508].

Unprops desrosj.
See #56817.

#13 @SergeyBiryukov
19 months ago

In 54724:

Tests: Temporarily skip WOFF file test on PHP 8.1.

A recent change to how WOFF files are processed in PHP 8.2 RC3 has caused a new test failure.

The tests was previously skipped on PHP 8.2, however, apparently after a fileinfo extension update, it started failing on PHP 8.1 too.

This commit adjusts the skipping condition to include PHP 8.1.

Follow-up to [54508], [54509].

See #56817.

#14 @SergeyBiryukov
19 months ago

In 54732:

Tests: Temporarily skip WOFF file test on PHP 8.1.

A recent change to how WOFF files are processed in PHP 8.2 RC3 has caused a new test failure.

The tests was previously skipped on PHP 8.2, however, apparently after a fileinfo extension update, it started failing on PHP 8.1 too.

This commit adjusts the skipping condition to include PHP 8.1.

Follow-up to [54508], [54509].

Reviewed by desrosj, SergeyBiryukov.
Merges [54724] to the 6.1 branch.
See #56817.

#15 follow-up: @costdev
16 months ago

Changing the filter_mime_types_woff() callback in the tests to this makes the test pass on PHP 7.4/8.2:

<?php 
if ( PHP_VERSION_ID >= 80200 ) {
        $mimes['woff'] = 'font/woff';
} else {
        $mimes['woff'] = 'application/font-woff';
}
return $mimes;

However, as noted in comment:13, on PHP 8.1, success will depend on the version of the fileinfo extension.

What is the currently thinking about the desirable end goal?

  • That the accurate PHP/fileinfo-dependent type is returned (application/font-woff vs font/woff)? Or
  • That Core returns a consistent type across all PHP/fileinfo versions (application/font-woff)?

This ticket was mentioned in Slack in #core by costdev. View the logs.


15 months ago

#17 in reply to: ↑ 15 @SergeyBiryukov
15 months ago

Replying to costdev:

What is the currently thinking about the desirable end goal?

  • That the accurate PHP/fileinfo-dependent type is returned (application/font-woff vs font/woff)?

I think this would be the preferred result here. Let's adjust the tests as needed.

  • Or that Core returns a consistent type across all PHP/fileinfo versions (application/font-woff)?

Returning a consistent type sounds good in theory, but if it was changed to font/woff upstream, introducing a discrepancy with that change doesn't seem worth it.

Last edited 15 months ago by SergeyBiryukov (previous) (diff)

#18 @costdev
15 months ago

  • Component changed from General to Build/Test Tools

This ticket was discussed in the bug scrub and it was agreed to move this ticket to the Build/Test Tools component, to pursue a test-only change now and see if a slurce change is needed in a future cycle.

This ticket was mentioned in Slack in #core by costdev. View the logs.


15 months ago

This ticket was mentioned in PR #4175 on WordPress/wordpress-develop by @SergeyBiryukov.


15 months ago
#20

  • Keywords has-patch has-unit-tests added; needs-patch removed

#21 @SergeyBiryukov
15 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 55462:

Tests: Adjust the expected mime type for WOFF fonts on PHP 8.1.12+.

As of PHP 8.1.12, which includes libmagic/file update to version 5.42, the expected mime type for WOFF files has changed to font/woff, so the type needs to be adjusted accordingly in wp_check_filetype_and_ext() tests.

References:

Follow-up to [40124], [54508], [54509], [54724].

Props costdev, SergeyBiryukov.
Fixes #56817.

@SergeyBiryukov commented on PR #4175:


15 months ago
#22

Merged in r55462.

#23 @SergeyBiryukov
15 months ago

In 55463:

Tests: Adjust the expected mime type for WOFF fonts on PHP 8.1.12+.

As of PHP 8.1.12, which includes libmagic/file update to version 5.42, the expected mime type for WOFF files has changed to font/woff, so the type needs to be adjusted accordingly in wp_check_filetype_and_ext() tests.

References:

Follow-up to [40124], [54508], [54509], [54724].

Props costdev, SergeyBiryukov.
Merges [55462] to the 6.1 branch.
Fixes #56817.

#24 @desrosj
15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It looks like this should be backported to the 6.0 and 5.9 branches as well.

The PHP 8.1 job is allowed to fail in these branches, but since this doesn't involve any source code changes, I don't think it hurts to fix the test. Any objections to this?

#25 @jrf
15 months ago

No objection from me.

#26 @SergeyBiryukov
15 months ago

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

In 55497:

Tests: Adjust the expected mime type for WOFF fonts on PHP 8.1.12+.

As of PHP 8.1.12, which includes libmagic/file update to version 5.42, the expected mime type for WOFF files has changed to font/woff, so the type needs to be adjusted accordingly in wp_check_filetype_and_ext() tests.

References:

Follow-up to [40124], [54508], [54509], [54724].

Props desrosj, jrf, costdev, SergeyBiryukov.
Merges [55462] to the 6.0 branch.
Fixes #56817.

#27 @SergeyBiryukov
15 months ago

In 55498:

Tests: Adjust the expected mime type for WOFF fonts on PHP 8.1.12+.

As of PHP 8.1.12, which includes libmagic/file update to version 5.42, the expected mime type for WOFF files has changed to font/woff, so the type needs to be adjusted accordingly in wp_check_filetype_and_ext() tests.

References:

Follow-up to [40124], [54508], [54509], [54724].

Props desrosj, jrf, costdev, SergeyBiryukov.
Merges [55462] to the 5.9 branch.
Fixes #56817.

Note: See TracTickets for help on using tickets.