feat(Local evaluation): Use the new context-based engine #109

emyller · 2025-10-02T02:26:49Z

Contributes to #98

This is pass 2 of 3.

Implement get_evaluation_result + engine tests passing. No deleted code. (PR)
Use the new engine in the client local evaluation. Minimal changes. (this PR)
Delete unused code. Minimal refactor. (PR)

Warning

CANNOT MERGE before #108 (pass 1).

The client work is largely inspired by flagsmith-python-client.

src/Utils/Mappers.php

src/Offline/LocalFileHandler.php

gagantrivedi

Looks good! Some minor questions/comments

gagantrivedi · 2025-10-09T07:32:35Z

src/Flagsmith.php

-            $this->analyticsProcessor,
-            $this->defaultFlagHandler,
-            $identityModel->compositeKey(),
+        if ($this->localEvaluationContext === null) {


I see that the existing function didn’t have this check. What was happening earlier—a cryptic error?

Either that, or this function was never called without an ->environment. This is to keep things closer to other implementations, e.g. Python.

gagantrivedi · 2025-10-09T07:34:04Z

src/Models/Flags.php

+        foreach ($evaluationResult->flags as $flagResult) {
+            $flag = new Flag();
+            $flag->feature_name = $flagResult->name;
+            $flag->feature_id = (int) $flagResult->feature_key;


What happens if we can't convert feature+_key to INT?

Yup good question.

AFAIK feature_key is expected carry the numeric ID of a feature (see references for PHP and Python) so this is consistent with other implementations.

It doesn't mean I like it, though. I'd rather choose between:

Change data type of both FlagContext.feature_key and FlagResult.feature_key to integer, embracing the type from our current data model.

Don't assume it will always be an integer, and keep the string type, while deleting the old Flag model so this translation code isn't necessary.

Either way, I think any change here may fall out of scope and would prompt for team efforts towards standardization. IMO we should target deprecating the old models but let me know if you'd rather tackle this separately across SDKs now.

I am okay with 1 or adding a metadata field with feature id if we can't go with 1

I'm onboard with either, slightly leaning towards a metadata field only if it doesn't contribute to complexity. cc @khvn26

I don't agree we should change the data type in the context schema. On the contrary, I think that, long-term, we should aim to store/pass ids as strings in SDKs.

The perfectly correct solution would be to use metadata.

We don't have to introduce feature metadata now; for now, the mapper layer could store a feature_key: flagsmith_id mapping to use when constructing Flags from FlagResults.

Both of the above come from the assumption we absolutely want to keep Flag.id an integer, which makes little sense to me. I don't see anything wrong with major-versioning the SDK and making it a string, which it should be in the long run, anyway.

I'm happy to keep feature_key a string, and proceed with either option 2 or 3 above.

gagantrivedi · 2025-10-09T07:34:50Z

src/Offline/IOfflineHandler.php

 interface IOfflineHandler
 {
-    public function getEnvironment(): ?EnvironmentModel;
+    public function getEvaluationContext(): EvaluationContext;


same as https://github.com/Flagsmith/flagsmith-php-client/pull/109/files#r2415653879

Same as https://github.com/Flagsmith/flagsmith-php-client/pull/109/files#r2416790285

Addressed in 7a467ef.

src/Utils/Mappers.php

gagantrivedi · 2025-10-09T07:43:30Z

src/Utils/Mappers.php

+
+            $feature->variants = [];
+            $multivariateFeatureStateValues = $featureState->multivariate_feature_state_values ?? [];
+            uksort($multivariateFeatureStateValues, fn ($a, $b) => $a->id <=> $b->id);


Why are we doing this?

Product-wise, I don't know. My best guess is that this is to keep a consistent order for testing. This matches the behavior of the Python SDK.

I still think we should make FeatureContext.variants a mapping rather than a list, specially if that's the reason for sorting.

cc @khvn26

Engine expects variants to be pre-sorted in an order determined by the Flagsmith implementation — in case with Core API, this is by MultivariateFeatureOption.id.

I still think we should make FeatureContext.variants a mapping rather than a list, specially if that's the reason for sorting.

I don't necessarily agree; the only thing the engine needs is order, and that is provided by the order of elements in the array.

Thanks @khvn26 — improved with a comment. c10189b

Why do we need to sort it again if it’s already provided by core in order? Or does core not return it in that order?

I agree that it should, but, currently, there is no guarantee that it does. Counterintuitively, Django does not emit ORDER BY pk clauses by default.

Is there a reason we can’t fix this on the Django side instead of implementing it in every SDK?

@gagantrivedi The reason might be related with migrating core models to have a default order by clause, and mostly the side effects of that.

I agree input can be sorted, but it also sounds a bit fragile to make the engine rely on that solely IMO. Maybe we could have chosen to order multivariates by weight in the past, but I guess we need to keep evaluation consistent — despite major versioning the engine.

The id field here works well, except that it assumes an integer from the data model, which isn't nice, but IMO it's acceptable given this logic only exists in this [temporary?] mapper layer. Perhaps we need a new index field to help clarity, populated with the multivariate IDs from the environment document.

Okay, this is a little confusing. So far, we’ve been using whatever order Django provides — we think it’s ordered by ID, but we can’t guarantee that, which is why we don’t want to make it explicit in Django. But by sorting it here based on ID, aren’t we doing the same thing? If Django is ordering by something else, and we sort by ID here, won’t the evaluation change once this is released?

Is there a reason we can’t fix this on the Django side instead of implementing it in every SDK?

As discussed offline, this sounds like a correct solution and I'm onboard with doing it. However, we need to either keep the sorting in the SDK mapper layer to cover the Core API versions that do not yet guarantee the order, or document the change very explicitly to avoid unexpected changes in the evaluation.

Perhaps we need a new index field to help clarity, populated with the multivariate IDs from the environment document.

Not sure how making a json array more verbose by introducing a named field improves clarity here? I'd rather focus on documenting the schema more clearly.

gagantrivedi · 2025-10-09T07:44:54Z

src/Utils/Mappers.php

+        $featuresToIdentifiers = [];
+        foreach ($identityOverrides as $identityOverride) {
+            $identityFeatures = $identityOverride->identity_features ?? [];
+            uksort($identityFeatures, fn ($a, $b) => strcasecmp($a->feature->name, $b->feature->name));


Why are we sorting this? If it’s needed, it should happen after the empty check.

This matches the Python SDK behavior, but I don't see a reason either. Fixed in a52c914.

Python SDK sorts identity overrides by feature name to get consistent hashes for distinct sets of feature states.

This reverts commit a52c914.

gagantrivedi · 2025-10-10T05:53:11Z

src/Utils/Mappers.php

+        $segments = [];
+        foreach ($featuresToIdentifiers as $serializedOverridesKey => $identifiers) {
+            $segment = new SegmentContext();
+            $segment->key = hash('sha256', $serializedOverridesKey);


Why are we setting the key here?

Because I believe it's safer than an empty string, in case such segment keys are consumed later on. I understand this differs from other SDKs but I'd rather discuss improving the meaning here, or make this field nullable. cc @khvn26

The consumption of segment keys is limited to the % Split operator. Generating a key here creates the false impression that identity override segments can have % Split conditions, but they can't, and I can't think of a reason to include them. An empty string is a legitimate key that clearly indicates, in my opinion, that these segments are useless for the hashing algorithm.

The consumption of segment keys is limited to the % Split operator.

Thanks for making this clearer.

An empty string is a legitimate key that clearly indicates, in my opinion, that these segments are useless for the hashing algorithm.

I would argue that an empty string can never a clear indication of anything. 😂 e.g. this wasn't the clearest to me until now, as it certainly takes context to come up with meaning.

Attempted improving with f5eaea7.

src/Utils/Mappers.php

WIP: First pass at using the new engine

dd1c027

emyller requested a review from a team as a code owner October 2, 2025 02:26

emyller requested review from khvn26 and removed request for a team October 2, 2025 02:26

emyller self-assigned this Oct 2, 2025

emyller changed the title ~~WIP: First pass at using the new engine~~ feat(Local evaluation): Use the new context-based engine Oct 2, 2025

emyller marked this pull request as draft October 2, 2025 02:32

khvn26 requested a review from gagantrivedi October 2, 2025 08:52

Merge branch 'feat/context-values' into feat/context-values-client

0c1f0ab

emyller mentioned this pull request Oct 3, 2025

feat!: engine evaluation context Flagsmith/flagsmith-go-client#178

Open

emyller added 2 commits October 3, 2025 22:50

Let's talk, environment document

340ab82

Fix the toolbox

00b8a2e

emyller commented Oct 4, 2025

View reviewed changes

src/Utils/Mappers.php Outdated Show resolved Hide resolved

emyller added 9 commits October 3, 2025 23:02

Merge branch 'feat/context-values' into feat/context-values-client

7747e52

Fix the talking

cc41007

Fix local evaluation setting

5441f6f

Fix cloning contexts

b1937cb

Make it easier to test private members

ec3bfa2

Delete outdated code

feae1d3

Make sure getEnvironmentFlags now works as expected

c63aa45

Merge branch 'feat/context-values' into feat/context-values-client

10bdc8b

Found the key

3c6c3ce

emyller force-pushed the feat/context-values-client branch 2 times, most recently from 116f50c to 59da131 Compare October 9, 2025 05:35

Connect all the dots

d925f98

emyller force-pushed the feat/context-values-client branch from 59da131 to d925f98 Compare October 9, 2025 05:42

emyller mentioned this pull request Oct 9, 2025

feat(Engine): Evaluate from EvaluationContext #108

Open

emyller marked this pull request as ready for review October 9, 2025 05:48

Please run tests

2913611

gagantrivedi reviewed Oct 9, 2025

View reviewed changes

src/Offline/LocalFileHandler.php Outdated Show resolved Hide resolved

gagantrivedi requested changes Oct 9, 2025

View reviewed changes

Stop wasting cycles

a52c914

emyller requested a review from gagantrivedi October 9, 2025 14:19

emyller added 4 commits October 9, 2025 13:10

Revert API changes in the offline handler

7a467ef

Revert "Stop wasting cycles"

a7e74cb

This reverts commit a52c914.

Prevent us from deleting code we don't understand

c10189b

Fix casting

b3351ab

gagantrivedi requested changes Oct 10, 2025

View reviewed changes

Improve clarity

f5eaea7

emyller force-pushed the feat/context-values-client branch from 7d6f89c to f5eaea7 Compare October 10, 2025 16:15

Merge branch 'feat/context-values' into feat/context-values-client

779e732

emyller requested a review from gagantrivedi October 11, 2025 01:31

Map segment metadata from the environment context

e7adeed

emyller force-pushed the feat/context-values-client branch from b0f060a to e7adeed Compare October 13, 2025 23:10

emyller added 2 commits October 14, 2025 19:17

Merge branch 'feat/context-values' into feat/context-values-client

9b43cb8

To each their responsibilities

4145960

feat(Local evaluation): Use the new context-based engine #109

Are you sure you want to change the base?

feat(Local evaluation): Use the new context-based engine #109

Conversation

emyller commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gagantrivedi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagantrivedi Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emyller Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

emyller commented Oct 2, 2025 •

edited

Loading

khvn26 Oct 15, 2025 •

edited

Loading

khvn26 Oct 10, 2025 •

edited

Loading

gagantrivedi Oct 14, 2025 •

edited

Loading

emyller Oct 10, 2025 •

edited

Loading