bestit / flagception-bundle

Feature flags on steroids!
MIT License
210 stars 42 forks source link

If value comes in as a string, parse the boolean value of it. #12

Closed RedactedProfile closed 7 years ago

RedactedProfile commented 7 years ago

This is useful when setting the active value via things like symfony's env() config functions or via standard parameters which for some reason get passed as strings.

Examples

parameters:
   feature_enabled: true 
   env(FEATURE_ENABLED): true 
   another_feature_enabled: "true"

best_it_feature_toggle:
    features:
       feature_123:
           active: "%feature_enabled%"
       feature_456:
           active: "%another_feature_enabled%"
       feature_789:
           active: true 
       feature_910:
           active: "%env(FEATURE_ENABLED)%"
       feature_1112:
           active: 1
RedactedProfile commented 7 years ago

I absolutely do not wish to sound pushy in any way, but I'm wondering what my expected wait time to get this change in is? My company operates off of environment variables and this is necessary to have to this work for us.

Primarily, I just don't want to have to copy in your entire library into my primary src/ folder and make the change there if it can be helped, though I can do that if it's necessary as I need it as soon as possible

Cheers, LOVE the bundle, this is more about dealing with a strange quirk that's more an issue with how Symfony handles boolean values in a strangely strict way haha

migo315 commented 7 years ago

Thanks for your PR. I will merge it in a few minutes and release it as 2.1.1 (i handle it as a bugfix)

RedactedProfile commented 7 years ago

You are amazing good sir! Thank you :D

RedactedProfile commented 7 years ago

Hmm, interesting, tested with Symfony 3.3.7 to 3.3.10; passing environment variables to this library always seems to just return true. Will need to try something else, I will investigate further.

migo315 commented 7 years ago

I think that boolval seems to be the problem. It validates the string 'false' as true. Maybe we should use filter_var at Configuration.php.

var_dump(boolval('false')); // return true
var_dump(boolval('0')); // return false

var_dump(filter_var('false', FILTER_VALIDATE_BOOLEAN)); // return false
var_dump(filter_var('0', FILTER_VALIDATE_BOOLEAN)); // return false
RedactedProfile commented 7 years ago

I’m thinking so as well. But the bigger problem is that when passing an environment variable like active: “%env(FEATURE_MYVAR)%”, when it comes to the DI Confifurator, confusingly instead of the value of the environment variable that populates $v It comes in as env_FEATURE_MYVAR_ab478hgfh747hfyj where the hash I believe is the Symfony request unique id.

The problem is that while this should be fine, I have no idea what class to use to extract the actual value. As of an issue submitted to Symfony/symfony (I’ll have to find it later) it appears that there’s no officially supported way to supply environment variables to config builders, and the recommendation is just to stick with parameters. Which sucks.

But in the mean time, I do agree. This fix needs some more fleshing out. That said proving a string “false” to boolval does indeed return a Boolean false for me, and I can turn features on and off with parameters using string values instead of strictly Boolean

On Sat, Nov 11, 2017 at 9:45 AM Michel Chowanski notifications@github.com wrote:

I think that boolval seems to be the problem. It validates the string 'false' as true. Maybe we should use filter_var at Configuration.php.

var_dump(boolval('false')); // return true var_dump(boolval('0')); // return false

var_dump(filter_var('false', FILTER_VALIDATE_BOOLEAN)); // return false var_dump(filter_var('0', FILTER_VALIDATE_BOOLEAN)); // return false

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bestit/feature-toggle-bundle/pull/12#issuecomment-343681468, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_eJw7ORKN5NRq5kZApRPt0KTbqQtlYks5s1d0egaJpZM4QXR-2 .

migo315 commented 7 years ago

ENV variables are resolved at runtime. In TreeBuilder and the DependencyInjection Extension we only have the hash value. As far as I read this is by design and will not change. Like you, I also found no class to resolve the hash value.

Symfony 3.4 support env casting ( https://symfony.com/blog/new-in-symfony-3-4-advanced-environment-variables ) but without the possibility to validate the value at the TreeBuilder. So we still have only the hash value in Configuration.php / Extension.php.

The only solution I currently see is accepting every scalar value and creating a boolean value at runtime with filter_var.

For example: Instead of validate / normalize the value and passing the active status directly as bool ...

// ConfigStash.php

    public function add(string $feature, bool $isActive, array $constraints)
    {
        $this->features[$feature] = [
            'default' => $isActive,
            'constraints' => $constraints
        ];
    }

... every type is accepted as an argument:

// ConfigStash.php

    public function add(string $feature, $isActive, array $constraints)
    {
        // At this point, $isActive is the resolved env hash value.
        $this->features[$feature] = [
            'default' => filter_var($isActive, FILTER_VALIDATE_BOOLEAN),
            'constraints' => $constraints
        ];
    }

But that's more a workaround and not really clean.

@Haehnchen , do you have any idea?

b3nl commented 7 years ago

Shouldn't you open an issue now?

migo315 commented 7 years ago

I opened two issues. One for wrong type casting and wrong for fix the env handling.