From 8b5644090a7698e707aacc8f6098fe2e80f4b334 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Mon, 5 Oct 2026 06:05:27 +0000 Subject: [PATCH 1/4] feat(flags): honor experiment holdouts in local flag evaluation Local evaluation ignored `filters.holdout`, so a held-out identifier got an ordinary variant instead of `holdout-`, disagreeing with the server. Resolve the holdout before release conditions, using the backend-compatible hash over `holdout-` and the flag-level bucketing identity. Co-Authored-By: Claude Opus 5 Generated-By: PostHog Desktop Task-Id: 1bb30980-7c8c-4de3-8b45-5f1b1563230b --- .changeset/local-flag-holdouts.md | 5 + lib/FeatureFlag.php | 54 ++++++++ test/FeatureFlagHoldoutTest.php | 220 ++++++++++++++++++++++++++++++ 3 files changed, 279 insertions(+) create mode 100644 .changeset/local-flag-holdouts.md create mode 100644 test/FeatureFlagHoldoutTest.php diff --git a/.changeset/local-flag-holdouts.md b/.changeset/local-flag-holdouts.md new file mode 100644 index 0000000..c3614ce --- /dev/null +++ b/.changeset/local-flag-holdouts.md @@ -0,0 +1,5 @@ +--- +"posthog-php": minor +--- + +Honor experiment holdouts when evaluating feature flags locally, so a held-out identifier resolves to `holdout-` instead of an ordinary variant. diff --git a/lib/FeatureFlag.php b/lib/FeatureFlag.php index 6d13da3..2b1dfba 100644 --- a/lib/FeatureFlag.php +++ b/lib/FeatureFlag.php @@ -845,6 +845,53 @@ private static function hash($key, $distinctId, $salt = "") return $hashVal / self::LONG_SCALE; } + /** + * Resolve the holdout variant for a flag, when its bucketing identity falls inside the holdout. + * + * @param mixed $holdout The flag's filters.holdout configuration, if any. + * @param string $bucketingValue Flag-level bucketing identity (distinct id or group key). + * @return string|null The "holdout-" variant, or null when no holdout applies. + */ + private static function matchHoldout($holdout, $bucketingValue) + { + if (!is_array($holdout)) { + return null; + } + + $holdoutId = $holdout["id"] ?? null; + $exclusionPercentage = $holdout["exclusion_percentage"] ?? null; + + if (is_null($holdoutId) || is_null($exclusionPercentage) || !is_numeric($exclusionPercentage)) { + return null; + } + + $holdoutKey = "holdout-" . $holdoutId; + // Fractional percentages are preserved; only the 0-100 range is clamped. + $percentage = min(100.0, max(0.0, (float) $exclusionPercentage)); + + if ($percentage >= 100.0) { + return $holdoutKey; + } + + return FeatureFlag::holdoutHash($bucketingValue) <= $percentage / 100 ? $holdoutKey : null; + } + + /** + * Hash a bucketing identity for holdout membership, matching the backend. + * + * Unlike the ordinary flag hash, neither the flag key nor the holdout id participates: + * the digest is taken over "holdout-" with no separator or salt. + * + * @param string $bucketingValue Flag-level bucketing identity (distinct id or group key). + * @return float Hash in the range [0, 1). + */ + private static function holdoutHash($bucketingValue) + { + $hashVal = base_convert(substr(sha1("holdout-" . $bucketingValue), 0, 15), 16, 10); + + return $hashVal / self::LONG_SCALE; + } + private static function getMatchingVariant($flag, $distinctId) { $variants = FeatureFlag::variantLookupTable($flag); @@ -911,6 +958,13 @@ public static function matchFeatureFlagProperties( $propertyMatchingVersion = 1 ) { $flagFilters = $flag["filters"] ?? []; + + // Experiment holdouts win over every release condition, variant override, and rollout. + $holdoutVariant = FeatureFlag::matchHoldout($flagFilters["holdout"] ?? null, $distinctId); + if (!is_null($holdoutVariant)) { + return $holdoutVariant; + } + $flagConditions = $flagFilters["groups"] ?? []; $flagAggregation = $flagFilters["aggregation_group_type_index"] ?? null; $earlyExitEnabled = $flagFilters["early_exit"] ?? false; diff --git a/test/FeatureFlagHoldoutTest.php b/test/FeatureFlagHoldoutTest.php new file mode 100644 index 0000000..42a84ef --- /dev/null +++ b/test/FeatureFlagHoldoutTest.php @@ -0,0 +1,220 @@ + $rolloutPercentage]; + if (!is_null($variant)) { + $condition['variant'] = $variant; + } + + return [ + 'key' => 'checkout', + 'active' => true, + 'filters' => [ + 'holdout' => $holdout, + 'groups' => [$condition], + 'multivariate' => [ + 'variants' => [ + ['key' => 'control', 'rollout_percentage' => 100], + ['key' => 'test', 'rollout_percentage' => 0], + ], + ], + ], + ]; + } + + public function testHoldoutWinsOverTargetingAndVariantOverrides(): void + { + // Release conditions require an unavailable person property, have rollout 0, and + // override the variant to "test" — the holdout still decides the flag, and the + // synthetic variant doesn't need to appear in filters.multivariate.variants. + $flag = [ + 'key' => 'checkout', + 'active' => true, + 'filters' => [ + 'holdout' => ['id' => 727, 'exclusion_percentage' => 100], + 'groups' => [ + [ + 'properties' => [['key' => 'plan', 'value' => 'pro', 'operator' => 'exact']], + 'rollout_percentage' => 0, + 'variant' => 'test', + ], + ], + 'multivariate' => [ + 'variants' => [ + ['key' => 'control', 'rollout_percentage' => 100], + ['key' => 'test', 'rollout_percentage' => 0], + ], + ], + ], + ]; + + self::assertSame('holdout-727', FeatureFlag::matchFeatureFlagProperties($flag, 'user-1', [])); + } + + public function testNoHoldoutPreservesOrdinaryAssignment(): void + { + $flag = self::holdoutFlag(null); + unset($flag['filters']['holdout']); + + self::assertSame('control', FeatureFlag::matchFeatureFlagProperties($flag, 'user-1', [])); + self::assertSame('control', FeatureFlag::matchFeatureFlagProperties(self::holdoutFlag(null), 'user-1', [])); + } + + #[DataProvider('incompleteHoldoutProvider')] + public function testIncompleteHoldoutPreservesOrdinaryAssignment($holdout): void + { + self::assertSame('control', FeatureFlag::matchFeatureFlagProperties(self::holdoutFlag($holdout), 'user-1', [])); + } + + public static function incompleteHoldoutProvider(): array + { + return [ + 'missing id' => [['exclusion_percentage' => 100]], + 'null id' => [['id' => null, 'exclusion_percentage' => 100]], + 'missing exclusion percentage' => [['id' => 727]], + 'null exclusion percentage' => [['id' => 727, 'exclusion_percentage' => null]], + 'non-numeric exclusion percentage' => [['id' => 727, 'exclusion_percentage' => 'all']], + 'not an object' => ['holdout-727'], + ]; + } + + public function testPartialMembershipMatchesServerBucketing(): void + { + $flag = self::holdoutFlag(['id' => 727, 'exclusion_percentage' => 20]); + + // The holdout hash is taken over "holdout-", not the ordinary + // dot-separated flag hash: 0.17805599206573022 for "user-1", 0.6563813925994418 + // for "user-5". + self::assertSame('holdout-727', FeatureFlag::matchFeatureFlagProperties($flag, 'user-1', [])); + self::assertSame('control', FeatureFlag::matchFeatureFlagProperties($flag, 'user-5', [])); + } + + #[DataProvider('percentageBoundaryProvider')] + public function testPercentageBoundariesAreInclusiveAndClamped($exclusionPercentage, $expected): void + { + // "user-1" hashes to 0.17805599206573022. + $flag = self::holdoutFlag(['id' => 727, 'exclusion_percentage' => $exclusionPercentage]); + + self::assertSame($expected, FeatureFlag::matchFeatureFlagProperties($flag, 'user-1', [])); + } + + public static function percentageBoundaryProvider(): array + { + return [ + 'fractional percentage above the hash holds out' => [17.9, 'holdout-727'], + 'fractional percentage below the hash does not' => [17.8, 'control'], + 'integer percentage is not rounded up' => [17, 'control'], + 'zero excludes nobody' => [0, 'control'], + 'negative clamps to zero' => [-10, 'control'], + 'hundred holds out everyone' => [100, 'holdout-727'], + 'above hundred clamps to hundred' => [150, 'holdout-727'], + ]; + } + + public function testFullHoldoutAppliesWithoutABucketingHash(): void + { + $flag = self::holdoutFlag(['id' => 727, 'exclusion_percentage' => 100]); + + self::assertSame('holdout-727', FeatureFlag::matchFeatureFlagProperties($flag, '', [])); + } + + public function testDependencyEvaluationPreservesTheHoldoutValue(): void + { + $checkout = self::holdoutFlag(['id' => 727, 'exclusion_percentage' => 100]); + $dependent = [ + 'key' => 'checkout-banner', + 'active' => true, + 'filters' => [ + 'groups' => [ + [ + 'properties' => [ + [ + 'key' => 'checkout', + 'type' => 'flag', + 'operator' => 'flag_evaluates_to', + 'value' => 'holdout-727', + 'dependency_chain' => ['checkout'], + ], + ], + 'rollout_percentage' => 100, + ], + ], + ], + ]; + + // The dependency only matches if "checkout" resolved to the holdout variant. + $evaluationCache = []; + self::assertTrue(FeatureFlag::matchFeatureFlagProperties( + $dependent, + 'user-1', + [], + [], + ['checkout' => $checkout], + $evaluationCache + )); + } + + public function testInactiveFlagRemainsDisabled(): void + { + $flag = self::holdoutFlag(['id' => 727, 'exclusion_percentage' => 100]); + $flag['active'] = false; + + $snapshot = $this->localEvaluationSnapshot(['flags' => [$flag]], 'user-1'); + + self::assertFalse($snapshot->getFlag('checkout')); + } + + public function testGroupAggregatedFlagUsesTheGroupKeyForMembership(): void + { + $flag = self::holdoutFlag(['id' => 727, 'exclusion_percentage' => 20]); + $flag['filters']['aggregation_group_type_index'] = 0; + + // The group key buckets into the holdout, the distinct id does not. + $snapshot = $this->localEvaluationSnapshot( + ['flags' => [$flag], 'group_type_mapping' => ['0' => 'company']], + 'user-5', + ['company' => 'user-1'] + ); + + self::assertSame('holdout-727', $snapshot->getFlag('checkout')); + } + + private function localEvaluationSnapshot(array $localFlags, string $distinctId, array $groups = []) + { + $httpClient = new MockedHttpClient('app.posthog.com', flagEndpointResponse: $localFlags); + $client = new Client(self::FAKE_API_KEY, ['debug' => true], $httpClient, 'test'); + PostHog::init(null, null, $client); + + return $client->evaluateFlags( + $distinctId, + $groups, + [], + $groups === [] ? [] : ['company' => []], + true + ); + } +} From 2e8dc97a74e9d05b0d18797f4ade1ce81fcbf0f3 Mon Sep 17 00:00:00 2001 From: Ioannis J Date: Fri, 16 Oct 2026 11:20:30 -0700 Subject: [PATCH 2/4] fix(flags): defer holdout dependencies aggregated differently from the evaluated identity --- lib/FeatureFlag.php | 141 ++++++++++++++++++++++++---- test/FeatureFlagHoldoutTest.php | 159 ++++++++++++++++++++++++++++++++ 2 files changed, 282 insertions(+), 18 deletions(-) diff --git a/lib/FeatureFlag.php b/lib/FeatureFlag.php index 2b1dfba..a728064 100644 --- a/lib/FeatureFlag.php +++ b/lib/FeatureFlag.php @@ -854,26 +854,47 @@ private static function hash($key, $distinctId, $salt = "") */ private static function matchHoldout($holdout, $bucketingValue) { - if (!is_array($holdout)) { + $percentage = FeatureFlag::holdoutPercentage($holdout); + if (is_null($percentage)) { return null; } - $holdoutId = $holdout["id"] ?? null; - $exclusionPercentage = $holdout["exclusion_percentage"] ?? null; + $holdoutKey = "holdout-" . $holdout["id"]; + + if ($percentage >= 100.0) { + return $holdoutKey; + } - if (is_null($holdoutId) || is_null($exclusionPercentage) || !is_numeric($exclusionPercentage)) { + return FeatureFlag::holdoutHash($bucketingValue) <= $percentage / 100 ? $holdoutKey : null; + } + + /** + * @param mixed $holdout The flag's filters.holdout configuration, if any. + * @return float|null Exclusion percentage clamped to 0-100, or null when the holdout is unusable. + */ + private static function holdoutPercentage($holdout): ?float + { + if (!is_array($holdout) || is_null($holdout["id"] ?? null)) { + return null; + } + + $exclusionPercentage = $holdout["exclusion_percentage"] ?? null; + if (!is_numeric($exclusionPercentage)) { return null; } - $holdoutKey = "holdout-" . $holdoutId; // Fractional percentages are preserved; only the 0-100 range is clamped. - $percentage = min(100.0, max(0.0, (float) $exclusionPercentage)); + return min(100.0, max(0.0, (float) $exclusionPercentage)); + } - if ($percentage >= 100.0) { - return $holdoutKey; - } + private static function hasActiveHoldout($holdout): bool + { + return (FeatureFlag::holdoutPercentage($holdout) ?? 0.0) > 0.0; + } - return FeatureFlag::holdoutHash($bucketingValue) <= $percentage / 100 ? $holdoutKey : null; + private static function aggregationKey($aggregation): ?string + { + return is_null($aggregation) ? null : strval($aggregation); } /** @@ -956,6 +977,40 @@ public static function matchFeatureFlagProperties( $groupProperties = [], $groupTypeMapping = [], $propertyMatchingVersion = 1 + ) { + return FeatureFlag::matchFeatureFlagPropertiesForIdentity( + $flag, + $distinctId, + $properties, + $cohorts, + $flagsByKey, + $evaluationCache, + $groups, + $groupProperties, + $groupTypeMapping, + $propertyMatchingVersion, + ($flag["filters"] ?? [])["aggregation_group_type_index"] ?? null + ); + } + + /** + * Same as matchFeatureFlagProperties, but bucketing by an identity whose aggregation is already known. + * Dependency recursion keeps the root flag's identity, which may differ from the dependency's own. + * + * @param int|string|null $identityAggregation Null for a person, a group type index otherwise. + */ + private static function matchFeatureFlagPropertiesForIdentity( + $flag, + $distinctId, + $properties, + $cohorts, + $flagsByKey, + $evaluationCache, + $groups, + $groupProperties, + $groupTypeMapping, + $propertyMatchingVersion, + $identityAggregation ) { $flagFilters = $flag["filters"] ?? []; @@ -982,6 +1037,7 @@ public static function matchFeatureFlagProperties( $effectiveProperties = $properties; $effectiveBucketing = $distinctId; + $effectiveAggregation = $identityAggregation; // Mixed-override path: condition-level aggregation differs from flag-level. // This assumes flag-level aggregation is null for mixed flags. @@ -997,6 +1053,7 @@ public static function matchFeatureFlagProperties( } $effectiveProperties = $groupProperties[$groupName]; $effectiveBucketing = $groups[$groupName]; + $effectiveAggregation = $conditionAggregation; } } @@ -1008,7 +1065,8 @@ public static function matchFeatureFlagProperties( $cohorts, $flagsByKey, $evaluationCache, - $propertyMatchingVersion + $propertyMatchingVersion, + $effectiveAggregation ); if ($matchResult === ConditionMatch::Match) { @@ -1064,9 +1122,10 @@ private static function isConditionMatch( $condition, $properties, $cohorts, - $flagsByKey = null, - $evaluationCache = null, - $propertyMatchingVersion = 1 + $flagsByKey, + $evaluationCache, + $propertyMatchingVersion, + $identityAggregation ): ConditionMatch { $rolloutPercentage = array_key_exists("rollout_percentage", $condition) ? $condition["rollout_percentage"] : null; @@ -1085,14 +1144,15 @@ private static function isConditionMatch( $propertyMatchingVersion ); } elseif ($propertyType == 'flag') { - $matches = FeatureFlag::evaluateFlagDependency( + $matches = FeatureFlag::evaluateFlagDependencyForIdentity( $property, $flagsByKey, $evaluationCache, $distinctId, $properties, $cohorts, - $propertyMatchingVersion + $propertyMatchingVersion, + $identityAggregation ); } else { $matches = FeatureFlag::matchProperty($property, $properties, $propertyMatchingVersion); @@ -1168,6 +1228,32 @@ public static function evaluateFlagDependency( $properties, $cohortProperties, $propertyMatchingVersion = 1 + ) { + return FeatureFlag::evaluateFlagDependencyForIdentity( + $property, + $flagsByKey, + $evaluationCache, + $distinctId, + $properties, + $cohortProperties, + $propertyMatchingVersion, + false + ); + } + + /** + * @param int|string|null|false $identityAggregation Aggregation of the identity being bucketed (null for a + * person, a group type index otherwise). False means unknown and skips the holdout aggregation guard. + */ + private static function evaluateFlagDependencyForIdentity( + $property, + $flagsByKey, + $evaluationCache, + $distinctId, + $properties, + $cohortProperties, + $propertyMatchingVersion, + $identityAggregation ) { if ($flagsByKey === null || $evaluationCache === null) { throw new InconclusiveMatchException(sprintf( @@ -1219,16 +1305,35 @@ public static function evaluateFlagDependency( if (!($depFlag["active"] ?? false)) { $evaluationCache[$depFlagKey] = false; } else { + // Holdout bucketing uses the evaluated identity, which can't be resolved for a + // dependency aggregated differently, so defer to server evaluation. + $depFilters = $depFlag["filters"] ?? []; + if ( + $identityAggregation !== false && + FeatureFlag::hasActiveHoldout($depFilters["holdout"] ?? null) && + FeatureFlag::aggregationKey($depFilters["aggregation_group_type_index"] ?? null) !== + FeatureFlag::aggregationKey($identityAggregation) + ) { + throw new InconclusiveMatchException(sprintf( + "Cannot evaluate flag dependency '%s' with a holdout aggregated differently from the dependent flag", //phpcs:ignore + $depFlagKey + )); + } + // Recursively evaluate the dependency try { - $depResult = FeatureFlag::matchFeatureFlagProperties( + $depResult = FeatureFlag::matchFeatureFlagPropertiesForIdentity( $depFlag, $distinctId, $properties, $cohortProperties, $flagsByKey, $evaluationCache, - propertyMatchingVersion: $propertyMatchingVersion + [], + [], + [], + $propertyMatchingVersion, + $identityAggregation ); $evaluationCache[$depFlagKey] = $depResult; } catch (InconclusiveMatchException $e) { diff --git a/test/FeatureFlagHoldoutTest.php b/test/FeatureFlagHoldoutTest.php index 42a84ef..447ea0e 100644 --- a/test/FeatureFlagHoldoutTest.php +++ b/test/FeatureFlagHoldoutTest.php @@ -203,6 +203,165 @@ public function testGroupAggregatedFlagUsesTheGroupKeyForMembership(): void self::assertSame('holdout-727', $snapshot->getFlag('checkout')); } + private static function bannerDependingOnCheckout(string $expectedValue): array + { + return [ + 'key' => 'checkout-banner', + 'active' => true, + 'filters' => [ + 'groups' => [ + [ + 'properties' => [ + [ + 'key' => 'checkout', + 'type' => 'flag', + 'operator' => 'flag_evaluates_to', + 'value' => $expectedValue, + 'dependency_chain' => ['checkout'], + ], + ], + 'rollout_percentage' => 100, + ], + ], + ], + ]; + } + + private static function groupCheckout(?array $holdout): array + { + $flag = self::holdoutFlag($holdout); + $flag['filters']['aggregation_group_type_index'] = 0; + + return $flag; + } + + private function dependencyEvaluation( + array $flags, + string $distinctId, + array $groups, + bool $onlyEvaluateLocally, + array $remoteFlags = [] + ): array { + $httpClient = new MockedHttpClient( + 'app.posthog.com', + flagEndpointResponse: ['flags' => $flags, 'group_type_mapping' => ['0' => 'company']], + flagsEndpointResponse: $remoteFlags + ); + $client = new Client( + self::FAKE_API_KEY, + ['debug' => true, 'consumer' => 'noop'], + $httpClient, + null, + secretKey: 'test-secret-key' + ); + PostHog::init(null, null, $client); + + $evaluations = $client->evaluateFlags($distinctId, $groups, [], ['company' => []], $onlyEvaluateLocally); + $remoteCalls = array_values(array_filter( + $httpClient->calls ?? [], + fn ($call) => str_starts_with($call['path'], '/flags/?') + )); + + return [$evaluations, $remoteCalls]; + } + + public function testDependencyWithHoldoutAggregatedDifferentlyIsInconclusiveLocally(): void + { + $flags = [ + self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]), + self::bannerDependingOnCheckout('control'), + ]; + + [$evaluations, $remoteCalls] = $this->dependencyEvaluation($flags, 'user-1', ['company' => 'user-5'], true); + + self::assertSame('control', $evaluations->getFlag('checkout')); + self::assertNull($evaluations->getFlag('checkout-banner')); + self::assertSame([], $remoteCalls); + } + + public function testGroupDependencyOfPersonFlagWithHoldoutIsInconclusiveLocally(): void + { + $flags = [ + self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]), + self::bannerDependingOnCheckout('holdout-727'), + ]; + + [$evaluations, $remoteCalls] = $this->dependencyEvaluation($flags, 'user-5', ['company' => 'user-1'], true); + + self::assertSame('holdout-727', $evaluations->getFlag('checkout')); + self::assertNull($evaluations->getFlag('checkout-banner')); + self::assertSame([], $remoteCalls); + } + + public function testDependencyWithHoldoutAggregatedDifferentlyFallsBackToRemoteEvaluation(): void + { + $flags = [ + self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]), + self::bannerDependingOnCheckout('control'), + ]; + + [$evaluations, $remoteCalls] = $this->dependencyEvaluation( + $flags, + 'user-1', + ['company' => 'user-5'], + false, + ['flags' => [ + 'checkout-banner' => [ + 'key' => 'checkout-banner', + 'enabled' => true, + 'variant' => 'remote-banner-value', + ], + ]] + ); + + self::assertCount(1, $remoteCalls); + self::assertSame('remote-banner-value', $evaluations->getFlag('checkout-banner')); + } + + public function testDependencyWithHoldoutAggregatedTheSameStillEvaluatesLocally(): void + { + $banner = self::bannerDependingOnCheckout('holdout-727'); + $banner['filters']['aggregation_group_type_index'] = 0; + $flags = [self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]), $banner]; + + // The company key "user-1" buckets into the holdout; the distinct id "user-5" would not. + [$evaluations, $remoteCalls] = $this->dependencyEvaluation($flags, 'user-5', ['company' => 'user-1'], true); + + self::assertSame('holdout-727', $evaluations->getFlag('checkout')); + self::assertTrue($evaluations->getFlag('checkout-banner')); + self::assertSame([], $remoteCalls); + } + + public function testDependencyWithoutHoldoutAggregatedDifferentlyStillEvaluatesLocally(): void + { + $flags = [self::groupCheckout(null), self::bannerDependingOnCheckout('control')]; + + [$evaluations, $remoteCalls] = $this->dependencyEvaluation($flags, 'user-1', ['company' => 'user-5'], true); + + self::assertSame('control', $evaluations->getFlag('checkout')); + self::assertTrue($evaluations->getFlag('checkout-banner')); + self::assertSame([], $remoteCalls); + } + + public function testNestedDependencyKeepsTheOriginalIdentityAggregation(): void + { + $middle = self::bannerDependingOnCheckout('control'); + $middle['key'] = 'middle'; + $middle['filters']['aggregation_group_type_index'] = 0; + $top = self::bannerDependingOnCheckout('true'); + $top['key'] = 'top'; + $top['filters']['groups'][0]['properties'][0]['key'] = 'middle'; + $top['filters']['groups'][0]['properties'][0]['dependency_chain'] = ['checkout', 'middle']; + $flags = [self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]), $middle, $top]; + + // "top" is evaluated for a person, so the group holdout reached through the group-aggregated + // "middle" still has a mismatched identity. + [$evaluations, $remoteCalls] = $this->dependencyEvaluation($flags, 'user-1', ['company' => 'user-5'], true); + + self::assertNull($evaluations->getFlag('top')); + self::assertSame([], $remoteCalls); + } + private function localEvaluationSnapshot(array $localFlags, string $distinctId, array $groups = []) { $httpClient = new MockedHttpClient('app.posthog.com', flagEndpointResponse: $localFlags); From 464a3946b1d70e91f3294955fdb80d462e6bb494 Mon Sep 17 00:00:00 2001 From: Ioannis J Date: Thu, 22 Oct 2026 11:35:16 +0300 Subject: [PATCH 3/4] fix(flags): carry identity aggregation through cohort dependency evaluation --- lib/FeatureFlag.php | 71 ++++++++++++++++++--- test/FeatureFlagHoldoutTest.php | 106 +++++++++++++++++++++++++++----- 2 files changed, 153 insertions(+), 24 deletions(-) diff --git a/lib/FeatureFlag.php b/lib/FeatureFlag.php index a728064..7904842 100644 --- a/lib/FeatureFlag.php +++ b/lib/FeatureFlag.php @@ -200,6 +200,29 @@ public static function matchCohort( $evaluationCache = null, $distinctId = null, $propertyMatchingVersion = 1 + ) { + return FeatureFlag::matchCohortForIdentity( + $property, + $propertyValues, + $cohortProperties, + $flagsByKey, + $evaluationCache, + $distinctId, + $propertyMatchingVersion, + false + ); + } + + /** @param int|string|null|false $identityAggregation See evaluateFlagDependencyForIdentity(). */ + private static function matchCohortForIdentity( + $property, + $propertyValues, + $cohortProperties, + $flagsByKey, + $evaluationCache, + $distinctId, + $propertyMatchingVersion, + $identityAggregation ) { $cohortId = strval($property["value"]); if (!array_key_exists($cohortId, $cohortProperties)) { @@ -210,14 +233,15 @@ public static function matchCohort( } $propertyGroup = $cohortProperties[$cohortId]; - return FeatureFlag::matchPropertyGroup( + return FeatureFlag::matchPropertyGroupForIdentity( $propertyGroup, $propertyValues, $cohortProperties, $flagsByKey, $evaluationCache, $distinctId, - $propertyMatchingVersion + $propertyMatchingVersion, + $identityAggregation ); } @@ -243,6 +267,29 @@ public static function matchPropertyGroup( $evaluationCache = null, $distinctId = null, $propertyMatchingVersion = 1 + ) { + return FeatureFlag::matchPropertyGroupForIdentity( + $propertyGroup, + $propertyValues, + $cohortProperties, + $flagsByKey, + $evaluationCache, + $distinctId, + $propertyMatchingVersion, + false + ); + } + + /** @param int|string|null|false $identityAggregation See evaluateFlagDependencyForIdentity(). */ + private static function matchPropertyGroupForIdentity( + $propertyGroup, + $propertyValues, + $cohortProperties, + $flagsByKey, + $evaluationCache, + $distinctId, + $propertyMatchingVersion, + $identityAggregation ) { if (!$propertyGroup) { return true; @@ -262,14 +309,15 @@ public static function matchPropertyGroup( // a nested property group foreach ($properties as $prop) { try { - $matches = FeatureFlag::matchPropertyGroup( + $matches = FeatureFlag::matchPropertyGroupForIdentity( $prop, $propertyValues, $cohortProperties, $flagsByKey, $evaluationCache, $distinctId, - $propertyMatchingVersion + $propertyMatchingVersion, + $identityAggregation ); if ($propertyGroupType === 'AND') { if (!$matches) { @@ -300,24 +348,26 @@ public static function matchPropertyGroup( $matches = false; $propType = $prop["type"] ?? null; if ($propType === 'cohort') { - $matches = FeatureFlag::matchCohort( + $matches = FeatureFlag::matchCohortForIdentity( $prop, $propertyValues, $cohortProperties, $flagsByKey, $evaluationCache, $distinctId, - $propertyMatchingVersion + $propertyMatchingVersion, + $identityAggregation ); } elseif ($propType === 'flag') { - $matches = FeatureFlag::evaluateFlagDependency( + $matches = FeatureFlag::evaluateFlagDependencyForIdentity( $prop, $flagsByKey, $evaluationCache, $distinctId, $propertyValues, $cohortProperties, - $propertyMatchingVersion + $propertyMatchingVersion, + $identityAggregation ); } else { $matches = FeatureFlag::matchProperty($prop, $propertyValues, $propertyMatchingVersion); @@ -1134,14 +1184,15 @@ private static function isConditionMatch( $matches = false; $propertyType = $property['type'] ?? null; if ($propertyType == 'cohort') { - $matches = FeatureFlag::matchCohort( + $matches = FeatureFlag::matchCohortForIdentity( $property, $properties, $cohorts, $flagsByKey, $evaluationCache, $distinctId, - $propertyMatchingVersion + $propertyMatchingVersion, + $identityAggregation ); } elseif ($propertyType == 'flag') { $matches = FeatureFlag::evaluateFlagDependencyForIdentity( diff --git a/test/FeatureFlagHoldoutTest.php b/test/FeatureFlagHoldoutTest.php index 447ea0e..e1d1162 100644 --- a/test/FeatureFlagHoldoutTest.php +++ b/test/FeatureFlagHoldoutTest.php @@ -10,6 +10,7 @@ use PHPUnit\Framework\TestCase; use PostHog\Client; use PostHog\FeatureFlag; +use PostHog\InconclusiveMatchException; use PostHog\PostHog; class FeatureFlagHoldoutTest extends TestCase @@ -204,29 +205,34 @@ public function testGroupAggregatedFlagUsesTheGroupKeyForMembership(): void } private static function bannerDependingOnCheckout(string $expectedValue): array + { + return self::checkoutBanner([self::checkoutDependencyProperty($expectedValue)]); + } + + private static function checkoutBanner(array $properties): array { return [ 'key' => 'checkout-banner', 'active' => true, 'filters' => [ 'groups' => [ - [ - 'properties' => [ - [ - 'key' => 'checkout', - 'type' => 'flag', - 'operator' => 'flag_evaluates_to', - 'value' => $expectedValue, - 'dependency_chain' => ['checkout'], - ], - ], - 'rollout_percentage' => 100, - ], + ['properties' => $properties, 'rollout_percentage' => 100], ], ], ]; } + private static function checkoutDependencyProperty(string $expectedValue): array + { + return [ + 'key' => 'checkout', + 'type' => 'flag', + 'operator' => 'flag_evaluates_to', + 'value' => $expectedValue, + 'dependency_chain' => ['checkout'], + ]; + } + private static function groupCheckout(?array $holdout): array { $flag = self::holdoutFlag($holdout); @@ -240,11 +246,16 @@ private function dependencyEvaluation( string $distinctId, array $groups, bool $onlyEvaluateLocally, - array $remoteFlags = [] + array $remoteFlags = [], + array $cohorts = [] ): array { $httpClient = new MockedHttpClient( 'app.posthog.com', - flagEndpointResponse: ['flags' => $flags, 'group_type_mapping' => ['0' => 'company']], + flagEndpointResponse: [ + 'flags' => $flags, + 'group_type_mapping' => ['0' => 'company'], + 'cohorts' => $cohorts, + ], flagsEndpointResponse: $remoteFlags ); $client = new Client( @@ -362,6 +373,73 @@ public function testNestedDependencyKeepsTheOriginalIdentityAggregation(): void self::assertSame([], $remoteCalls); } + private static function checkoutCohort(string $expectedValue): array + { + return [42 => ['type' => 'AND', 'values' => [self::checkoutDependencyProperty($expectedValue)]]]; + } + + private static function bannerTargetingCheckoutCohort(): array + { + return self::checkoutBanner([['key' => 'id', 'type' => 'cohort', 'value' => 42]]); + } + + private static function matchBannerAgainstHoldoutCohort(array $banner) + { + $checkout = self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]); + $cache = []; + + return FeatureFlag::matchFeatureFlagProperties( + $banner, + 'user-1', + [], + self::checkoutCohort('holdout-727'), + ['checkout' => $checkout, 'checkout-banner' => $banner], + $cache + ); + } + + public function testCohortDependencyWithHoldoutAggregatedDifferentlyIsInconclusiveLocally(): void + { + // Hashing the person id "user-1" would land in the holdout, but "checkout" buckets by company key. + $this->expectException(InconclusiveMatchException::class); + self::matchBannerAgainstHoldoutCohort(self::bannerTargetingCheckoutCohort()); + } + + public function testCohortDependencyWithHoldoutAggregatedTheSameStillEvaluatesLocally(): void + { + $banner = self::bannerTargetingCheckoutCohort(); + $banner['filters']['aggregation_group_type_index'] = 0; + + // The company key "user-1" buckets into the holdout. + self::assertTrue(self::matchBannerAgainstHoldoutCohort($banner)); + } + + public function testCohortDependencyWithHoldoutAggregatedDifferentlyFallsBackToRemoteEvaluation(): void + { + $flags = [ + self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]), + self::bannerTargetingCheckoutCohort(), + ]; + + [$evaluations, $remoteCalls] = $this->dependencyEvaluation( + $flags, + 'user-1', + ['company' => 'user-5'], + false, + ['flags' => [ + 'checkout-banner' => [ + 'key' => 'checkout-banner', + 'enabled' => true, + 'variant' => 'remote-banner-value', + ], + ]], + self::checkoutCohort('holdout-727') + ); + + self::assertCount(1, $remoteCalls); + self::assertSame('remote-banner-value', $evaluations->getFlag('checkout-banner')); + } + private function localEvaluationSnapshot(array $localFlags, string $distinctId, array $groups = []) { $httpClient = new MockedHttpClient('app.posthog.com', flagEndpointResponse: $localFlags); From f9ca21d913341d99be3e4cc1f87328c0d7d4c69e Mon Sep 17 00:00:00 2001 From: Ioannis J Date: Thu, 22 Oct 2026 11:41:33 +0300 Subject: [PATCH 4/4] test(flags): cover nested cohort and property-group holdout dependencies --- test/FeatureFlagHoldoutTest.php | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/test/FeatureFlagHoldoutTest.php b/test/FeatureFlagHoldoutTest.php index e1d1162..f8f114d 100644 --- a/test/FeatureFlagHoldoutTest.php +++ b/test/FeatureFlagHoldoutTest.php @@ -383,7 +383,7 @@ private static function bannerTargetingCheckoutCohort(): array return self::checkoutBanner([['key' => 'id', 'type' => 'cohort', 'value' => 42]]); } - private static function matchBannerAgainstHoldoutCohort(array $banner) + private static function matchBannerAgainstHoldoutCohort(array $banner, ?array $cohorts = null) { $checkout = self::groupCheckout(['id' => 727, 'exclusion_percentage' => 20]); $cache = []; @@ -392,7 +392,7 @@ private static function matchBannerAgainstHoldoutCohort(array $banner) $banner, 'user-1', [], - self::checkoutCohort('holdout-727'), + $cohorts ?? self::checkoutCohort('holdout-727'), ['checkout' => $checkout, 'checkout-banner' => $banner], $cache ); @@ -414,6 +414,25 @@ public function testCohortDependencyWithHoldoutAggregatedTheSameStillEvaluatesLo self::assertTrue(self::matchBannerAgainstHoldoutCohort($banner)); } + public function testNestedPropertyGroupInCohortKeepsTheIdentityAggregation(): void + { + $dependency = self::checkoutDependencyProperty('holdout-727'); + $cohorts = [42 => ['type' => 'AND', 'values' => [['type' => 'OR', 'values' => [$dependency]]]]]; + + $this->expectException(InconclusiveMatchException::class); + self::matchBannerAgainstHoldoutCohort(self::bannerTargetingCheckoutCohort(), $cohorts); + } + + public function testNestedCohortKeepsTheIdentityAggregation(): void + { + $cohorts = self::checkoutCohort('holdout-727'); + $cohorts[43] = $cohorts[42]; + $cohorts[42] = ['type' => 'AND', 'values' => [['key' => 'id', 'type' => 'cohort', 'value' => 43]]]; + + $this->expectException(InconclusiveMatchException::class); + self::matchBannerAgainstHoldoutCohort(self::bannerTargetingCheckoutCohort(), $cohorts); + } + public function testCohortDependencyWithHoldoutAggregatedDifferentlyFallsBackToRemoteEvaluation(): void { $flags = [