Make WordPress Core

Opened 4 years ago

Last modified 4 weeks ago

#49728 assigned defect (bug)

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

Reported by: ayeshrajans's profile ayeshrajans Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.6 Priority: normal
Severity: minor Version:
Component: Build/Test Tools Keywords: needs-patch needs-unit-tests needs-dev-note php80 needs-docs
Focuses: Cc:

Description

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.

Change History (30)

#1 @desrosj
4 years ago

  • Milestone changed from Awaiting Review to 5.6

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

#2 @desrosj
4 years ago

  • Keywords php8 added

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

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

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


3 years ago

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


3 years ago

#7 @desrosj
3 years ago

  • Milestone changed from 5.6 to 5.7

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!

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


3 years ago

#10 @lukecarbis
3 years ago

  • Milestone changed from 5.7 to Future Release

This ticket was mentioned in Slack in #core by hareesh-pillai. View the logs.


3 years ago

#12 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.9

#13 @hellofromTonya
3 years ago

  • Owner set to hellofromTonya
  • Status changed from new to assigned

#14 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to Future Release

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.

#15 @hellofromTonya
2 years ago

  • Milestone changed from Future Release to 6.0

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

#16 follow-up: @JavierCasares
2 years ago

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.

#17 in reply to: ↑ 16 ; follow-up: @SergeyBiryukov
2 years ago

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.

#18 in reply to: ↑ 17 @JavierCasares
2 years ago

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

#19 @milana_cap
2 years ago

  • Keywords needs-docs added; needs-codex removed

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


23 months ago

#21 @kirasong
23 months ago

  • Milestone changed from 6.0 to 6.1

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.

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


18 months ago

#23 @hellofromTonya
18 months ago

  • Milestone changed from 6.1 to 6.2

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.

#24 @hellofromTonya
13 months ago

  • Milestone changed from 6.2 to 6.3

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.

#25 @hellofromTonya
12 months ago

  • Keywords php80 added; php8 removed

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


9 months ago

#27 @oglekler
9 months ago

  • Milestone changed from 6.3 to 6.4

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

#28 @hellofromTonya
5 months ago

  • Milestone changed from 6.4 to 6.5

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

#29 @dalleyne
4 months ago

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.

#30 @swissspidy
4 weeks ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.