Do not lower the max_execution_time if it is already set to 0 (unlimited)

[barryhughes]

Our ActionScheduler_Compatibility::raise_time_limit() is designed so that it (theoretically), "Only allows raising the existing limit and prevents lowering it."

However, in cases where the max time is already 0 (zero means unlimited) and a value such as 30 is passed to this method, the max_execution_time will indeed be changed to 30, effectively lowering it. This change addresses that.

Testing

Testing this is a little awkward, but I'd suggest using the WP CLI shell (wp shell) and running through permutations similar to those you see in the related tests, for example:

% wp shell
>>> ini_set( 'max_execution_time', '0' )
=> "0"
>>> ActionScheduler_Compatibility::raise_time_limit( 30 )
=> null
>>> ini_get( 'max_execution_time' )
=> "0"
>>> # ↑ Should be '0' (with this branch)
>>> #   May be '30' (with current tagged release)

Things you can try with this approach:

• Ensure if the max_execution_time is zero (unlimited), subsequent calls to this method do not change it.
• Ensure if max_execution_time is a non-zero positive int (such as 30), calls to this method where a lower non-zero value (such as 20) are passed do not change it.
• Ensure if max_execution_time is a non-zero positive int (such as 30), calls to this method where zero or a higher value (such as 60) are passed do change it appropriately.

If you decide to test this way, remember you will need to restart wp shell when you change branch.

Closes #754 and #745.

[dgoring]

@vedanshujain Can you please review and get this patch merged

[vedanshujain]

(Sorry for delayed review).

LGTM! Given that we are touching this method, might as well add also add return statements to both this and in WC's method so that callee can check whether the op was successful or not.

[barryhughes]

Hi @vedanshujain!

Given that we are touching this method, might as well add also add return statements

The more I think about it, and experiment with a few possibilities locally, the less sure I am how valuable a return value would be.

• If the requested $limit is less than or equal to max_execution_time then we don't try to change anything (notably, this is true regardless of how much execution time has already elapsed).
• I also don't think there is a reliable way to determine how much execution time has elapsed at the point the call is made.
• This leads me to ask, if $limit <= max_execution_time should we return false or true (assuming the return type is boolean) and would either of those things actually be useful to the caller?
• In my local testing, rather confusingly, it is possible for a call to set_time_limit() to result in (bool) false (indicating failure) yet the value of the max_execution_time ini setting may still have been modified.
• Rather than return a bool we could return an integer. This might reflect the value of max_execution_time at the end of the call, but I'm not sure how useful that would be in itself. Returning the remaining execution time would seem more useful, but as above I'm not convinced that can be reliably calculated.

Last but not least, even if a call to ::raise_time_limit() is made and that in turn triggers a successful call to set_time_limit() ... it doesn't necessarily mean much: execution could still stop prematurely (whether because of userland logic calling exit or wp_die() or because the web server or some other supervisor decides to end execution).

Given these things ... wdyt?

[dgoring]

@barryhughes
Given that set_time_limit() resets the clock for timeout and that php has no way to know reliably how long is left for execution.

I would suggest you always call set_time_limit() (unless already infinite) using either the current max_execution_time or the $limit which ever is bigger that way you are always "raising the time limit".

in terms of the return value there really isn't anything other than the result of the set_time_limit() until php add a way to retrieve how long left for execution, if it's not reliable then I suggest you leave as is.

[vedanshujain]

Hmm, ok, let' leave as it is then for the time being.