having a new bundle does not match the PR description where you show it being configured as part of the FrameworkBundle configuration.

Indeed. It was something I wanted to discuss in the PR actually. I first worked on this as a separate package a few years ago and I've reworked it lately. I was wondering if it should be merged onto FrameworkBundle or should it be a separate one ? Should I update the description while it is decided ?

This should go in FrameworkBundle, like all core components except Security ones.

This implementation compiling all features from various providers on usage makes it impossible to build any optimization where we would build the list in a compiler pass during the cache warmup.
It also forces to instantiate all features to be able to use a single one.

IMO, a better API would be to inject a PSR ContainerInterface in the constructor. This way, the wiring in the framework could inject a ServiceLocator, which would lazy-load the Feature object that we need and no other.

For the iterator, I see 2 options:

• don't make it iterable at all (if we can)
• inject a list of feature ids in that class so that it could iterate over them and lazy-load the Feature objects associated with them.

makes it impossible to build any optimization where we would build the list in a compiler pass during the cache warmup.

Actually this is what is currently dumped in the cache :

$instance->withFeatures((new \Symfony\Component\FeatureToggle\Provider\InMemoryProvider([
    new \Symfony\Component\FeatureToggle\Feature('my-feature', 'Desc feature 1', true, 
        new \Symfony\Component\FeatureToggle\Strategy\PriorityStrategy([
            new \Symfony\Component\FeatureToggle\Strategy\RequestQueryStrategy('my_query'), 
            new \Symfony\Bundle\FeatureToggleBundle\Strategy\RequestStackAttributeStrategy('some_attribute', ($container->services['request_stack'] ??= new \Symfony\Component\HttpFoundation\RequestStack())),
        ])
    )
]))->provide(...));

I agree all features (in case of InMemoryProvider) are always instanciated but I don't see what else could be optimized during cache warmup.

Also take into account that feature names can come from SAAS as well. So unless we accept doing http calls during cache warmup it won't be possible to build the full list of available features. Also doing this during cache warmup would prevent people from updating the list on those SAAS providers without having to refresh symfony's cache.

IMO, a better API would be to inject a PSR ContainerInterface in the constructor. This way, the wiring in the framework could inject a ServiceLocator, which would lazy-load the Feature object that we need and no other.

Like I said, this implies to have access to all the features which is currently not possible unless doing http calls or else. Lazy loading the Feature Object why not but it is a very light object. What would need lazy is the strategy classes IMO

If the feature comes from a SaaS tool, loading the full list might still not be the right API (and which strategy would they use then ?). I'm not convinced the existing API is good for those SaaS providers either.

We got inspiration with how they do it in their own package and the HTTP API they exposes. They don't provide several (one to list, one to determine and so on), they pull everything locally. They continue to have some remote call to store per request which user was given which answer. What they do is on first call they put everything in cache. ATM this component does not provide full integration with such external tools. Yes there will be improvement to bring these capabilities "10% of users" for example. ATM this can already be plugged in with those tools by wrapping their actual plugins.

Regarding which strategies they will choose this has to be discussed because this can go several ways. We implement those logic in our own micro components (like notifier) or we simply bridge with their existing packages.

The strategies they offer always relies on a user list or percentage or globally available but is not as flexible as what this component tries to bring to the table (switch based on query, header, request attribute, etc etc).

For now what I can provide as a better implementation is to not compile everything right from the start but iterate over providers and stop as soon as one does provide the feature.

I've done an update on the FeatureCollection class. What could be done to improve further would be to add a has and a get method on providers. Regarding the InMemory it is an easy one and could avoid to instanciate feature if not needed. For external ones it would up to each of them to optimize things. For gitlab for example :

• fetch from https://docs.gitlab.com/ee/api/feature_flags.html#list-feature-flags-for-a-project
• store it in cache or keep it in memory (don't know yet which will be better suited)
• and simply extract feature names from it to implement the has / get methods.

WDYT ?

Providers were totally reworked. Let me know what you think.

should this grant or abstain ?

I didn't find any valid use case for Abstain in this strategy. You are either valid according to dates or not. There is "I can't answer" Maybe the Deny part could be kind of dynamic to allow either Deny or Abstain ? But I think that would make things much more complicated to understand.

Well, with those strategies that return a result based on the first one that does not abstain, it is important to define clearly those cases to make combinators useful.

However, with the current API of the component where each Feature holds a strategy, the abstain case looks quite useless. You would wire the strategy you need, skipping the irrelevant one entirely (or having a default as part of the config of that strategy alone).

For security voters, the abstain state is very important because the authorization checker does not have different voters for each permission being checked. It has a single list of voters (and a strategy about how their decisions should be combined).

We allow custom strategies and nested strategies. Abstain allows to fallback to other strategies easily. We could get rid of it with syntax like ((a OR b) AND c) but the problem is then how do we parse this to automatically create the strategies (with the right configuration)

Custom strategies and nested strategies don't need a feature to abstain. The possibility to abstain only makes those cases harder.
And as each feature currently has its own strategy object, I see to reason to register strategies that will abstain. Registering the exact strategy it needs is much easier to understand.

A case like (a OR b) AND c would be instantiated as new AndStrategy([new OrStrategy([new A(), new ()]), new C()]). And AndStrategy and OrStrategy are much simpler to implement if they don't need to care about potential abstain results.

With your current combined strategies, it is not even possible to represent that case (AffirmativeStrategy is a OrStrategy as any grant provides a grant, but there is no AndStrategy). And PriorityStrategy is meaningful only because of abstain, while I see no case where I would actually want this behavior for an understandable behavior (especially when many strategies never abstain).

We allow custom strategies and nested strategies. Abstain allows to fallback to other strategies easily. We could get rid of it with syntax like ((a OR b) AND c) but the problem is then how do we parse this to automatically create the strategies (with the right configuration)

to me, we should not have both an expression syntax with functions corresponding to some strategies and operators while having a single strategies where combining other strategies together can be done in custom ways. those are simply 2 different models which will be hard to reconcile (your PriorityStrategy has no sane representation as an expression for instance)

It is not simply cases or OR and AND. More like a == grant || b == grant , a != abstain || b != abstain, ... Abstain makes teh difference between "I couldn't answer ask someone else or use default" and "I was able to answer and the answer is to not activate it".

PriorityStrategy is represented like this : a != abstain || b != abstain. That is waht I meant by "parsing" might be hard is we need to check for the operator + the value it is compared to.

Affirmative is a == grant || b == grant

Consensus would be a != abstain && b != abstain and so on.

Should we really iterate over all registered features to collect them ? Those don't represent what happens during that request (and this will force to instantiate them all).

To me, this looks like a use case for a debug:feature-toggles command allowing to show the configuration of toggles (or maybe debug:container --tag="feature_toggle.feature" would already be enough)

a dedicated command could be indeed the solution. To me the debug is not enough as it doesn't show each inner configuration and nested strategies.

👍

Features are not listed anymore in profiler. This will be done in a dedicated command.

I think this naming makes more sense in a feature toggle context.

To me Disable does not make much sense in a Startegy context. It does not Disable a feature. Simply stating "I would not enabling it"

Also, "Abstain" means "I can't give a result, ask someone else". Renaming it to "Fallback" may be interpreted as "I can't give a result, I give you my default value".

Yeah, could be. Still, to me it reads that fallback is "I can't know for sure, so fallback to the feature default state".

In a strategy depending on a request param or env var for instance. If it's not defined fallback to default, if is defined use the value to enable/disable.

In a date period strategy, it doesn't return a fallback, the date is either inside the period or not

For the date strategy there is indeed no "abstain" possible. Either in or out. Regarding others, it means "I cannot answer for sure neither grant nor deny" Instead of Abstain (which made sense to me because of Security voters) it could be renamed something else. Neither for example. But Fallback gives too much meaning as to what happens as a consequence. To me, it shouldn't have to care about what will happen only what is the Strategy answer.

And because it cannot answer, I thought "Abstain" makes sense.

The Feature class looks more like a value object, so how about passing the strategy to the method as a collaborator and remove from constructor.

The FeatureChecker implementation can contain a mapping of feature name -> strategy service ID following this comment

If FeatureChecker contains the mapping, this code could be in

    public function isEnabled(string $featureName): bool
    {
        // Retrieve Feature and Strategy from mapping

        match ($strategy->compute()) {
            StrategyResult::Grant => true,
            StrategyResult::Deny => false,
            StrategyResult::Abstain => $feature->getDefault(),
        }
    }

The Feature object won't have any logic anymore.

ATM the Feature object is meant to hold the strategy so you cannot instanciate it without it. If we move to some kind of mapping, then yes the logic would probably be in the FeatureChecker and would require to have some kind of "default strategy".

can't we imagine adding an optional context here?
this logic of computation reminds me in some way the workflow apply logic

we can also define some internal context keys that override the bool $whenNotFound for example

We gave it a thought based on Serializer. ATM there is no need for a context. There will be when we introduce the "allow only for 10% of users" because we will need to calculate some IDs per request. Open to discuss )(either now if it must be part of the initial release or later if not)

Shouldn't the feature you ask for always exist? 🤔 What's the point of calling it with a feature that does not exist?

You might have missed some checks and you don't want to display an error. You want a global fallback value. Usually it will be false but it might be true for some reason.

Might be worth doing a command similar to translation:extract to find all feature flag usages in the code, which are not covered by config. Not sure how it will work with external provider though.

that's the thing. External provider can be configured on the fly to provide a flag or remove it. Having a not found feature flag is not an invalid state in these cases. But a debug command to list where each feature is being checked yes that would be awesome. Before doing that let's see if the component will be accepted or not as it is or if it need a major rework.

Shouldn't we throw if 2 features have the same name?

Maybe. I am not sure. Either we do some kind of last in "wins" or we ensure each is unique. But because we don't pull from teh providers during cache creation, we will figure this out only during runtime. I think it would be too much to throw an error because of this.

To provide a good DX I would throw as proposed by @fancyweb

Bear in mind that feature can be remotely provided. Thus making it impossible to know in advance what they will be without iterating over all providers. Because of this we tend to have to do this during runtime.

I think raising an exception during runtime would be bad DX. But we could make it a parameter $throwOnError. In the class it would default to false and in framework to kernel.debug.

WDYT ?

The PR description does not show any explanation about how this remote loading is expected to work. As the public API of the component takes a feature name as input, modeling a concept of feature loader taking the feature name as argument might be a much better fit than a concept of provider returning an iterable forcing to load everything all the time.

This would mean only one provider at a time ? Agree I can make things to not load everything all the time. Like stated in a previous comment :

• only iterate until I find a provider that provide the feature
• then instanciate the strategy classes only when calling the isEnabled (this can already be achieved by making the first strategy as lazy)

Why do we sometimes call it $featureName and sometimes $id?

my bad. Will make it consistent with $featureName. good catch.

Sorry I checked why in fact it is because here we implement the Psr/ContainerInterface. And to not break API with it (named arguments and such) We kept it $id