fix(context): forward all BucketingAttributes to ExperienceManager - #37
Merged
Merged
Conversation
Context::runExperience and Context::runExperiences rebuilt a fresh BucketingAttributes object with only four hardcoded keys (visitorProperties, locationProperties, updateVisitorProperties, environment) before forwarding to ExperienceManager::selectVariation and selectVariations. This silently dropped every other BucketingAttributes field — enableTracking, forceVariationId, ignoreLocationProperties, typeCasting, experienceKeys — so callers opting out of tracking, forcing a variation, or ignoring location targeting were silently ignored. DataManager destructures these by name with enableTracking defaulting to true (DataManager::531), which is how the opt-out was being lost. Switch to get_object_vars-spread-then-override so all caller-supplied attributes pass through while still applying the two transforms Context must do: merging visitorProperties via getVisitorProperties() and defaulting environment to the Context's own environment. Add tests covering forwarding of enableTracking, forceVariationId, ignoreLocationProperties, and updateVisitorProperties through both runExperience and runExperiences. Mirrors convertcom/javascript-sdk#382 — same bug, same fix shape adapted to PHP. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
abbaseya
requested review from
DmytroConvert and
JosephSamirL
and removed request for
JosephSamirL
May 21, 2026 01:07
There was a problem hiding this comment.
Code Review
This pull request updates the runExperience and runExperiences methods in Context.php to ensure all bucketing attributes are correctly forwarded by dynamically capturing object variables. New unit tests were added to verify that attributes like enableTracking and forceVariationId are preserved. Feedback suggests using clone instead of get_object_vars for better performance and maintainability, and recommends refactoring the duplicated logic into a helper method to ensure consistency across the SDK.
DmytroConvert
approved these changes
May 21, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Context::runExperienceandContext::runExperiencesrebuilt a freshBucketingAttributesobject with only four hardcoded keys (visitorProperties,locationProperties,updateVisitorProperties,environment) before forwarding toExperienceManager::selectVariation/selectVariations. Every otherBucketingAttributesfield —enableTracking,forceVariationId,ignoreLocationProperties,typeCasting,experienceKeys— was silently dropped.The most user-visible consequence: a caller invoking
$context->runExperience($key, new BucketingAttributes(['enableTracking' => false]))still fires the bucketing track event, because by the time the call reachesDataManager::_getBucketingByField(which reads$attributes->enableTracking ?? true), the field has been discarded one layer up. Same forforceVariationId(the forced variation was never honored) andignoreLocationProperties.Root cause
A whitelist-then-rebuild pattern in
Contextinstead of spread-then-override. Two transforms must happen at the Context layer:visitorPropertiesmust be merged with stored visitor props viagetVisitorProperties().environmentmust default to$this->environmentwhen the caller omits it.Everything else can pass through unmodified;
BucketingAttributes::__constructalready does?? nulldefaulting per key, so extra/unknown fields are safe.Fix
Applied identically to
runExperienceandrunExperiences.get_object_vars()is the PHP analog to JS's object-spread; it copies all public properties of$attributesinto an array, then we override the two we need to transform.Tests
Added two cases in
packages/Php-sdk/tests/ContextTest.php:testRunExperienceForwardsAllBucketingAttributestestRunExperiencesForwardsAllBucketingAttributesBoth wire a PHPUnit mock of
ExperienceManagerInterfacethat captures theBucketingAttributespassed in and delegates to the real manager. They assert thatenableTracking,forceVariationId,ignoreLocationProperties, andupdateVisitorPropertiessurvive the trip throughContext. UseenableTracking: falseto keep the path side-effect-free.Test plan
phpunit packages/Php-sdk/tests/ContextTest.php→ 39 tests pass, 208 assertionsphpunit packages/Php-sdk/tests/→ 176 tests pass, 539 assertions (no regressions)testRunExperienceandtestGetVariationsAcrossAllExperiencesstill pass — happy path unchangedNotes
BucketingAttributestype all along.BucketingAttributestype,DataManager,ExperienceManager, or any downstream layer.🤖 Generated with Claude Code