ref: replace symfony options resolver with custom resolver #1914
Conversation
| if (!$isValid) { | ||
| // 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)); |
There was a problem hiding this comment.
I added the throw here to make it easier for us to debug and to make sure we have proper defaults that adhere to the specified rules.
As per comment, we also need some defaults to fall back to so I think it makes sense to keep this, but I'm open for discussions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
# Conflicts: # src/Event.php # src/EventHint.php # src/Integration/IntegrationRegistry.php # src/Options.php # src/Stacktrace.php # src/State/Scope.php # src/UserDataBag.php
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| $logger = $options['logger'] ?? $this->getLoggerOrNullLogger(); | ||
|
|
||
| return $this->resolver->resolve($options, $logger); | ||
| } |
There was a problem hiding this comment.
Bug: Logger Initialization Issue
When resolveWithLogger is called from the constructor, it attempts to retrieve a logger. If no logger is explicitly provided in the constructor options, the fallback path accesses $this->options['logger'] before $this->options has been initialized, causing a fatal error.
There was a problem hiding this comment.
I think this is the correct behaviour. If we don't get a logger in $options and no logger is currently present in the other options, we can't log anything.
It will also not cause a fatal error because it's guarded in different places with ??
| * | ||
| * @param array<string, mixed> $override | ||
| */ | ||
| private function updateOptions(array $override = []): self |
There was a problem hiding this comment.
We might need to make this public (and @internal) if we end up having a use for it in the SDKs. But private is fine for now.
| { | ||
| $options = array_merge($this->options, $override); | ||
|
|
||
| $this->options = $this->resolveWithLogger($options); |
There was a problem hiding this comment.
My only concern is that this is fairly expensive to do for updating 2 or 3 options. Can we pass the overriden options to the resolver and have it only resolve those (new) keys and merge that with $this->options instead? Like:
| $this->options = $this->resolveWithLogger($options); | |
| $this->options = array_merge($this->options, $this->resolver->resolveOnly($options)); |
I don't know if we use setters internally anywhere but I know users do and keeping this as cheap as possible seems like a good idea.
| // 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)); |
There was a problem hiding this comment.
Good use case for an exception since this is an "us" problem and not caused by user input.
Description
This PR replaces
symfony/options-resolverwith a custom option resolver that acts almost as a drop in replacement, but comes with a reduced feature set.Notable changes between symfony and our resolver:
I also had to replace usages of PHP 8+ features since they used a polyfill which was pulled in through the
symfony/options-resolver.Issues
resolves #1896
closes PHP-24