-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX Reset time limit back to original value #11621
FIX Reset time limit back to original value #11621
Conversation
0548c9d
to
e6709e3
Compare
Have moved fix to SapphireTest |
|
||
// Reset max_execution_time in case some code or a unit test changed it, | ||
// either via ini_set() or set_time_limit() | ||
ini_set('max_execution_time', $this->origMaxExecutionTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we still need to reset set_time_limit()
? This changes the max execution time setting, but does that also result in changing the actual time limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_time_limit() is an shorthand alias for ini_set('max_execution_time'). It seems slightly less confusing just not use the alias.
I've tested locally that it still fixes the issue that was seen in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find anything in the docs of set_time_limit()
that supports it being an alias, but I'll accept "I've tested locally" as good enough.
Issue silverstripe/.github#368