feat: FlagsmithProvider #126
Conversation
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
…coded JSON array Signed-off-by: Chris Lightfoot-Wild <[email protected]>
…faultValue Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
ChrisLightfootWild
force-pushed
the
provider/flagsmith
branch
from
43e7651 to
e0bf3b6
Compare
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
Signed-off-by: Chris Lightfoot-Wild <[email protected]>
|
|
||
| class FlagsmithProvider extends AbstractProvider implements Provider | ||
| { | ||
| protected static string $NAME = 'FlagsmithProvider'; |
There was a problem hiding this comment.
| protected static string $NAME = 'FlagsmithProvider'; | |
| protected static string $NAME = 'Flagsmith'; |
This is up to you but I feel like Provider is implicit.
There was a problem hiding this comment.
Is it possible to detect if a flag isn't found?
There was a problem hiding this comment.
At the moment, the Flagsmith client throws a generic FlagsmithClientError in this situation, but I don't see any reason not to use this opportunity to improve the Flagsmith SDK too and extend that exception for a specific FlagsmithFlagNotFound error, similar to how we do it in the python client here.
|
|
||
| ### PHP Versioning | ||
|
|
||
| This library targets PHP version and newer. As long as you have any compatible version of PHP on your system you should be able to utilize the OpenFeature SDK. |
There was a problem hiding this comment.
| This library targets PHP version and newer. As long as you have any compatible version of PHP on your system you should be able to utilize the OpenFeature SDK. | |
| This library targets PHP version X and newer. As long as you have any compatible version of PHP on your system you should be able to utilize the OpenFeature SDK. |
| "require": { | ||
| "php": "^8.1", | ||
| "ext-json": "*", | ||
| "flagsmith/flagsmith-php-client": "^4.1", |
There was a problem hiding this comment.
I guess this is fine since we're using the ^ to specify that any version since 4.1 can be used, but the latest version is 4.5.1 and includes some important changes. Would it be better to specify ^4.5.1 here?
There was a problem hiding this comment.
At the moment, the Flagsmith client throws a generic FlagsmithClientError in this situation, but I don't see any reason not to use this opportunity to improve the Flagsmith SDK too and extend that exception for a specific FlagsmithFlagNotFound error, similar to how we do it in the python client here.
| if ($flag->getEnabled()) { | ||
| /** @var array<array-key, mixed>|bool|DateTime|float|int|string|null $value */ | ||
| $value = $flag->getValue(); | ||
|
|
||
| $builder->withValue($value); | ||
| } else { |
There was a problem hiding this comment.
In other providers (e.g. this logic in the python client) we only check this value before returning the value based on a configuration argument passed to the initialisation of the provider.
I'm not against handling this differently here, but just though I'd highlight it.
| public function resolveStringValue(string $flagKey, string $defaultValue, ?EvaluationContext $context = null): ResolutionDetails | ||
| { | ||
| return $this->resolve($flagKey, $defaultValue, $context); | ||
| } | ||
|
|
||
| public function resolveIntegerValue(string $flagKey, int $defaultValue, ?EvaluationContext $context = null): ResolutionDetails | ||
| { | ||
| return $this->resolve($flagKey, $defaultValue, $context); | ||
| } | ||
|
|
||
| public function resolveFloatValue(string $flagKey, float $defaultValue, ?EvaluationContext $context = null): ResolutionDetails | ||
| { | ||
| return $this->resolve($flagKey, $defaultValue, $context); | ||
| } |
There was a problem hiding this comment.
Since the value from the flagsmith SDK can have different types, shouldn't we do some string casting here (or in the resolve method perhaps?
This PR
open-feature/flagsmith-providerOpenFeature\interfaces\provider\Providerinterface for Flagsmithflagsmith/flagsmith-php-clientNotes
Follow-up Tasks
How to test
Run the tests with:
You should (hopefully) see the below output:
