#55711 closed enhancement (fixed)
Check that set_time_limit() function is available before using it in core
Reported by: | theode | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | php8 needs-refresh |
Focuses: | administration | Cc: |
Description (last modified by )
Original title: set_time_limit( 300 ); in Class WP_Upgrader causes crashes in PHP8 when hosting disables it
Some hosting providers disable set_time_limit(). That is causing a fatal error when installing plugins, themes and import files under PHP8.
My Suggestion is to prefix them all with an @ or remove them entirely.
Reproducing. Not sure how to deactivate it for PHP8, but is an issue.
Can't find a ticket, already stating this.
Change History (27)
#3
@
3 years ago
I believe that @-suppressing this call will still cause a fatal with PHP 8 because it no longer suppresses fatal errors. If it's common for hosts to disable this function then the calls need to be wrapped with function_exists( 'set_time_limit' )
. I know WooCommerce for example wraps this in its wc_set_time_limit()
function.
#4
@
3 years ago
I agree with @johnbillion. function_exists( 'set_time_limit' )
would guard against invoking the native PHP function when it's been disabled and is not in memory.
The issue though is beyond just the WP_Upgrader
class. Core has 10 total instances of using set_time_limit()
. Introducing a wp_set_time_limit()
wrapper function could encapsulate the code for reuse in each of these instances.
How are other projects handling this?
SMTP and WooCommerce offer slightly different solutions.
- Both are checking the if
'set_time_limit
' is in'disable_functions'
ini list:
false === strpos( ini_get( 'disable_functions' ), 'set_time_limit' )
- WooCommerce goes further to also check
function_exists()
and'safe_mode'
(in the ini).
- SMTP uses the
@
though this will be ineffective in >= PHP 8.
Reference:
- https://www.php.net/manual/en/function.set-time-limit.php
wc_set_time_limit()
in WooCommerce https://github.com/woocommerce/woocommerce/blob/33bea63963493cb3feb19c7ba4a49a3f186faff8/plugins/woocommerce/includes/wc-core-functions.php#L1686-L1696- SMTP https://core.trac.wordpress.org/browser/trunk/src/wp-includes/PHPMailer/SMTP.php#L435-437
#5
follow-up:
↓ 6
@
3 years ago
- Keywords php8 added
@hellofromTonya function_exists( 'set_time_limit' ) would guard against invoking the native PHP function when it's been disabled and is not in memory.
To clarify this:
function_exists()
will returnfalse
for a native function that is not in memory or in the PHP binary. However, it will returntrue
for a native function disabled via thedisable_functions
directive. (I know!)- There are three ways I'm aware of to return
true
for a native function disabled viadisabled_functions
:false === strpos()
as WooCommerce and SMTP use.! str_contains()
- a modern version of the above.is_callable()
- Which feels more accurate in context. Check if it exists withfunction_exists()
, check if it's callable withis_callable()
.
#6
in reply to:
↑ 5
@
3 years ago
Replying to costdev:
@hellofromTonya function_exists( 'set_time_limit' ) would guard against invoking the native PHP function when it's been disabled and is not in memory.
To clarify this:
function_exists()
will returnfalse
for a native function that is not in memory or in the PHP binary. However, it will returntrue
for a native function disabled via thedisable_functions
directive. (I know!)- There are three ways I'm aware of to return
true
for a native function disabled viadisabled_functions
:
false === strpos()
as WooCommerce and SMTP use.! str_contains()
- a modern version of the above.is_callable()
- Which feels more accurate in context. Check if it exists withfunction_exists()
, check if it's callable withis_callable()
.
Well actually....
Only a check with strpos()
/str_contains()
will give consistent results cross-version, which makes every other way of checking this invalid.
There was a change in how PHP reports on disabled functions in PHP 8.0 which is relevant here:
Disabled functions are now treated exactly like non-existent functions.
Calling a disabled function will report it as unknown, and redefining a disabled function is now possible.
Ref: https://github.com/php/php-src/blob/5e34638c1cc42520c86008cec2f7b20f3d678701/UPGRADING#L186-L188
This can be confirmed by running the following script on multiple PHP versions:
<?php echo 'function_exists: ', var_export(function_exists('set_time_limit'), true), PHP_EOL; echo 'is_callable: ', var_export(is_callable('set_time_limit'), true), PHP_EOL; echo 'strpos/str_contains: ', var_export(strpos( ini_get( 'disable_functions' ), 'set_time_limit' ), true), PHP_EOL;
Do make sure to set disable_functions = 'set_time_limit'
in the php.ini
file prior to running the script as it can not be changed at run-time (which is also the reason why I'm not posting an 3v4l link).
The output will look like this:
"PHP 5.6" ------------------ function_exists: false is_callable: true strpos/str_contains: 0 "PHP 7.0" ------------------ function_exists: false is_callable: true strpos/str_contains: 0 "PHP 7.2" ------------------ function_exists: false is_callable: true strpos/str_contains: 0 "PHP 7.4" ------------------ function_exists: true is_callable: true strpos/str_contains: 0 "PHP 8.0" ------------------ function_exists: false is_callable: false strpos/str_contains: 0 "PHP 8.1" ------------------ function_exists: false is_callable: false strpos/str_contains: 0
#7
@
3 years ago
Note: what about introducing a wp_is_function_disabled( $function_name )
function (better names welcome) ?
I know there are several places in the code base where function checks are being done as misinformed hosts have a tendency to disable those functions.
Now might be good time to streamline those checks throughout the codebase to ensure the verification of the callability of the potentially disabled functions is done consistently.
#8
follow-up:
↓ 10
@
3 years ago
@jrf Well actually... I stand corrected 😂. I spend most of my Core dev time on PHP 7.4, so that explains my results. It's surprising to me that PHP 7.4 returns a different result for function_exists()
, but that's a discussion for another forum.
I agree that wrapping these checks in a function would be a great addition to Core!
On the name: I wonder, since we're checking function_exists()
and such, would wp_is_function_available()
, wp_is_function_callable()
or even wp_is_callable()
be more appropriate?
Obligatory: naming is hard
.
#9
@
3 years ago
- Component changed from Upgrade/Install to General
- Description modified (diff)
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 6.1
- Severity changed from critical to normal
- Summary changed from set_time_limit( 300 ); in Class WP_Upgrader causes crashes in PHP8 when hosting disables it to Introduce a function to determine whether a function can be called.
- Type changed from defect (bug) to task (blessed)
- Version 5.9.3 deleted
As the arguments in favour of a function to handle all the checks is clear, I think we should handle this in this ticket instead of adding overhead by potentially patching three conditions for this issue and then removing it shortly after.
I'm changing this to a Task and widening the scope so that we can discuss the addition of a new function to handle this.
#10
in reply to:
↑ 8
@
3 years ago
Replying to costdev:
@jrf Well actually... I stand corrected 😂. I spend most of my Core dev time on PHP 7.4, so that explains my results. It's surprising to me that PHP 7.4 returns a different result for
function_exists()
, but that's a discussion for another forum.
Hmm, I could not reproduce that on PHP 7.4.15. This is what the script above returns for me, same as on other PHP 7.x versions:
"PHP 7.4" ------------------ function_exists: false is_callable: true strpos/str_contains: 0
As previously noted in comment:3:ticket:42085 and comment:116:ticket:51857, function_exists()
can return true
for disabled functions when Suhosin is in use, but I'm not aware of any other factors that might cause that:
Per the comments in the PHP manual,
function_exists()
should returnfalse
for functions disabled viadisable_functions
setting.
As also noted in comment:3:ticket:42085, it is possible for the
function_exists()
check to returntrue
if Suhosin is in use. However, that would be an edge case, as Suhosin was only officially available for PHP 5.4 to 5.6, and its development was discontinued in 2015.
It's also worth noting that as of PHP 8, disabled functions can be redeclared. Unless the function is redeclared,
function_exists()
still returnsfalse
.
It looks like most of the instances in core where we need to check for a function that might be disabled just use a simple
function_exists()
check.
So at the moment I'm not sure we need to do anything besides a function_exists()
check here, that seems to be its intended purpose.
#11
@
2 years ago
If this adds a new function in a milestone, this should be an Enhancement
.
Anyone strongly against moving this to 6.2? The deadline for FEA/EHN for 6.1 was Beta 1 a couple of days ago.
This ticket was mentioned in PR #3716 on WordPress/wordpress-develop by jokerrs.
2 years ago
#14
- Keywords has-patch added; needs-patch removed
There is a lots of shared hosting servers, which one has disabled set_time_limit function by php.ini. So usually that makes problems with updating and maintaining the WordPress.
I've made a if test for all set_time_limit I've found at the source, so they do not trigger if that function is disabled by php.ini.
<?php if (strpos(ini_get('disable_functions'), 'set_time_limit') === 0){ set_time_limit($some_time); }
2 years ago
#15
Please read the thread in https://core.trac.wordpress.org/ticket/55711 - function_exists()
is not a good way to do this.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
23 months ago
#17
follow-up:
↓ 20
@
23 months ago
- Keywords needs-patch added; has-patch removed
This ticket was discussed in the recent bug scrub. PR 3716 uses function_exists()
, which the discussion in this ticket has determined isn't reliable.
Removing has-patch
and adding needs-patch
.
Props @mukesh27
This ticket was mentioned in Slack in #core by costdev. View the logs.
22 months ago
#19
@
22 months ago
- Milestone changed from 6.2 to Future Release
This ticket was discussed during the bug scrub. As a new patch is needed and we're approaching 6.2 Beta 1 in a few days, I'm moving this ticket to Future Release.
Additional props: @davidbaumwald @petitphp @audrasjb
#20
in reply to:
↑ 17
@
22 months ago
Replying to costdev:
This ticket was discussed in the recent bug scrub. PR 3716 uses
function_exists()
, which the discussion in this ticket has determined isn't reliable.
I might be missing something, but I don't see any follow-up to comment:10 that would indicate anything other than a function_exists()
check is needed here. Except for when Suhosin is in use, do we know any other circumstances when this does not work as expected?
#21
follow-up:
↓ 23
@
22 months ago
- Keywords needs-refresh added; needs-patch removed
- Milestone changed from Future Release to 6.2
- Owner set to SergeyBiryukov
- Status changed from new to accepted
function_exists()
seems to match what we generally use in other places of core to check for potentially disabled functions, e.g. ini_get()
, so I'm not quite sure why this ticket is blocked in search of a different solution.
Doing the same for set_time_limit()
seems like a good first step here. PR 3716 could use a refresh to exclude external libraries (getID3, PHPMailer), but otherwise looks good to me.
A new ticket could then be opened to explore alternatives to function_exists()
if there is evidence of its unreliability, and to replace all of its instances in core.
#22
@
22 months ago
- Summary changed from Introduce a function to determine whether a function can be called. to Check that set_time_limit() function is available before using it in core
#23
in reply to:
↑ 21
@
22 months ago
Replying to SergeyBiryukov:
Doing the same for
set_time_limit()
seems like a good first step here.
+1
Also wondering how many actual cases of disabled set_time_limit()
exist "in the wild"? Seems this function has been used in WP for many years. If it was disabled and throwing fatal errors, there would have been a lot of reports about it, right?
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
22 months ago
@SergeyBiryukov commented on PR #3716:
22 months ago
#26
Thanks for the PR! Merged in r55258.
As noted in the comments on the ticket, we can follow up later if an alternative to function_exists()
should be considered.
Hi there, welcome back to WordPress Trac! Thanks for the ticket.
Just linking to some related tickets here: #19487, #21521, #46292, #49014, #52734.