-
Notifications
You must be signed in to change notification settings - Fork 8k
Specify timeout type in timeout error message #8320
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
Conversation
Closes #25215
|
No tests? |
|
@krakjoe I suggested you because you reviewed the previous patch. Any idea on how I can test this? (unit or manual, neither worked for me) |
|
@iluuu1994 What do you mean by "I'd always run out of something else first"? There are already tests for "max execution time", so only the "input time" case would need testing. I also tried to write a test for this case, but I had to come to the conclusion that it is not currently possible. My approach would have been to use a callable default filter ( |
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.
The PR looks good to me!
| #endif | ||
|
|
||
| if (PG(max_input_time) == -1) { | ||
| zend_set_timeout(EG(timeout_seconds), 1); |
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.
actually, this is also an input timeout: when max_input_time equals to -1 the value of max_execution_time is used (https://www.php.net/manual/en/info.configuration.php#ini.max-input-time)
|
Would be nice to have this covered by test. |
|
|
This is an uncommon error message to hit so I don't want to spend more time on this. If anybody else wants to feel free to take over. |
Closes #25215
Rebase of GH-1010. I had a hard time testing this as I'd always run out of something else first.