[FeatureFlag] Add ArgumentResolver #8
Conversation
Co-authored-by: Adrien Roches <adrien@roc-it.tech>
Co-authored-by: Adrien Roches <adrien@roc-it.tech>
f3a25d2 to
f64b650
Compare
f64b650 to
5e44fa4
Compare
| @@ -20,6 +20,7 @@ final class InMemoryProvider implements ProviderInterface | |||
| */ | |||
| public function __construct( | |||
| private readonly array $features, | |||
| private readonly ArgumentResolverInterface|null $argumentResolver = null, | |||
There was a problem hiding this comment.
I would rather have a ArgumentResolverAwareChecker : allows to enable / disable this feature per project and becomes global to all providers. what about chain provider ? Shouldn't it be done recurisvely then ?
There was a problem hiding this comment.
Will an ArgumentResolver be used outside the InMemoryProvider? Argument resolving, and so TypeInfo usage, may not be necessary for some feature providers (ex: based on a database or a SaaS service).
There was a problem hiding this comment.
For some clearly it would'nt be useful. But having a way to enable it would be great eitehr through a tag (controller like) or though an interface or both.
| $feature = $this->features[$featureName] ?? fn() => false; | ||
|
|
||
| return function() use ($feature): mixed { | ||
| $arguments = $this->argumentResolver?->getArguments($feature) ?? []; |
There was a problem hiding this comment.
Might be worth to have a InMemoryCache for argument resolver ? In case of controller they are not called multiple times. But flags might be.
There was a problem hiding this comment.
Not sure that is necessary since the feature's value is already cached by the FeatureChecker service.
There was a problem hiding this comment.
But if we move the argument resolver to checker it might be worth don't you think ? Or maybe I am missing somehting.
| @@ -17,7 +17,8 @@ | |||
| ], | |||
| "require": { | |||
| "php": ">=8.2", | |||
| "psr/container": "^1.1|^2.0" | |||
| "psr/container": "^1.1|^2.0", | |||
| "symfony/type-info": "^7.2" | |||
…rsimpsons) This PR was merged into the 5.4 branch. Discussion ---------- [Yaml] 🐛 throw ParseException on invalid date | Q | A | ------------- | --- | Branch? | 5.4 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | None <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT (found in symfony-tools/docs-builder#179) When parsing the following yaml: ``` date: 6418-75-51 ``` `symfony/yaml` will throw an exception: ``` $ php main.php PHP Fatal error: Uncaught Exception: Failed to parse time string (6418-75-51) at position 6 (5): Unexpected character in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php:714 Stack trace: #0 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(714): DateTimeImmutable->__construct() #1 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(312): Symfony\Component\Yaml\Inline::evaluateScalar() #2 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(80): Symfony\Component\Yaml\Inline::parseScalar() #3 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(790): Symfony\Component\Yaml\Inline::parse() #4 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(341): Symfony\Component\Yaml\Parser->parseValue() #5 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(86): Symfony\Component\Yaml\Parser->doParse() #6 /tmp/symfony-yaml/vendor/symfony/yaml/Yaml.php(77): Symfony\Component\Yaml\Parser->parse() #7 /tmp/symfony-yaml/main.php(8): Symfony\Component\Yaml\Yaml::parse() #8 {main} thrown in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php on line 714 ``` This is because the "month" is invalid. Fixing the "month" will trigger about the same issue because the "day" would be invalid. With the current change it will throw a `ParseException`. Commits ------- 6d71a7e 🐛 throw ParseException on invalid date
265286b to
f81d729
Compare
f81d729 to
47d16ba
Compare
47d16ba to
2f490f8
Compare
Adds a
ArgumentResolverInterfaceto resolve feature's arguments.