draft: feat: Unleash as env variable processor #60
Conversation
| $this->client = $client; | ||
| } | ||
|
|
||
| public function getEnv(string $prefix, string $name, \Closure $getEnv): bool |
There was a problem hiding this comment.
Please import the \Closure to be consistent with the rest of the codebase.
src/UnleashClientBundle.php
Outdated
| $container->registerForAutoconfiguration(UnleashEnvVarProcessor::class) | ||
| ->addTag('container.env_var_processor'); |
There was a problem hiding this comment.
Please remove these changes, the tag should be added in the yaml configuration.
Co-authored-by: Rikudou_Sage <dominik@chrastecky.cz>
Co-authored-by: Rikudou_Sage <dominik@chrastecky.cz>
| $value = $this->client->isEnabled($name); | ||
|
|
||
| // Env vars declared from yaml/xml files have string type | ||
| // 1 : Use unleash value first | ||
| // 2 : Retrieve the value of declared/default env var of unleash value is false or not retrieved | ||
| return $value || filter_var($getEnv($name), FILTER_VALIDATE_BOOL); |
There was a problem hiding this comment.
Looking at this here, I don't think this is the correct approach... There should be some differentiation between Unleash returning false and the feature not existing. What happens here is that when the feature is evaluated to false a default is used, which IMO is not correct.
What do you think @sighphyre?
There was a problem hiding this comment.
@RikudouSage Absolutely agree, PHP SDK does support default value as part of the isEnabled call, which is probably a better place for it. That should mean the fallback is only used when the toggle is missing, which is generally the correct flow for an SDK
Side note, if this isn't already using a context provider, I'd consider passing a context as well. Some toggle configurations, like a gradual rollout with a custom stickiness will always resolve (or should, I haven't checked) to false without a context, which is usually not what one wants
There was a problem hiding this comment.
@sighphyre This uses an instance provided by the Symfony dependency injection, meaning it already has the correct context.
@ybenhssaien Can you use the default value in the call to isEnabled()? It should be something like this:
| $value = $this->client->isEnabled($name); | |
| // Env vars declared from yaml/xml files have string type | |
| // 1 : Use unleash value first | |
| // 2 : Retrieve the value of declared/default env var of unleash value is false or not retrieved | |
| return $value || filter_var($getEnv($name), FILTER_VALIDATE_BOOL); | |
| return $this->client->isEnabled($name, default: filter_var($getEnv($name), FILTER_VALIDATE_BOOL)); |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Make it possible to use unleash as an Environment variable in a config file (Yaml, XML, PHP)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: