[PHP 8] Prepare for the internal functions throwing `TypeError` or `ValueError` exceptions on unexpected types/values

[ayeshrajans]

Description

PHP 8.0 is still in the making, but we know that certain changes are coming up to make things more straightforward and in favor of exceptions instead of rather silent PHP warnings.

One of these changes is that from PHP 8+, internal functions will throw an exception if the function call arguments are of a type that is not expected.

The simplest example would be strlen([]), which would throw TypeError: strlen() expects parameter 1 to be string, array given exception in PHP 8. You can read more at the RFC (​https://wiki.php.net/rfc/consistent_type_errors) and a detailed Change Record (​https://php.watch/versions/8.0/internal-function-exceptions).

WordPress has a PHPUnit annotation with the name @expectedIncorrectUsage that are used to track these kinds of errors. One example is at Tests_DB::test_prepare_sprintf_invalid_args, which uses the @expectedIncorrectUsage annotation to track PHP warnings, but instead, PHP 8's new TypeError triggers an exception (​https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/308369270#L2827).

Proposal

I propose to extend the expectedIncorrectUsage annotation to check the PHP version, and make PHPUnit expect a TypeError or a ValueError. Ideally, we should be expecting exactly a TypeError or a ValueError (sprintf too few arguments for example), which we can pass either from the annotation in the caller, or a check on the parent/sibling classes.

[desrosj]

Let's aim to get this in 5.6, though it is dependent on PHP8 entering the beta/RC stages in time.

[desrosj]

Also, feel free to use the php8 keyword for anything related. It makes it easier to get the big picture of all those tickets.

[desrosj]

Looks like we were not able to circle back to this one during 5.6. RC1 is in less than 24 hours, and even though this is a test related ticket (which can still be committed during RC), there is no patch and a bit more discussion needed.

Punting to 5.7 for now, but if we're able to get this ready before 5.6 is released, we can move it back!

[SergeyBiryukov]

Related: #51894

[hellofromTonya]

Moving this to the next cycle to be part of ongoing work in the tests and further PHP 8+ compatibility. As the next cycle is not yet available to select, moving to Future Release.

As test patches can be committed at any time into trunk, once there's a patch ready, please pull into the "current" milestone cycle.

[hellofromTonya]

Hey 6.0 is now available. Moving this ticket into the cycle.

[JavierCasares]

I just update from WordPress 5.8.3 to WordPress 5.9-RC4 (with PHP 8.0) and get this error:

2022/01/25 17:17:30 [error] 392481#392481: *6305 FastCGI sent in stderr: "PHP message: PHP Fatal error:  Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, function "wp_admin_bar_edit_site_menu" not found or invalid function name in /path/wp-includes/class-wp-hook.php:303
Stack trace:
#0 /path/wp-includes/class-wp-hook.php(327): WP_Hook->apply_filters()
#1 /path/wp-includes/plugin.php(518): WP_Hook->do_action()
#2 /path/wp-includes/admin-bar.php(95): do_action_ref_array()
#3 /path/wp-includes/class-wp-hook.php(303): wp_admin_bar_render()
#4 /path/wp-includes/class-wp-hook.php(327): WP_Hook->apply_filters()
#5 /path/wp-includes/plugin.php(470): WP_Hook->do_action()
#6 /path/wp-admin/admin-header.php(267): do_action()
#7 /path/wp-admin/admin.php(239): require_once('...')
#8 /path/wp-admin/tools.php(40): require_once('...')
#9 {main}
  thrown in /path/wp-includes/class-wp-hook.php on line" while reading upstream, client: 185.140.212.136, server: www.example.com, request: "GET /wp-admin/tools.php?page=health-check HTTP/2.0", upstream: "fastcgi://unix:/var/run/php/example80.sock:", host: "www.example.com", referrer: "https://www.example.com/wp-admin/tools.php"

Just changed it to PHP 7.4 and works fine, but many people using PHP 8.0 may get this error just in WP-Admin.

[SergeyBiryukov]

Replying to JavierCasares:

I just update from WordPress 5.8.3 to WordPress 5.9-RC4 (with PHP 8.0) and get this error:

PHP message: PHP Fatal error: Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, function "wp_admin_bar_edit_site_menu" not found or invalid function name

Thanks for testing! This sounds like an incomplete upgrade, as the function is defined in wp-includes/admin-bar.php​. It's a new function added in [52158] / #54337, so it seems like the file was not fully updated on the affected install for some reason.

[JavierCasares]

Thanks for testing! This sounds like an incomplete upgrade, as the function is defined in wp-includes/admin-bar.php​. It's a new function added in [52158] / #54337, so it seems like the file was not fully updated on the affected install for some reason.

I'm going to test and reinstall :)

[kirasong]

This ticket was brought up in a Bug Scrub today.

Thanks for all your work on it!

Since we're getting close to RC for 6.0 on May 3, and there isn't a patch yet, I'm going to move this to the 6.1 milestone

If a solution comes up before then, feel free to add it back.

[hellofromTonya]

No work happened on this ticket during the 6.1 cycle. Moving this one to 6.2 with an intent to focus on more PHP 8+ compatibility.

[hellofromTonya]

No work happened on this ticket. I know this has been punted down the road release-after-release. But this one is for the test suite. I'm moving it to the next release milestone. Why?

• Doubt there's time to discuss and implement in the remaining 6.2 cycle.
• To keep it in a milestone for visibility.

That said, when a patch is ready, it can be committed at any time and is not bound to a specific release.

[oglekler]

This ticket was discussed in the recent bug scrub.

No changes so far and no chances that it will be ready to get into 6.3, so, I am changing Milestone to 6.4.

Additional props to @hareesh

[hellofromTonya]

No progress during the 6.4 cycle. With RC1 and branching trunk tomorrow, moving this to the next cycle.

[dalleyne]

Hello,

I'm interested in contributing to this ticket and I need some guidance on where exactly extend the expectedIncorrectUsage annotation to check the PHP version should occur within the WordPress codebase.

It looks like ​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/includes/abstract-testcase.php#L560, but I'm unsure.

Could you please specify which files or components need to be updated to implement this change? Additionally, if there's any existing documentation or examples related to this annotation's current implementation, that would be highly beneficial for understanding the context and requirements of the task.

[hellofromTonya]

Clearing me as the ticket owner, as I'm no longer am a sponsored contributor. Clearing ownership opens the ticket for someone to step in to shepherd this ticket forward to resolution.

[hugod]

Hi @ayeshrajans,
I've looked into your proposal trying to find an implementation. But I cannot find a case where we would want to override the current behavior of expectedIncorrectUsage annotation.

As of today, it's mainly used to test _doing_it_wrong errors.

So if a PHP error is raised in version greater than 8.0, I would expect the test to fail anyway because it would mean that the test is using broken code.

For instance, if a test calls strlen() with an array, I expect it to fail right away with a TypeError.

Do you have any case where your request could be applicable?

[hellofromTonya]

Moving to 6.7 as the last scheduled 6.6 beta release will start in about 40 minutes.

In 6.7, would be great to return to this ticket to evaluate if it still makes sense and, if yes, what actions need to be taken to land changes during the cycle. Been punting it for far too long.

[chaion07]

Thanks @ayeshrajans for reporting this. We reviewed this Ticket during a recent bug-scrub session. Here's a summary of the feedback received:

1. Updating milestone to 6.8
2. It would be worth evaluating the need for this as the Ticket has been open for quite some time now

Thanks.

Props to @engahmeds3ed

Cheers!

[ayeshrajans]

I think we can close this. If there are any related errors, they probably have their own tickets by now.

Thank you for keeping this active and under consideration for this long :)