Skip to content

Conversation

AleMercadal
Copy link

This PR makes the documented timeout (milliseconds) passed via config options in PostHog::init effective for event delivery.

PostHog::init($token, [
  'host' => 'http://localhost:8030',
  'consumer' => 'lib_curl', // or 'fork_curl'
  'timeout' => 1000,
]);

What’s included

  • LibCurl: propagate timeout (ms) to the HttpClient constructor (default remains 10000 ms).
  • ForkCurl: convert timeout ms → integer seconds using ceil and append curl flags:

Backwards compatibility

  • If timeout is not specified or is <= 0 it is treated as unlimited. This matches previous behavior.
  • If timeout is > 0, events captured via PostHog::capture(...) will now honor the configured timeout for both lib_curl and fork_curl consumers. Previously, this value was ignored by those consumers, so requests could wait longer than intended.
  • No public API changes.

Tests

  • LibCurl: assert via Reflection that HttpClient receives the configured options.
  • ForkCurl: spy consumer asserts curl command includes/omits timeout flags based on config.

* 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
@haacked haacked requested a review from Copilot August 26, 2025 18:33
Copilot

This comment was marked as outdated.

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.
@AleMercadal AleMercadal requested a review from Copilot September 1, 2025 12:42
Copy link
Contributor

@Copilot Copilot AI left a 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 timeout option from client configuration to the underlying HTTP transport layers
  • Adds timeout support to fork_curl consumer 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant