-
Notifications
You must be signed in to change notification settings - Fork 25
fix(consumer): Respect timeout config in lib_curl & fork_curl
#81
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
base: master
Are you sure you want to change the base?
fix(consumer): Respect timeout config in lib_curl & fork_curl
#81
Conversation
* LibCurl: pass 'timeout' to HttpClient constructor (default 10000 ms). * ForkCurl: convert ms→integer seconds (ceil, min 1s) and add --max-time/--connect-timeout only when > 0.
* ForkCurl: introduce protected runCommand() and use it instead of exec() (no runtime behavior change).
* ForkCurl: asserts --max-time/--connect-timeout generation * LibCurl: asserts HttpClient receives config options
Introduced HttpClient::DEFAULT_CURL_TIMEOUT and replaced hardcoded 10000ms timeout values in Client, ForkCurl, and LibCurl with the new constant. This improves maintainability by centralizing the default timeout configuration.
Corrected the spacing in the foreach loop declaration in ConsumerLibCurlTest.php.
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.
Pull Request Overview
This PR fixes the timeout configuration option to be properly respected by both lib_curl and fork_curl consumers in PostHog PHP SDK.
- Propagates the
timeoutoption from client configuration to the underlying HTTP transport layers - Adds timeout support to
fork_curlconsumer by converting milliseconds to seconds and using curl's timeout flags - Maintains backward compatibility with existing behavior when timeout is not specified
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/HttpClient.php | Adds default timeout constant for consistency |
| lib/Consumer/LibCurl.php | Passes timeout option to HttpClient constructor |
| lib/Consumer/ForkCurl.php | Adds timeout conversion logic and curl command flags |
| lib/Client.php | Uses new HttpClient constant for default timeout |
| test/MockedForkCurlConsumer.php | Test utility to mock curl command execution |
| test/ConsumerLibCurlTest.php | Tests timeout propagation via reflection |
| test/ConsumerForkCurlTest.php | Tests timeout behavior with mocked consumer |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@haacked when you get a chance, could you take a look at this PR? Thanks! |
This PR makes the documented
timeout(milliseconds) passed via config options inPostHog::initeffective for event delivery.What’s included
timeout(ms) to theHttpClientconstructor (default remains 10000 ms).timeoutms → integer seconds usingceiland append curl flags:--max-time <seconds>(curl docs)--connect-timeout <seconds>(curl docs)Backwards compatibility
timeoutis not specified or is <= 0 it is treated as unlimited. This matches previous behavior.timeoutis > 0, events captured viaPostHog::capture(...)will now honor the configured timeout for bothlib_curlandfork_curlconsumers. Previously, this value was ignored by those consumers, so requests could wait longer than intended.Tests
HttpClientreceives the configured options.