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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
stayallive
left a comment
stayallive
left a comment
There was a problem hiding this comment.
This looks pretty good to me, I have a few nits but nothing major. Code looks simple enough but still powerful validations present.
| * | ||
| * @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.
2 participants
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 theWas done in #1922symfony/options-resolver.Issues
resolves #1896
closes PHP-24