Environment::setTimeLimit() ignores current value, can reduce timeout value

Adding a patch for 9.5.x but following @borisson_'s advice in #57.

Cleaning up the inline comments.

Created an MR from the latest patch.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding patches.

Even though a small change could we get a test? Seems like a scenario that should be covered.

Added some small tests.

Since I just added tests and not actual fix and the tests were super simple think I'm good to mark this one.

   * This function is a wrapper around the PHP function set_time_limit(). When
   * called, set_time_limit() restarts the timeout counter from zero. In other
   * words, if the timeout is the default 30 seconds, and 25 seconds into script
   * execution a call such as set_time_limit(20) is made, the script will run
   * for a total of 45 seconds before timing out.
   *
   * If the current time limit is not unlimited it is possible to decrease the
   * total time limit if the sum of the new time limit and the current time
   * spent running the script is inferior to the original time limit. It is
   * inherent to the way set_time_limit() works, it should rather be called with
   * an appropriate value every time you need to allocate a certain amount of
   * time to execute a task than only once at the beginning of the script.

Here's the current docs from the \Drupal\Component\Utility\Environment::setTimeLimit() - they need a lot of work with this change. And also this change represents a regression because it does not cover the

   * This function is a wrapper around the PHP function set_time_limit(). When
   * called, set_time_limit() restarts the timeout counter from zero. In other
   * words, if the timeout is the default 30 seconds, and 25 seconds into script
   * execution a call such as set_time_limit(20) is made, the script will run
   * for a total of 45 seconds before timing out.

part of it and it might result in current working code now timing out because we'll fail to add more time.

- resolved regression issue
- updated IS for proposed solution

For the feedback on the MR.

Review comments resolved.

Left a comment on MR.

Believe this should be good now.

The problem with this change is that it does not work as expected.

If the time limit is set to 500 by your php.ini... and after 499 seconds the code calls \Drupal\Component\Utility\Environment::setTimeLimit(100)... you'll get 100 more seconds as per the PHP manual

When called, set_time_limit() restarts the timeout counter from zero. In other words, if the timeout is the default 30 seconds, and 25 seconds into script execution a call such as set_time_limit(20) is made, the script will run for a total of 45 seconds before timing out.

See https://www.php.net/manual/en/function.set-time-limit.php

This change will prevent that from happening.

The issue does not really contain any justifications from overriding PHP's behaviour and why this is important.

If the issue is that we want to be able to config node_access_rebuild() and cron to have longer than 240… I think we should solve that - rather than trying to work around PHP vagaries.

If the time limit is set to 500 by your php.ini... and after 499 seconds the code calls \Drupal\Component\Utility\Environment::setTimeLimit(100)... you'll get 100 more seconds as per the PHP manual

With the current change, it does allocate more time instead of 100 more seconds, it allocates 600 more seconds. This solves the problem that if the time limit set in php.ini was 500 and after 2 seconds code calls \Drupal\Component\Utility\Environment::setTimeLimit(100), the time limit reduces from 498 to 100 seconds. The fix allocates always the maximum possible so that the process can complete. This allows site administrators to increase the time limit in php.ini for timeouts occurring on their site where reduction in time happens due to settimelimit in code.

@sukr_s oh I see... so now we need to explain why calling setTimeLimit(100) can result in adding 600 seconds and not 100. Again I think we're solving the problem wrong. But it gives me a good idea.

I think we should deprecate this method - it is complex to use and doesn't not what we want. I think we should add a new method with this code that is called something like ensureMinimumTimeLimit() that only sets the time limit when the current limit is smaller that that is being set.

This new method should also document that it behaves different from PHPs set_time_limit and is designed to be called once per process and early on.

@alexpott the sarcasm is not appreciated.

No developer sets the exact time they need. Excerpt from core

 // Try to allocate enough time to run all the hook_cron implementations.
    Environment::setTimeLimit(240);

They hope that the process will end in the given time. The intent is not to run for another 240 seconds, but to have the task completed. When that does not work, the site administrators increase the time in php.ini which has no effect due to the behaviour of settimelimit.

Enhancing the current function will strength the execution of Drupal core. I do not agree that a new method is required for this. And additional education required for all developers to use the new method instead of the standard function.

@sukr_s I'm sorry. I didn't intend to be sarcastic. I guess I missed some words out.

#83 should have started like: @sukr_s oh I see... thank you for pointing this out. This means that we now need to explain why calling setTimeLimit(100) can result in adding 600 seconds and not 100.

@sukr_s I disagree, We need a new method otherwise we are not helping things. The workings of PHP's set_time_limit() are clear making the wrapper Environment::setTimeLimit() work different does everyone a disservice. A method that is well named and documented is a good thing.

I had a look at some our frameworks and applications to see way they do... Symfony and Laravel have no wrappers are this method and don't really use it. Wordpress doesn't wrap the method and has plenty of calls directly to the PHP builtin https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop%20set_t... ... and craftcms has something funny... https://github.com/craftcms/cms/blob/7f77ea0be02f5be1ebc6733115f4469f0b3...

We could consider removing the method completely and add a config setting to automated cron to set the time limit to 240 (or whatever is configured) before it run's cron. And then Drupal will obey PHP's setting always with no oddness apart from automated cron - where it will do something special which feels okay because the whole way of running cron during kernel termination is funky.

There's one other place we run cron from the browser which is the manual 'run cron' button from the status report, but this could use the same setting (might need to be in system somewhere instead of automated cron).

In the interest of progress, should this issue be rescoped to the deprecation? Or closed as outdated and two new tasks created (with credit transferred as needed) -- one for the deprecation and one for the new service?

Now that we are up to 90 comments and back to square one, it seems maybe more efficient. Unless we will be using any of the work so far.

IMO the current changes can be committed and issue closed. Additional function and deprecation is not required. The promise of settimelimit is to allocate additional time. The changes does exactly that. Since the developer cannot guarantee the amount of time needed when calling this function, it can be increased by setting the php.ini in production by administrator to ensure the job is done. Adding more time than called for by the developer does not impact the site since the process will end once the task is complete irrespective of the remaining time.

This method is similar to Json::encode & Json::decode wrappers implemented in Drupal. These wrappers provide a "preferred way" in Drupal that developers don't need to remember all the flags to use each time. This method on similar lines has a "preferred behaviour" which is allowing administrators to allow increasing the time limit without changing the default behaviour.

I am not seeing a lot of support for the changes here, and I disagree with #91 that it is analogous to Json::encode. With the Json::encode example we are passing preferred options to the PHP function.

With setTimeLimit we are also doing that (in its current state).

With the changes in the MR we are changing the behaviour by adding to the php ini time limit - this could be a huge behaviour change for people calling this function.

I think our best bet is deprecate + new function.

Yes I agree with #92, we should replace this with something that's more explicit/less confusing.

Opened 📌 Deprecate Environment::setTimeLimit() with no replacement Active .

Moving over credit