fix: Allow endpoint customization based on new Betterstack Source Config#4
fix: Allow endpoint customization based on new Betterstack Source Config#4ioannis-papikas wants to merge 1 commit intogoedemiddag:masterfrom
Conversation
dvdheiden
left a comment
There was a problem hiding this comment.
@ioannis-papikas Thank you for the merge request; we appreciate your effort! 👍
My colleague @johankrijt did most of the work on this package, so he's probably the one that can help you out the best.
A comment from my side: I would like this upgrade to be as smooth as possible for current users (including a lot of our own website/applications), so I already would like to give some suggestions to achieve that by handling the missing .env entry and falling back to the default without suppressing any errors. Would you mind taking a look at those comments?
| public function __construct( | ||
| private readonly string $sourceToken, | ||
| private readonly string $endpoint, | ||
| private CurlHandle|false $handle = false, | ||
| ) { | ||
| } |
There was a problem hiding this comment.
To prevent a breaking change, we could move the endpoint parameter to be the third parameter and assign the self::URL in the constructor.
| public function __construct( | |
| private readonly string $sourceToken, | |
| private readonly string $endpoint, | |
| private CurlHandle|false $handle = false, | |
| ) { | |
| } | |
| public function __construct( | |
| private readonly string $sourceToken, | |
| private CurlHandle|false $handle = false, | |
| private readonly string $endpoint = self::URL, | |
| ) { | |
| } |
| $url = @$this->endpoint ?: self::URL; | ||
| curl_setopt($this->handle, CURLOPT_URL, $url); |
There was a problem hiding this comment.
With my constructor suggestion, you can change this to:
| $url = @$this->endpoint ?: self::URL; | |
| curl_setopt($this->handle, CURLOPT_URL, $url); | |
| curl_setopt($this->handle, CURLOPT_URL, $endpoint); |
| class BetterStackHandler extends BufferHandler | ||
| { | ||
| public function __construct(string $sourceToken, ?string $appName = null, int|string|Level $level = Level::Debug) | ||
| public function __construct(string $sourceToken, string $endpoint, ?string $appName = null, int|string|Level $level = Level::Debug) |
There was a problem hiding this comment.
| public function __construct(string $sourceToken, string $endpoint, ?string $appName = null, int|string|Level $level = Level::Debug) | |
| public function __construct(string $sourceToken, ?string $endpoint = null, ?string $appName = null, int|string|Level $level = Level::Debug) |
As the endpoint might not be set by every user of this package, it should be nullable and null by default, I think.
|
|
||
| public function __construct( | ||
| string $sourceToken, | ||
| string $endpoint, |
There was a problem hiding this comment.
| string $endpoint, | |
| ?string $endpoint = null, |
See: https://github.com/goedemiddag/betterstack-logs/pull/4/changes#r2672524685
No description provided.