Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#54826 closed task (blessed) (fixed)

PHP 8.0: Fatal errors are no longer suppressed by `@`.

Reported by: costdev's profile costdev Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: php8 has-patch
Focuses: Cc:

Description

Overview

From PHP 8: Backward Incompatible Changes:

The @ operator will no longer silence fatal errors (E_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR, E_PARSE)

Example case

Initially reported by @swb1192 on Slack, a Fatal Error was thrown when disk_free_space() was called during a plugin update. disk_free_space() had been disabled by the host.

In earlier versions of PHP, if a function was unavailable (i.e. disabled via php.ini), this would silently fail. In PHP 8+, the Fatal Error is thrown.

Typical fix

This is typically fixed by using function_exists() before calling the function that may be unavailable. If the check fails, return the function's documented failure value. In the case of disk_free_space(), this value is false.

Notes

  • disk_free_space() was reported on a popular plugin which also used the function. The issue was resolved by using the fix noted above. Other plugins have also used this fix for the same reason.
  • I have reached out in the #hosting-community Slack channel to request functions that are commonly disabled on hosting platforms so that we can target these functions ahead of other instances being reported.
  • Efforts are ongoing to reduce the use of @ in Core. However, until such time as that has been achieved, the fix above can be used, when appropriate, at least as a stop-gap fix.
  • Other occurrences of Fatal Errors when using @ should be reported here so that we can target these, or related tickets linked in the comments below.
  • There are currently ~269 uses of @. 239 of these occur in Core, and 30 occur upstream.

Change History (16)

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


2 years ago

#2 @costdev
2 years ago

Version 0, edited 2 years ago by costdev (next)

This ticket was mentioned in PR #2169 on WordPress/wordpress-develop by costdev.


2 years ago
#3

  • Keywords has-patch added

In PHP 8+, @ no longer suppresses fatal errors.

disk_free_space may be disabled by hosts, which will throw a Fatal Error on 'undefined function'.

This change prevents the Fatal Error, and falls back to false when disk_free_space() is unavailable.

Trac ticket:

costdev commented on PR #2169:


2 years ago
#4

The (int) cast is taken from the Rollback Update Failure plugin's usage. While I don't know the reason, I took the judgment to be sound.

I believe @SergeyBiryukov suggested the (int) for that usage. Sergey, can you clarify why this was suggested in the Rollback Update Failure plugin?

jrfnl commented on PR #2169:


2 years ago
#5

An int cast would only make sense if the fall back value was set to 0, which would effectively change the type of $available_space to always be an int.

Curious to hear the reasoning.

SergeyBiryukov commented on PR #2169:


2 years ago
#6

The (int) cast is taken from the Rollback Update Failure plugin's usage. While I don't know the reason, I took the judgment to be sound.

I believe @SergeyBiryukov suggested the (int) for that usage. Sergey, can you clarify why this was suggested in the Rollback Update Failure plugin?

If I recall correctly, my suggestion there was to use a function_exists() check instead of checking the disable_functions setting, see https://core.trac.wordpress.org/ticket/51857#comment:116 for more details. The (int) cast was pre-existing and was not a part of my comment,

I agree the (int) cast seems out of place here as is, we should either remove it completely, or use 0 as a fallback value to keep a consistent type.

jrfnl commented on PR #2169:


2 years ago
#7

IMO, removing the (int) cast is the preferred way forward, otherwise the "no space available" vs "failure to verify space available" cases can no longer be distinguished.
While it _may_ not seem important now to be able to distinguish between those cases, it may become important at a later date and I see no reason to deviate from the PHP native return types.

#8 @costdev
2 years ago

PR updated to remove the (int) cast. Ready for review.

#9 @SergeyBiryukov
2 years ago

  • Component changed from General to Upgrade/Install
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#10 @SergeyBiryukov
2 years ago

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

In 52585:

Upgrade/Install: Check if the disk_free_space() function exists before calling it.

In PHP 8+, @ no longer suppresses fatal errors:

The @ operator will no longer silence fatal errors (E_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR, E_PARSE).

Reference: PHP 8: Backward Incompatible Changes.

disk_free_space() may be disabled by hosts, which will throw a fatal error on a call to undefined function.

This change prevents the fatal error, and falls back to false when disk_free_space() is unavailable.

Follow-up to [25540], [25774], [25776], [25831], [25869].

Props costdev, jrf, swb1192, SergeyBiryukov.
Fixes #54826. See #54730.

jrfnl commented on PR #2169:


2 years ago
#12

Thanks @SergeyBiryukov ! Just checking - should an issue be opened in the plugin repo to remove the (int) cast there as well ?

SergeyBiryukov commented on PR #2169:


2 years ago
#13

Yes please! Looks like I don't have commit there, but I'm sure Andy would appreciate the PR.

This ticket was mentioned in Slack in #hosting-community by costdev. View the logs.


2 years ago

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


2 years ago

This ticket was mentioned in Slack in #hosting-community by amykamala. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.