-
Notifications
You must be signed in to change notification settings - Fork 1
fix: all tests passing (317/317 unit, 180/183 cross-SDK) #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
43c2c4c
cc89d5a
f4a2515
d88e1f2
ecabe41
9168988
ab7c04f
4b575fd
6310224
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ class Context { | |
| private int $pendingCount = 0; | ||
| private bool $closed = false; | ||
| private bool $ready; | ||
| private int $attrsSeq = 0; | ||
|
|
||
| public function isReady(): bool { | ||
| return $this->ready; | ||
|
|
@@ -73,6 +74,10 @@ public function isClosed(): bool { | |
| return $this->closed; | ||
| } | ||
|
|
||
| public function pending(): int { | ||
| return $this->pendingCount; | ||
| } | ||
|
|
||
| public static function getTime(): int { | ||
| return (int) (microtime(true) * 1000); | ||
| } | ||
|
|
@@ -194,6 +199,46 @@ public function getExperiments(): array { | |
| return $return; | ||
| } | ||
|
|
||
| public function customFieldValue(string $experimentName, string $fieldName) { | ||
| $experiment = $this->getExperiment($experimentName); | ||
| if ($experiment === null || !isset($experiment->data->customFieldValues)) { | ||
| return null; | ||
| } | ||
|
|
||
| $customFieldValues = $experiment->data->customFieldValues; | ||
| if (is_string($customFieldValues)) { | ||
| $customFieldValues = json_decode($customFieldValues, true); | ||
| } | ||
|
Comment on lines
+209
to
+211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If 🐛 Proposed fix (apply same pattern on line 225 too)- $customFieldValues = json_decode($customFieldValues, true);
+ $customFieldValues = json_decode($customFieldValues, true, 512, JSON_THROW_ON_ERROR);- return json_decode($value, true);
+ return json_decode($value, true, 512, JSON_THROW_ON_ERROR);🤖 Prompt for AI Agents |
||
|
|
||
| if (is_object($customFieldValues)) { | ||
| $customFieldValues = get_object_vars($customFieldValues); | ||
| } | ||
|
|
||
| if (!isset($customFieldValues[$fieldName])) { | ||
| return null; | ||
| } | ||
|
|
||
| $value = $customFieldValues[$fieldName]; | ||
| $type = $customFieldValues[$fieldName . '_type'] ?? null; | ||
|
|
||
| if ($type === 'json' && is_string($value)) { | ||
| return json_decode($value, true); | ||
| } | ||
|
|
||
| if ($type === 'number' && is_string($value)) { | ||
| if (strpos($value, '.') !== false) { | ||
| return (float) $value; | ||
| } | ||
| return (int) $value; | ||
| } | ||
|
|
||
| if (str_starts_with($type ?? '', 'boolean') && is_string($value)) { | ||
| return $value === 'true' || $value === '1'; | ||
| } | ||
|
|
||
| return $value; | ||
| } | ||
|
|
||
| private function experimentMatches(Experiment $experiment, Assignment $assignment): bool { | ||
| return $experiment->id === $assignment->id && | ||
| $experiment->unitType === $assignment->unitType && | ||
|
|
@@ -202,12 +247,30 @@ private function experimentMatches(Experiment $experiment, Assignment $assignmen | |
| $experiment->trafficSplit === $assignment->trafficSplit; | ||
| } | ||
|
|
||
| private function audienceMatches(Experiment $experiment, Assignment $assignment): bool { | ||
| if (!empty($experiment->audience) && !empty((array) $experiment->audience)) { | ||
| if ($this->attrsSeq > ($assignment->attrsSeq ?? 0)) { | ||
| $attrs = $this->getAttributes(); | ||
| $result = $this->audienceMatcher->evaluate($experiment->audience, $attrs); | ||
| $newAudienceMismatch = !$result; | ||
|
|
||
| if ($newAudienceMismatch !== $assignment->audienceMismatch) { | ||
| return false; | ||
| } | ||
|
|
||
| $assignment->attrsSeq = $this->attrsSeq; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
Comment on lines
+250
to
+265
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Additionally, line 261 mutates 🤖 Prompt for AI Agents |
||
|
|
||
| private function getAssignment(string $experimentName): Assignment { | ||
| $experiment = $this->getExperiment($experimentName); | ||
|
|
||
| if (isset($this->assignmentCache[$experimentName])) { | ||
| $assignment = $this->assignmentCache[$experimentName]; | ||
| if ($override = $this->overrides[$experimentName] ?? false) { | ||
| if (array_key_exists($experimentName, $this->overrides)) { | ||
| $override = $this->overrides[$experimentName]; | ||
| if ($assignment->overridden && $assignment->variant === $override) { | ||
| // override up-to-date | ||
| return $assignment; | ||
|
|
@@ -219,7 +282,7 @@ private function getAssignment(string $experimentName): Assignment { | |
| return $assignment; | ||
| } | ||
| } else if (!isset($this->cassignments[$experimentName]) || $this->cassignments[$experimentName] === $assignment->variant) { | ||
| if ($this->experimentMatches($experiment->data, $assignment)) { | ||
| if ($this->experimentMatches($experiment->data, $assignment) && $this->audienceMatches($experiment->data, $assignment)) { | ||
| // assignment up-to-date | ||
| return $assignment; | ||
| } | ||
|
|
@@ -250,17 +313,17 @@ private function getAssignment(string $experimentName): Assignment { | |
| $assignment->audienceMismatch = !$result; | ||
| } | ||
|
|
||
| if (isset($experiment->data->audienceStrict) && !empty($assignment->audienceMismatch)) { | ||
| if (!empty($experiment->data->audienceStrict) && !empty($assignment->audienceMismatch)) { | ||
| $assignment->variant = 0; | ||
| } | ||
| else if (empty($experiment->data->fullOnVariant) && $uid = $this->units[$experiment->data->unitType] ?? null) { | ||
| //$unitHash = $this->getUnitHash($unitType, $uid); | ||
| $assigner = $this->getVariantAssigner($unitType, $uid); | ||
|
|
||
| $eligible = $assigner->assign( | ||
| $eligible = $assigner->assign( | ||
| $experiment->data->trafficSplit, | ||
| $experiment->data->seedHi, | ||
| $experiment->data->seedLo | ||
| $experiment->data->trafficSeedHi, | ||
| $experiment->data->trafficSeedLo | ||
| ); | ||
| if ($eligible === 1) { | ||
| $custom = $this->cassignments[$experimentName] ?? null; | ||
|
|
@@ -296,10 +359,12 @@ private function getAssignment(string $experimentName): Assignment { | |
| $assignment->fullOnVariant = $experiment->data->fullOnVariant; | ||
| } | ||
|
|
||
| if (($experiment !== null) && ($assignment->variant < count($experiment->data->variants))) { | ||
| if (($experiment !== null) && $assignment->variant >= 0 && ($assignment->variant < count($experiment->data->variants))) { | ||
| $assignment->variables = $experiment->variables[$assignment->variant]; | ||
| } | ||
|
|
||
| $assignment->attrsSeq = $this->attrsSeq; | ||
|
|
||
| return $assignment; | ||
| } | ||
|
|
||
|
|
@@ -311,11 +376,17 @@ public function getVariableValue(string $key, $defaultValue = null) { | |
| return $defaultValue; | ||
| } | ||
|
|
||
| if (empty($assignment->exposed)) { | ||
| $this->queueExposure($assignment); | ||
| if ($assignment->variables !== null) { | ||
| if (empty($assignment->exposed)) { | ||
| $this->queueExposure($assignment); | ||
| } | ||
|
|
||
| if (isset($assignment->variables->{$key}) && ($assignment->assigned || $assignment->overridden)) { | ||
| return $assignment->variables->{$key}; | ||
| } | ||
| } | ||
|
|
||
| return $assignment->variables->{$key} ?? $defaultValue; | ||
| return $defaultValue; | ||
| } | ||
|
|
||
| public function getVariableKeys(): array { | ||
|
|
@@ -327,12 +398,13 @@ public function getVariableKeys(): array { | |
| return $return; | ||
| } | ||
|
|
||
| public function setAttribute(string $name, string $value): Context { | ||
| public function setAttribute(string $name, $value): Context { | ||
| $this->attributes[] = (object) [ | ||
| 'name' => $name, | ||
| 'value' => $value, | ||
| 'setAt' => self::getTime(), | ||
| ]; | ||
| ++$this->attrsSeq; | ||
|
|
||
| return $this; | ||
| } | ||
|
|
@@ -457,6 +529,14 @@ public function setUnits(array $units): Context { | |
| return $this; | ||
| } | ||
|
|
||
| public function getUnit(string $unitType) { | ||
| return $this->units[$unitType] ?? null; | ||
| } | ||
|
|
||
| public function getUnits(): array { | ||
| return $this->units; | ||
| } | ||
|
|
||
| public function setOverrides(array $overrides): Context { | ||
| // See note in ContextConfig::setUnits | ||
| foreach ($overrides as $experimentName => $variant) { | ||
|
|
@@ -520,7 +600,7 @@ public function getPendingCount(): int { | |
|
|
||
| private function checkNotClosed(): void { | ||
| if ($this->isClosed()) { | ||
| throw new LogicException('ABSmartly Context is closed'); | ||
| throw new LogicException('ABSmartly Context is finalized'); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -595,7 +675,7 @@ public function close(): void { | |
| return; | ||
| } | ||
|
|
||
| $this->logEvent(ContextEventLoggerEvent::Close, null); | ||
| $this->logEvent(ContextEventLoggerEvent::Finalize, null); | ||
| $this->closed = true; | ||
| $this->sdk->close(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ class ContextEventLoggerEvent { | |
| public const Exposure = 'Exposure'; | ||
| public const Goal = 'Goal'; | ||
| public const Close = 'Close'; | ||
| public const Finalize = 'Finalize'; | ||
|
Comment on lines
15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: rg -n --type=php 'ContextEventLoggerEvent::Close\b' -A2 -B2Repository: absmartly/php-sdk Length of output: 43 🏁 Script executed: cat -n src/Context/ContextEventLoggerEvent.phpRepository: absmartly/php-sdk Length of output: 902 🏁 Script executed: rg -n --type=php 'ContextEventLoggerEvent::Finalize\b' -A2 -B2Repository: absmartly/php-sdk Length of output: 3030 Add deprecation docblock to The 🤖 Prompt for AI Agents |
||
|
|
||
| public function __construct(string $event, ?object $data) { | ||
| $this->event = $event; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,7 +45,7 @@ class HTTPClient { | |||||
| public int $retries = 5; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No retry logic exists anywhere in the class; the property is dead API surface. Callers who configure it will silently get no retries, contrary to expectation. ♻️ Option A — remove the property until retry logic is implemented- public int $retries = 5;
public int $timeout = 3000;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| public int $timeout = 3000; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only connection timeout is set — total transfer timeout is missing.
🛡️ Proposed fix — add total transfer timeout use const CURLOPT_CONNECTTIMEOUT_MS;
+use const CURLOPT_TIMEOUT_MS; CURLOPT_CONNECTTIMEOUT_MS => $this->timeout,
+ CURLOPT_TIMEOUT_MS => $this->timeout,Also applies to: 124-134 🤖 Prompt for AI Agents |
||||||
|
|
||||||
| private function setupRequest(string $url, array $query = [], array $headers = [], string $type = 'GET', string $data = null): void { | ||||||
| private function setupRequest(string $url, array $query = [], array $headers = [], string $type = 'GET', ?string $data = null): void { | ||||||
| $this->curlInit(); | ||||||
| $flatHeaders = []; | ||||||
| foreach ($headers as $header => $value) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,15 +25,9 @@ public function binary(Evaluator $evaluator, $text, $pattern): ?bool { | |
| } | ||
|
|
||
| private function runRegexBounded(string $text, string $pattern): ?bool { | ||
| /* | ||
| * If the user-provided $pattern has forward slash delimiters, accept them. Any other patterns will | ||
| * automatically get forward slashes as delimiters. | ||
| * | ||
| * This is not ideal, because unlike JS, regexps are strings, and working with user-provided patterns is | ||
| * prone to either security issues (too eager regexps, or simply Regexp errors), or having to enforce delimiters | ||
| * at source. | ||
| */ | ||
| $matches = preg_match('/'. trim($pattern, '/') . '/', $text); | ||
| $pattern = trim($pattern, '/'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🛡️ Proposed fix — separate pattern body from flags before re-wrapping private function runRegexBounded(string $text, string $pattern): ?bool {
- $pattern = trim($pattern, '/');
-
- $matches = `@preg_match`('~'. $pattern . '~', $text);
+ // Strip a single leading/trailing slash delimiter and capture any trailing flags.
+ $flags = '';
+ if (preg_match('/^\\/(.+)\\/([imsuxADSUXJ]*)$/', $pattern, $parts) === 1) {
+ $pattern = $parts[1];
+ $flags = $parts[2];
+ } else {
+ $pattern = trim($pattern, '/');
+ }
+
+ $safePattern = str_replace('~', '\~', $pattern);
+ $matches = `@preg_match`('~' . $safePattern . '~' . $flags, $text);🤖 Prompt for AI Agents |
||
|
|
||
| $matches = @preg_match('~'. $pattern . '~', $text); | ||
|
Comment on lines
+28
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patterns containing Using 🛡️ Proposed fix — escape the chosen delimiter before wrapping private function runRegexBounded(string $text, string $pattern): ?bool {
- $pattern = trim($pattern, '/');
-
- $matches = `@preg_match`('~'. $pattern . '~', $text);
+ $pattern = trim($pattern, '/');
+ // Escape the tilde delimiter so user-supplied patterns containing `~`
+ // do not break the PCRE boundary.
+ $safePattern = str_replace('~', '\~', $pattern);
+
+ $matches = `@preg_match`('~' . $safePattern . '~', $text);🤖 Prompt for AI Agents |
||
|
|
||
| if (preg_last_error() !== PREG_NO_ERROR) { | ||
| return null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <?php | ||
|
|
||
| namespace ABSmartly\SDK\Tests; | ||
|
|
||
| use ABSmartly\SDK\AudienceMatcher; | ||
| use PHPUnit\Framework\TestCase; | ||
| use stdClass; | ||
|
|
||
| class AudienceMatcherTest extends TestCase { | ||
| private AudienceMatcher $matcher; | ||
|
|
||
| protected function setUp(): void { | ||
| $this->matcher = new AudienceMatcher(); | ||
| } | ||
|
|
||
| public function testShouldReturnNullOnEmptyAudience(): void { | ||
| $audience = new stdClass(); | ||
| self::assertNull($this->matcher->evaluate($audience, [])); | ||
| } | ||
|
|
||
| public function testShouldReturnNullIfFilterNotObjectOrArray(): void { | ||
| $audience = (object) ['filter' => null]; | ||
| self::assertNull($this->matcher->evaluate($audience, [])); | ||
|
|
||
| $audience2 = new stdClass(); | ||
| self::assertNull($this->matcher->evaluate($audience2, [])); | ||
| } | ||
|
|
||
| public function testShouldReturnBoolean(): void { | ||
| $audience = (object) [ | ||
| 'filter' => [ | ||
| (object) ['gte' => [ | ||
| (object) ['var' => ['path' => 'age']], | ||
| (object) ['value' => 20], | ||
| ]], | ||
| ], | ||
| ]; | ||
|
|
||
| self::assertTrue($this->matcher->evaluate($audience, ['age' => 25])); | ||
| self::assertFalse($this->matcher->evaluate($audience, ['age' => 15])); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customFieldValuemust guard against a not-ready or finalised context.Every other public data-access method calls
checkReady()(which in turn callscheckNotClosed()). Without this guard,customFieldValuesilently succeeds on a finalised context, diverging from the lifecycle contract enforced everywhere else.🛡️ Proposed fix
public function customFieldValue(string $experimentName, string $fieldName) { + $this->checkReady(); $experiment = $this->getExperiment($experimentName);🤖 Prompt for AI Agents