ref(options): backport sentry OptionsResolver from 5.x#2151
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| if (!$validation[0]) { | ||
| // Since defaults are used as fallback values if passed options are invalid, we want to | ||
| // get hard errors here to make sure we have something to fall back to. | ||
| throw new \InvalidArgumentException(\sprintf('Invalid default for option "%s"', $option)); | ||
| } | ||
|
|
||
| $processed[$option] = $validation[1]; | ||
| } | ||
|
|
||
| $this->defaults = $processed; |
There was a problem hiding this comment.
Bug: An invalid SENTRY_DSN environment variable causes a generic InvalidArgumentException during SDK initialization, without explaining why the DSN is invalid.
Severity: HIGH
Suggested Fix
Modify the validation logic to catch the specific exception from Dsn::createFromString() and re-throw it or include its message in the InvalidArgumentException. This will provide developers with actionable information about why the DSN is considered invalid, restoring the helpfulness of the previous error reporting.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/OptionsResolver.php#L47-L59
Potential issue: The new `OptionsResolver` performs eager validation on default values
at construction time. If the `SENTRY_DSN` environment variable is set to an invalid
value, the `Options` constructor immediately throws a generic
`InvalidArgumentException('Invalid default for option "dsn"')`. This behavior is a
regression, as the previous implementation would fail later with a more descriptive
error. The current generic exception prevents developers from understanding what is
wrong with the DSN string, hindering debugging.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5b6ec69. Configure here.
| public function setDefault(string $name, $value): void | ||
| { | ||
| $validation = $this->normalizeAndValidate($name, $value); | ||
| $this->defaults[$name] = $validation[0] ? $validation[1] : null; |
There was a problem hiding this comment.
Invalid defaults stored as null
Medium Severity
When a default fails validation, setDefaults/setDefault store null even if null is not an allowed type. That surfaces with server_name, whose default comes from gethostname() (can be false): the option becomes null, then getServerName() hits a TypeError during event capture outside the transport try/catch, which can crash the host app instead of failing soft.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5b6ec69. Configure here.
There was a problem hiding this comment.
We have tests to assert the defaults are correct and we only have the null fallback for values derived from the env
| */ | ||
| public function setEnableLogs(?bool $enableLogs): self | ||
| { | ||
| $options = array_merge($this->options, ['enable_logs' => $enableLogs]); | ||
|
|
||
| $this->options = $this->resolver->resolve($options); | ||
|
|
||
| return $this; | ||
| return $this->updateOptions(['enable_logs' => $enableLogs]); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Bug: The setEnableLogs method accepts null, but the resolver is not configured to allow null for enable_logs, causing it to silently fall back to the default value.
Severity: MEDIUM
Suggested Fix
Update the OptionsResolver configuration for enable_logs to allow null values. Change $resolver->setAllowedTypes('enable_logs', 'bool'); to $resolver->setAllowedTypes('enable_logs', ['null', 'bool']);. This will align the resolver's behavior with the method's type hint.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/Options.php#L155-L161
Potential issue: The `setEnableLogs` method signature is `setEnableLogs(?bool
$enableLogs)`, indicating it accepts `null`. However, the underlying `OptionsResolver`
is configured to only allow `'bool'` for the `enable_logs` option, not `null`. When
`null` is passed to the setter, the resolver silently discards the `null` value and
falls back to the default value of `false`. This is a behavioral regression from the
previous implementation which would have thrown an exception, and it leads to the option
being set incorrectly without any warning.
2 participants
A while ago we introduced a custom OptionsResolver that works similar to the Symfony OptionsResolver but allows us to remove it as dependency and making the PHP SDK easier to install.
The new data collection configuration structure introduced nested configuration, which increased the necessity of the options resolver in the current major branch.
This PR replaces the symfony options resolver with the custom one but does not alter the required dependencies.