From 9fb65736cbf3c35bbd367b7dc23cd7c7cc8ece03 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 21 Oct 2025 13:54:28 -0700 Subject: [PATCH] fix(flags): fallback to API for multi-condition flags with static cohorts When a feature flag has multiple conditions and one contains a static cohort, the SDK should fallback to API for the entire flag evaluation instead of skipping the static cohort condition and evaluating subsequent conditions locally. This prevents returning incorrect variants when later conditions match locally but the user is actually in the static cohort. Changes: - Introduce RequiresServerEvaluationException class to distinguish missing server-side data (static cohorts, experience continuity) from evaluation errors (bad regex, invalid dates, missing properties) - Update matchCohort() to throw RequiresServerEvaluationException when cohort not found in local cohorts - Propagate RequiresServerEvaluationException immediately in matchPropertyGroup() and matchFeatureFlagProperties() - Catch RequiresServerEvaluationException in getFeatureFlag(), getAllFlags(), and getFeatureFlagPayload() to trigger API fallback - Add test cases validating multi-condition flags with static cohorts fall back to API and return correct variants and payloads Follows same pattern as Ruby SDK fix: PostHog/posthog-ruby#80 --- lib/Client.php | 4 + lib/FeatureFlag.php | 14 +++- lib/RequiresServerEvaluationException.php | 14 ++++ test/FeatureFlagLocalEvaluationTest.php | 91 +++++++++++++++++++++ test/assests/MockedResponses.php | 96 +++++++++++++++++++++++ 5 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 lib/RequiresServerEvaluationException.php diff --git a/lib/Client.php b/lib/Client.php index 9e6fe89..327d00e 100644 --- a/lib/Client.php +++ b/lib/Client.php @@ -256,6 +256,8 @@ public function getFeatureFlag( $personProperties, $groupProperties ); + } catch (RequiresServerEvaluationException $e) { + $result = null; } catch (InconclusiveMatchException $e) { $result = null; } catch (Exception $e) { @@ -387,6 +389,8 @@ public function getAllFlags( $personProperties, $groupProperties ); + } catch (RequiresServerEvaluationException $e) { + $fallbackToFlags = true; } catch (InconclusiveMatchException $e) { $fallbackToFlags = true; } catch (Exception $e) { diff --git a/lib/FeatureFlag.php b/lib/FeatureFlag.php index 17e95ec..deb679d 100644 --- a/lib/FeatureFlag.php +++ b/lib/FeatureFlag.php @@ -103,7 +103,10 @@ public static function matchCohort($property, $propertyValues, $cohortProperties { $cohortId = strval($property["value"]); if (!array_key_exists($cohortId, $cohortProperties)) { - throw new InconclusiveMatchException("can't match cohort without a given cohort property value"); + throw new RequiresServerEvaluationException( + "cohort {$cohortId} not found in local cohorts - " . + "likely a static cohort that requires server evaluation" + ); } $propertyGroup = $cohortProperties[$cohortId]; @@ -141,6 +144,9 @@ public static function matchPropertyGroup($propertyGroup, $propertyValues, $coho return true; } } + } catch (RequiresServerEvaluationException $err) { + // Immediately propagate - this condition requires server-side data + throw $err; } catch (InconclusiveMatchException $err) { $errorMatchingLocally = true; } @@ -183,6 +189,9 @@ public static function matchPropertyGroup($propertyGroup, $propertyValues, $coho return true; } } + } catch (RequiresServerEvaluationException $err) { + // Immediately propagate - this condition requires server-side data + throw $err; } catch (InconclusiveMatchException $err) { // If this is a flag dependency error, preserve the original message if ($propType === 'flag') { @@ -402,6 +411,9 @@ function ($conditionA, $conditionB) { return FeatureFlag::getMatchingVariant($flag, $distinctId) ?? true; } } + } catch (RequiresServerEvaluationException $e) { + // Immediately propagate - this condition requires server-side data + throw $e; } catch (InconclusiveMatchException $e) { // If this is a flag dependency error, preserve the original message if ( diff --git a/lib/RequiresServerEvaluationException.php b/lib/RequiresServerEvaluationException.php new file mode 100644 index 0000000..b0ae805 --- /dev/null +++ b/lib/RequiresServerEvaluationException.php @@ -0,0 +1,14 @@ +getLine() . ' in ' . $this->getFile() . ': Requires Server Evaluation:' . $this->getMessage() . ''; //phpcs:ignore + return $errorMsg; + } +} diff --git a/test/FeatureFlagLocalEvaluationTest.php b/test/FeatureFlagLocalEvaluationTest.php index 8fafa0f..b4a3114 100644 --- a/test/FeatureFlagLocalEvaluationTest.php +++ b/test/FeatureFlagLocalEvaluationTest.php @@ -3840,4 +3840,95 @@ public function testFeatureFlagsWithFlagDependencies(): void } $this->assertTrue($threwException, "Expected InconclusiveMatchException was not thrown"); } + + public function testFallsBackToAPIWhenFlagHasStaticCohort() + { + $this->http_client = new MockedHttpClient( + host: "app.posthog.com", + flagEndpointResponse: MockedResponses::LOCAL_EVALUATION_WITH_STATIC_COHORT, + flagsEndpointResponse: MockedResponses::FLAGS_WITH_STATIC_COHORT_RESPONSE + ); + + $this->client = new Client( + self::FAKE_API_KEY, + [ + "debug" => true, + ], + $this->http_client, + "test" + ); + + $result = $this->client->getFeatureFlag( + 'multi-condition-flag', + 'test-user', + [], + ['$geoip_country_code' => 'DE'] + ); + + // Should return 'set-1' from API, not 'set-8' from local evaluation + $this->assertEquals('set-1', $result); + + $this->checkEmptyErrorLogs(); + } + + public function testFallsBackToAPIInGetAllFlagsWhenFlagHasStaticCohort() + { + $this->http_client = new MockedHttpClient( + host: "app.posthog.com", + flagEndpointResponse: MockedResponses::LOCAL_EVALUATION_WITH_STATIC_COHORT, + flagsEndpointResponse: MockedResponses::FLAGS_WITH_STATIC_COHORT_RESPONSE + ); + + $this->client = new Client( + self::FAKE_API_KEY, + [ + "debug" => true, + ], + $this->http_client, + "test" + ); + + $result = $this->client->getAllFlags( + 'test-user', + [], + ['$geoip_country_code' => 'DE'] + ); + + // Should return flags from API + $this->assertEquals([ + 'multi-condition-flag' => 'set-1' + ], $result); + + $this->checkEmptyErrorLogs(); + } + + public function testFallsBackToAPIInGetFeatureFlagPayloadWhenFlagHasStaticCohort() + { + $this->http_client = new MockedHttpClient( + host: "app.posthog.com", + flagEndpointResponse: MockedResponses::LOCAL_EVALUATION_WITH_STATIC_COHORT_FOR_PAYLOAD, + flagsEndpointResponse: MockedResponses::FLAGS_WITH_STATIC_COHORT_PAYLOAD_RESPONSE + ); + + $this->client = new Client( + self::FAKE_API_KEY, + [ + "debug" => true, + ], + $this->http_client, + "test" + ); + + $result = $this->client->getFeatureFlagPayload( + 'flag-with-payload', + 'test-user' + ); + + // Should return payload from API, not local evaluation + $this->assertEquals([ + 'message' => 'from-api' + ], $result); + + $this->checkEmptyErrorLogs(); + } } diff --git a/test/assests/MockedResponses.php b/test/assests/MockedResponses.php index 69307d5..2082550 100644 --- a/test/assests/MockedResponses.php +++ b/test/assests/MockedResponses.php @@ -1429,4 +1429,100 @@ class MockedResponses ], ] ]; + + public const LOCAL_EVALUATION_WITH_STATIC_COHORT = [ + 'flags' => [ + [ + 'id' => 1, + 'key' => 'multi-condition-flag', + 'filters' => [ + 'groups' => [ + [ + 'properties' => [ + [ + 'key' => 'id', + 'value' => 999, + 'type' => 'cohort' + ] + ], + 'rollout_percentage' => 100, + 'variant' => 'set-1' + ], + [ + 'properties' => [ + [ + 'key' => '$geoip_country_code', + 'operator' => 'exact', + 'value' => ['DE'], + 'type' => 'person' + ] + ], + 'rollout_percentage' => 100, + 'variant' => 'set-8' + ] + ], + 'multivariate' => [ + 'variants' => [ + ['key' => 'set-1', 'rollout_percentage' => 50], + ['key' => 'set-8', 'rollout_percentage' => 50] + ] + ], + 'payloads' => [ + 'set-1' => '{"message": "local-payload-1"}', + 'set-8' => '{"message": "local-payload-8"}' + ] + ], + 'active' => true, + 'is_simple_flag' => false + ] + ], + 'cohorts' => [] + ]; + + public const FLAGS_WITH_STATIC_COHORT_RESPONSE = [ + 'featureFlags' => [ + 'multi-condition-flag' => 'set-1' + ], + 'featureFlagPayloads' => [ + 'multi-condition-flag' => '{"message": "from-api"}' + ] + ]; + + public const LOCAL_EVALUATION_WITH_STATIC_COHORT_FOR_PAYLOAD = [ + 'flags' => [ + [ + 'id' => 2, + 'key' => 'flag-with-payload', + 'filters' => [ + 'groups' => [ + [ + 'properties' => [ + [ + 'key' => 'id', + 'value' => 999, + 'type' => 'cohort' + ] + ], + 'rollout_percentage' => 100 + ] + ], + 'payloads' => [ + 'true' => '{"message": "local-payload"}' + ] + ], + 'active' => true, + 'is_simple_flag' => false + ] + ], + 'cohorts' => [] + ]; + + public const FLAGS_WITH_STATIC_COHORT_PAYLOAD_RESPONSE = [ + 'featureFlags' => [ + 'flag-with-payload' => true + ], + 'featureFlagPayloads' => [ + 'flag-with-payload' => '{"message": "from-api"}' + ] + ]; }