Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

draft: feat: Unleash as env variable processor #60

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions README.md
Expand Up @@ -341,6 +341,41 @@ have access to implicit `variant` variable.
{% endfeature %}
```

## Environment variable

You can use it within a yaml/xml/php configuration as an environment variable and combine it with others [Environment Variable Processors](https://symfony.com/doc/current/configuration/env_var_processors.html)

### YAML
```yaml
# config/services.yaml
parameters:
# The next line in case you need a default value
%env(unleash:feature_name)%: false
feature_toggle: %env(unleash:feature_name)%
```
### XML
```xml
<!-- config/services.xml -->
<?xml version="1.0" encoding="UTF-8" ?>
<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:framework="http://symfony.com/schema/dic/symfony"
xsi:schemaLocation="http://symfony.com/schema/dic/services
https://symfony.com/schema/dic/services/services-1.0.xsd
http://symfony.com/schema/dic/symfony
https://symfony.com/schema/dic/symfony/symfony-1.0.xsd">
<parameters>
<parameter key="feature_toggle">%env(unleash:feature_name)%</parameter>
</parameters>

</container>
```
### PHP
```php
// config/services.php
$container->setParameter('feature_toggle', '%env(unleash:feature_name)%');
```

## Custom strategies

Defining custom strategies is very easy because they get automatically injected, you just need to create a class
Expand Down
1 change: 1 addition & 0 deletions composer.json
Expand Up @@ -7,6 +7,7 @@
"symfony/event-dispatcher": "^5.0 | ^6.0 | ^7.0",
"symfony/http-client": "^5.0 | ^6.0 | ^7.0",
"symfony/cache": "^5.0 | ^6.0 | ^7.0",
"symfony/dependency-injection": "^5.0 | ^6.0 | ^7.0",
"nyholm/psr7": "^1.0",
"unleash/client": "^1.6 | ^2.0",
"php": "^8.2"
Expand Down
36 changes: 36 additions & 0 deletions src/DependencyInjection/UnleashEnvVarProcessor.php
@@ -0,0 +1,36 @@
<?php

namespace Unleash\Client\Bundle\DependencyInjection;

use Symfony\Component\DependencyInjection\EnvVarProcessorInterface;
use Unleash\Client\Unleash;

final class UnleashEnvVarProcessor implements EnvVarProcessorInterface
{
private Unleash $client;

public function __construct(Unleash $client)
{
$this->client = $client;
}
ybenhssaien marked this conversation as resolved.
Show resolved Hide resolved

public function getEnv(string $prefix, string $name, \Closure $getEnv): bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import the \Closure to be consistent with the rest of the codebase.

{
$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);
Comment on lines +17 to +22
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

Suggested change
$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));

}

/**
* @return string[]
*/
public static function getProvidedTypes(): array
{
return [
'unleash' => 'string',
];
}
}
5 changes: 5 additions & 0 deletions src/Resources/config/services.yaml
Expand Up @@ -160,3 +160,8 @@ services:
$cache: '@unleash.client.internal.cache'
tags:
- console.command

unleash.client.env_var_processor:
class: Unleash\Client\Bundle\DependencyInjection\UnleashEnvVarProcessor
arguments:
$client: '@unleash.client.unleash'
ybenhssaien marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions src/UnleashClientBundle.php
Expand Up @@ -9,6 +9,7 @@
use Unleash\Client\Bundle\DependencyInjection\Compiler\BootstrapResolver;
use Unleash\Client\Bundle\DependencyInjection\Compiler\CacheServiceResolverCompilerPass;
use Unleash\Client\Bundle\DependencyInjection\Compiler\HttpServicesResolverCompilerPass;
use Unleash\Client\Bundle\DependencyInjection\UnleashEnvVarProcessor;
use Unleash\Client\Strategy\StrategyHandler;

final class UnleashClientBundle extends Bundle
Expand All @@ -19,6 +20,8 @@ public function build(ContainerBuilder $container): void
->addTag('unleash.client.strategy_handler');
$container->registerForAutoconfiguration(BootstrapProvider::class)
->addTag('unleash.client.bootstrap_provider');
$container->registerForAutoconfiguration(UnleashEnvVarProcessor::class)
->addTag('container.env_var_processor');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these changes, the tag should be added in the yaml configuration.


$container->addCompilerPass(
new HttpServicesResolverCompilerPass(),
Expand Down