Check that set_time_limit() function is available before using it in core

[theode]

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.

[SergeyBiryukov]

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Just linking to some related tickets here: #19487, #21521, #46292, #49014, #52734.

[theode]

so whats the solution, why they keep it?

[johnbillion]

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.

[hellofromTonya]

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

[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 return false for a native function that is not in memory or in the PHP binary. However, it will return true for a native function disabled via the disable_functions directive. (I know!)
• There are three ways I'm aware of to return true for a native function disabled via disabled_functions:
  1. false === strpos() as WooCommerce and SMTP use.
  2. ! str_contains() - a modern version of the above.
  3. is_callable() - Which feels more accurate in context. Check if it exists with function_exists(), check if it's callable with is_callable().

[jrf]

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 return false for a native function that is not in memory or in the PHP binary. However, it will return true for a native function disabled via the disable_functions directive. (I know!)
• There are three ways I'm aware of to return true for a native function disabled via disabled_functions:
  1. false === strpos() as WooCommerce and SMTP use.
  2. ! str_contains() - a modern version of the above.
  3. is_callable() - Which feels more accurate in context. Check if it exists with function_exists(), check if it's callable with is_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

[jrf]

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.

[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.

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.

[costdev]

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.

[SergeyBiryukov]

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 return false for functions disabled via disable_functions setting.

As also noted in comment:3:ticket:42085, it is possible for the function_exists() check to return true 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 returns false.

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.

Related: #33112, #37680, #37978, #48693, #52226, #54780.

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.

[davidbaumwald]

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.

[SergeyBiryukov]

#57248 was marked as a duplicate.

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);
}

Trac ticket: #55711, #57248

Please read the thread in https://core.trac.wordpress.org/ticket/55711 - function_exists() is not a good way to do this.

[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.

Removing has-patch and adding needs-patch.

Props @mukesh27

[costdev]

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

[SergeyBiryukov]

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?

[SergeyBiryukov]

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.

[azaozz]

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?

[SergeyBiryukov]

In 55258:

General: Check that set_time_limit() function is available before using it in core.

This avoids a fatal error if the function is disabled on certain environments.

Props theode, jokerrs, johnbillion, hellofromTonya, costdev, jrf, azaozz, SergeyBiryukov.
Fixes #55711.

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.

[SergeyBiryukov]

Looks like I missed the props for @TobiasBg here. Sorry for that!

I have updated the props list for [55258] in the Core Props tool on make/core to correct that.