Open
Conversation
- Add customFieldValue() method to Context for custom field access - Add Finalize event constant to ContextEventLoggerEvent - Add customFieldValues property to Experiment class - Fix Assignment class: make variables nullable with null default - Fix Assignment class: set exposed default to false - Fix getAssignment() to use array_key_exists() for override check (fixes variant 0) - Fix audienceStrict check from isset() to !empty() (scenario 44 fix) - Fix getVariableValue() to check variables before queueing exposure
- Fix implicit nullable parameter deprecation in HTTPClient::setupRequest - Update test expectations for Finalize event (was incorrectly expecting Close) - Fix type coercion in tests - SDK correctly preserves numeric attribute types
Match JavaScript SDK behavior: queue exposure when assignment.variables is not null, regardless of whether the specific variable key exists. Only return the variable value if the key exists AND the user is assigned or has an override. This fixes exposure tracking for cases where the user is assigned a variant that doesn't contain the specific variable being requested.
Change seedHi/seedLo to trafficSeedHi/trafficSeedLo to match the experiment data structure for traffic split assignment.
- Add failed state testing - Add attribute management tests - Add timeout/retry configuration tests - Add error handling tests - Add event handler scenario tests - Add integration scenario tests Total: 30 new tests added, all 142 tests pass
Create MD5Test (14), Murmur3Test (36), AudienceMatcherTest (3) with parameterized data providers. Refactor VariantAssignerTest to 42 individual cases. Add ~76 context tests covering event logger, exposure queuing, variable values, publish lifecycle, refresh cache invalidation, custom fields, and audience matching.
Fix InOperator and MatchOperator for proper type handling. Add async interfaces for Client, HttpClient, ContextDataProvider, and ContextEventHandler. Add RuntimeException and ABsmartly facade class.
WalkthroughThis PR introduces a new high-level ABsmartly facade class and deprecates the previous SDK class. It adds synchronous and asynchronous client/HTTP interfaces (ClientInterface, AsyncClientInterface, HttpClientInterface, AsyncHttpClientInterface) and implements an async ReactHttpClient plus retry/timeout logic in HTTP clients. The Client class gains async methods with sync fallbacks. Context lifecycle is extended with pending/ready/finalize states, attribute sequencing, custom-field support and pending-context flows; ContextEventHandler is replaced by ContextPublisher (deprecated alias kept). AsyncContextDataProvider and AsyncContextEventHandler are added. Assignment, Experiment and other model defaults were adjusted. A RuntimeException class, composer dev deps for ReactPHP, extensive README updates, many tests and fixtures, and miscellaneous parsing/operator changes were also included. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
createWithDefaults() had swapped environment/application params. Add createSimple() with correct order matching ClientConfig constructor. Deprecate createWithDefaults() for backward compatibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (9)
tests/JsonExpression/Operator/InOperatorTest.php (1)
64-67: Consider renaming this test method for clarity.The method name
testArrayDiffNullsuggests it tests null behaviour, but it actually verifies that sub-arrays are not found as elements within other arrays. A clearer name might betestArrayNotFoundAsElementortestNestedArrayNotContained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/JsonExpression/Operator/InOperatorTest.php` around lines 64 - 67, Rename the test method testArrayDiffNull to a clearer name that reflects its behavior (e.g., testArrayNotFoundAsElement or testNestedArrayNotContained) and update any references to it; specifically change the method name in the test class containing the method testArrayDiffNull and ensure any test runner or annotations still recognize it (the body calling $this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]]) etc. should remain unchanged).src/Experiment.php (1)
36-38: Consider importing the exception class.The exception is referenced using its fully-qualified class name. For consistency with other files (e.g.,
ContextConfig.phpwhich imports the exception), consider adding an import statement.♻️ Proposed refactor
Add at the top of the file:
use ABSmartly\SDK\Exception\RuntimeException;Then simplify the throw statement:
- throw new \ABSmartly\SDK\Exception\RuntimeException( + throw new RuntimeException( sprintf('Missing required field "%s" in experiment data', $field) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Experiment.php` around lines 36 - 38, Import the RuntimeException class and use the short name in the throw to match other files: add a top-level use statement for ABSmartly\SDK\Exception\RuntimeException and replace the fully-qualified instantiation in the throw (the throw new \ABSmartly\SDK\Exception\RuntimeException(sprintf(...))) with throw new RuntimeException(sprintf(...))); locate this change around the throw in Experiment.php to keep consistency with files like ContextConfig.php.src/JsonExpression/Operator/MatchOperator.php (1)
27-36: Verify that aggressive delimiter trimming is the intended behaviour.Using
trim($pattern, '/')removes all leading and trailing/characters, not just a single delimiter pair. For example, a pattern like//test//becomestestrather than/test/. If patterns legitimately start or end with/(e.g., matching paths like/api/), this could alter the matching behaviour unexpectedly.If the intent is to strip only wrapping delimiters, consider a more precise approach:
♻️ Proposed refinement to strip only a single delimiter pair
private function runRegexBounded(string $text, string $pattern): ?bool { - $pattern = trim($pattern, '/'); + if (strlen($pattern) >= 2 && $pattern[0] === '/' && $pattern[-1] === '/') { + $pattern = substr($pattern, 1, -1); + } $matches = `@preg_match`('~'. $pattern . '~', $text);If this aggressive trimming is intentional for cross-SDK consistency, please confirm that the other SDK implementations behave identically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/JsonExpression/Operator/MatchOperator.php` around lines 27 - 36, The code in runRegexBounded uses trim($pattern, '/') which removes all leading/trailing slashes; change it to strip only a single wrapping delimiter pair: if (substr($pattern, 0, 1) === '/' && substr($pattern, -1) === '/') { $pattern = substr($pattern, 1, -1); } so patterns like //test// or paths like /api/ are not mangled; update runRegexBounded to use this check before calling preg_match and keep the rest of the error handling unchanged (and confirm consistent behaviour with other SDKs if cross-SDK consistency was intended).src/Http/ReactHttpClient.php (1)
79-79: Consider adding type hint for$reactResponseparameter.The parameter lacks a type declaration. Adding
\Psr\Http\Message\ResponseInterface(or the specific React response type) would improve type safety and IDE support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/ReactHttpClient.php` at line 79, The toResponse method signature lacks a parameter type; update the private function toResponse($reactResponse): Response to include a type hint (e.g. private function toResponse(\Psr\Http\Message\ResponseInterface $reactResponse): Response or the concrete React response type) and adjust imports or use the fully-qualified name; then update any callers if necessary to satisfy the stricter type and ensure IDE/type-checking benefits.tests/Context/AsyncContextDataProviderTest.php (1)
35-52: Consider usingawait()for more robust promise testing.The test relies on the synchronous resolution behaviour of
resolve()in react/promise 3.x. While this works, it's somewhat implicit. UsingReact\Async\await()would make the test more explicit and resilient to future changes.♻️ Proposed refactor using await()
+use function React\Async\await; + public function testGetContextDataAsyncResolvesToContextData(): void { $client = $this->createMock(AsyncClientInterface::class); $contextData = new ContextData(); $contextData->experiments = []; $client->method('getContextDataAsync') ->willReturn(resolve($contextData)); $provider = new AsyncContextDataProvider($client); $promise = $provider->getContextDataAsync(); - $result = null; - $promise->then(function($data) use (&$result) { - $result = $data; - }); - - self::assertSame($contextData, $result); + $result = await($promise); + self::assertSame($contextData, $result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/AsyncContextDataProviderTest.php` around lines 35 - 52, The test testGetContextDataAsyncResolvesToContextData should use React\Async\await() instead of manually attaching a then callback to the promise to make the assertion robust; update the test so the mock client still returns resolve($contextData) and then call await($provider->getContextDataAsync()) to obtain $result synchronously before asserting assertSame($contextData, $result), referencing AsyncContextDataProvider::getContextDataAsync and the resolve() usage when making the change.tests/MD5Test.php (1)
30-37: ExercisePublishEvent::hashUnit()directly.This currently proves PHP's MD5/base64url primitives, not the SDK code path. If
PublishEvent::hashUnit()regresses, the test still passes.💡 Proposed change
+use ABSmartly\SDK\PublishEvent; ... public function testShouldMatchKnownHashes(string $input, string $expectedHash): void { - $hash = hash('md5', $input, true); - $base64url = strtr(base64_encode($hash), [ - '+' => '-', - '/' => '_', - '=' => '', - ]); - self::assertSame($expectedHash, $base64url); + $event = new PublishEvent(); + self::assertSame($expectedHash, $event->hashUnit($input)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MD5Test.php` around lines 30 - 37, The test testShouldMatchKnownHashes is exercising PHP primitives instead of the SDK; replace the manual hash computation with a call to PublishEvent::hashUnit($input) inside the test (keep the same $input and $expectedHash variables), and assert that the returned value equals $expectedHash using self::assertSame; locate this change in the testShouldMatchKnownHashes method so it directly validates PublishEvent::hashUnit rather than using hash/base64_encode logic.src/ABsmartly.php (1)
84-108: Let the deprecated factory delegate tocreateSimple().The two factories are otherwise identical, so keeping both bodies in sync will get harder as this façade evolves.
♻️ Proposed change
public static function createWithDefaults( string $endpoint, string $apiKey, string $environment, string $application, int $retries = 5, int $timeout = 3000, ?callable $eventLogger = null ): ABsmartly { - - $clientConfig = new ClientConfig( - $endpoint, - $apiKey, - $application, - $environment, - ); - $clientConfig->setRetries($retries); - $clientConfig->setTimeout($timeout); - - $client = new Client($clientConfig, new HTTPClient()); - $sdkConfig = new Config($client); - if ($eventLogger !== null) { - $sdkConfig->setContextEventLogger(new \ABSmartly\SDK\Context\ContextEventLoggerCallback($eventLogger)); - } - return new ABsmartly($sdkConfig); + return self::createSimple( + $endpoint, + $apiKey, + $application, + $environment, + $retries, + $timeout, + $eventLogger, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ABsmartly.php` around lines 84 - 108, The createWithDefaults factory duplicates createSimple; change createWithDefaults to delegate to createSimple by calling ABsmartly::createSimple(...) with the same parameters (endpoint, apiKey, environment, application, retries, timeout, eventLogger) instead of duplicating client/config construction, so both factories share one implementation; locate createWithDefaults and replace its body with a call to createSimple while preserving the method signature and any deprecation annotation.tests/Client/ClientAsyncTest.php (1)
55-69: Assert the async path was actually used.Both async-branch tests pass if these methods return any
PromiseInterface, even whengetAsync()/putAsync()are never called or the response mapping is broken. Add mock expectations on the async methods and assert the resolvedContextData/ publish fulfilment.Also applies to: 94-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Client/ClientAsyncTest.php` around lines 55 - 69, The test currently only verifies that getContextDataAsync() returns a PromiseInterface but not that the async branch or response mapping runs; update testGetContextDataAsyncWithAsyncClient to set expectations on the mock from createMockAsyncHttpClient() that getAsync() is invoked (e.g. once with the expected endpoint) and returns the prepared response promise, then resolve the returned PromiseInterface and assert the resolved value is a ContextData instance with the expected fields; apply the same pattern to the publish test (ensure putAsync() is expected and that the resolved promise indicates successful publish/fulfilment).tests/Context/ContextTest.php (1)
1220-1245: This recovery test never verifies recovery.It only checks that
refresh()invoked the provider twice. A context can stay permanently failed after the second call and this test would still pass. Please assert thatisFailed()flips back tofalseand that a subsequent publish reaches the handler.Suggested assertion upgrade
$context->refresh(); self::assertSame(2, $callCount); + self::assertFalse($context->isFailed()); + + $context->track('goal_after_recovery'); + $context->publish(); + self::assertCount(1, $eventHandler->submitted); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/ContextTest.php` around lines 1220 - 1245, The test only verifies the provider was called twice but not that the context recovered; after calling Context::refresh() in testRecoveryFromFailedState you should assert the context is no longer failed (call Context::isFailed() returns false) and then exercise Context::publish(...) and verify ContextEventHandlerMock recorded/received the event (e.g. check the mock's published event list or invocation count) to prove the context actually recovered and can handle publishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 11-20: The Composer manifest currently lists react/http,
react/async, and react/promise as dev dependencies but multiple runtime classes
(e.g., ReactHttpClient which imports React\Http\Browser and React\Async\await,
public APIs like Client, AsyncClientInterface, ABsmartly, and your async context
classes that reference React\Promise\PromiseInterface) require them at runtime;
update composer.json by moving "react/http", "react/async", and "react/promise"
from require-dev into the "require" section (or alternatively fully isolate
async-only code behind a separate package or lazy-loaded wrapper so none of the
autoloaded runtime classes reference these symbols), ensuring consumers
installing without --dev will have these packages available.
In `@README.md`:
- Around line 379-387: Update the table row that currently reads "Close" /
`Context->close()` to use the correct event name `Finalize` to match the emitted
event type; specifically change the Event column label from `Close` to
`Finalize` and indicate that `Context::close()` emits
`ContextEventLoggerEvent::Finalize` (so integrators see the correct event name
to wire up).
- Around line 124-145: Update the async README examples to use the actual async
Context API: replace calls to the non-existent $context->ready() and avoid
chaining .then() on void methods; instead call $sdk->createContextAsync($config)
and attach a PromiseInterface then/catch to the returned promise (it resolves
with a ready Context), or use $sdk->createContextPending($config) which returns
['context' => $context, 'promise' => $promise] and attach .then()/.catch() to
that promise; keep calls to Context::isReady(), and do not chain .then() on
Context::publish() or Context::close() (they return void).
In `@src/ABsmartly.php`:
- Around line 119-125: createContextAsync currently calls
$this->createContext($contextConfig) directly inside resolve(), which runs
synchronously and lets exceptions escape instead of producing a rejected
Promise; change the sync-fallback to defer execution like createContextPending
does by wrapping the call in resolve(null)->then(fn() =>
$this->createContext($contextConfig)) (or equivalently resolve()->then(...)) so
any thrown exception becomes a rejected Promise and the method always returns a
proper PromiseInterface from createContextAsync.
In `@src/Client/Client.php`:
- Around line 82-85: The publish() method allows outbound PUTs without
validating the API key; update Client::publish to perform the same API-key
validation as authRequest()/getContextData() by either calling the existing
authRequest() guard or explicitly checking the stored API key before encoding
and calling $this->httpClient->put; if the API key is missing/invalid, throw or
abort the publish operation (same behavior as authRequest()) to prevent
blank-key requests from being sent.
- Around line 71-95: getContextDataAsync and publishAsync currently run
synchronous code (authRequest, encode, or publish) that can throw, causing sync
exceptions instead of rejected promises, and publish() lacks auth validation;
wrap the synchronous parts of getContextDataAsync (the authRequest call) and
publishAsync (authRequest + encode or the fallback publish call) in try/catch
and return a rejected PromiseInterface on exception, and add authRequest() at
the start of the synchronous publish(PublishEvent) method so both sync and async
publish paths validate auth; update references: getContextDataAsync,
publishAsync, publish, authRequest, encode, AsyncHttpClientInterface, and
PromiseInterface to implement these changes.
In `@src/Context/Context.php`:
- Around line 208-212: The successful-reload paths never clear the failed flag
so after a transient error the instance stays in failed mode; in the try blocks
that repopulate context (e.g. where $this->data = $contextData;
$this->setData($contextData); $this->ready = true; $this->logEvent(...)), reset
$this->failed = false when recovery succeeds so subsequent publish() and close()
take the normal path; apply the same fix to the other identical recovery block
referenced (around lines 751-753) so both successful reloads clear the failed
state.
- Around line 346-348: The branch using $this->cassignments can return a cached
natural Assignment without marking it as custom, so exposures are misreported;
when a custom assignment exists for $experimentName and matches
$assignment->variant, set $assignment->custom = true (and update any relevant
metadata like timestamp if present) before returning. Locate the logic around
$this->cassignments, experimentMatches(...), and audienceMatches(...) and ensure
that whenever a custom assignment is applied (even if the variant equals the
cached natural one) the returned Assignment object has its custom flag set to
true.
- Around line 297-299: The use of str_starts_with in the boolean branch will
break PHP 7.4; change the check in Context.php where the branch inspects $type
(the block using str_starts_with($type ?? '', 'boolean') && is_string($value'))
to a PHP 7.4-safe prefix test (e.g. use strpos(...) === 0 or substr_compare(...,
0) === 0) so the boolean coercion still runs on types that start with "boolean"
while remaining compatible with PHP 7.4; keep the rest of the branch logic (the
is_string($value) check and returning $value === 'true' || $value === '1')
unchanged.
In `@src/Experiment.php`:
- Around line 31-40: The Experiment class's typed property audienceStrict is
left uninitialized and can cause a PHP 8+ TypeError when Context accesses it;
initialize the audienceStrict property with a sensible default (e.g., false or
null matching its declared type) in the Experiment class declaration so it is
always set by default, and do not add audienceStrict to the $requiredFields
array used in the validation loop (leave $requiredFields as-is) so optional
behaviour is preserved.
In `@src/Http/HTTPClient.php`:
- Around line 73-116: The fetchResponse method can throw null when
$this->retries is 0 because the while loop never runs and $lastException stays
null; fix by validating or ensuring at least one attempt: either (A) at the
start of fetchResponse validate $this->retries and throw a clear exception
(e.g., InvalidArgumentException) when it's less than 1, or (B) change the retry
loop to guarantee one request (convert the while to a do/while or use max(1,
$this->retries)) so an actual HttpClientError is assigned to $lastException
before rethrowing; update references in fetchResponse, $this->retries,
$lastException and adjust the final throw to always throw an Exception instance.
In `@src/Http/ReactHttpClient.php`:
- Around line 15-16: ReactHttpClient exposes a public $retries property but
never uses it, causing async methods to lack retry behavior; update the async
request methods (e.g., getAsync, postAsync, requestAsync or sendAsync in
ReactHttpClient) to implement retry logic that uses $this->retries and respects
$this->timeout (loop attempts, catch network/timeout errors, backoff or
immediate retry as HTTPClient does), or if async retries are intentionally
unsupported remove the public $retries property and stop setting
httpClient->retries in the constructor; ensure whichever path you choose keeps
behavior consistent with HTTPClient and updates any constructor assignment that
references $retries.
In `@tests/JsonExpression/Operator/InOperatorTest.php`:
- Around line 29-36: In the test method testReturnFalseOnEmptyArray remove the
duplicate assertion that calls $this->operator->evaluate($this->evaluator,
[false, []]) (it's repeated twice); keep one instance and ensure the method
still asserts the distinct cases for false, "1", true and null against an empty
array; update only the duplicate assertion in the test (method name:
testReturnFalseOnEmptyArray, symbol: $this->operator->evaluate,
$this->evaluator) so the test no longer contains the duplicated line.
---
Nitpick comments:
In `@src/ABsmartly.php`:
- Around line 84-108: The createWithDefaults factory duplicates createSimple;
change createWithDefaults to delegate to createSimple by calling
ABsmartly::createSimple(...) with the same parameters (endpoint, apiKey,
environment, application, retries, timeout, eventLogger) instead of duplicating
client/config construction, so both factories share one implementation; locate
createWithDefaults and replace its body with a call to createSimple while
preserving the method signature and any deprecation annotation.
In `@src/Experiment.php`:
- Around line 36-38: Import the RuntimeException class and use the short name in
the throw to match other files: add a top-level use statement for
ABSmartly\SDK\Exception\RuntimeException and replace the fully-qualified
instantiation in the throw (the throw new
\ABSmartly\SDK\Exception\RuntimeException(sprintf(...))) with throw new
RuntimeException(sprintf(...))); locate this change around the throw in
Experiment.php to keep consistency with files like ContextConfig.php.
In `@src/Http/ReactHttpClient.php`:
- Line 79: The toResponse method signature lacks a parameter type; update the
private function toResponse($reactResponse): Response to include a type hint
(e.g. private function toResponse(\Psr\Http\Message\ResponseInterface
$reactResponse): Response or the concrete React response type) and adjust
imports or use the fully-qualified name; then update any callers if necessary to
satisfy the stricter type and ensure IDE/type-checking benefits.
In `@src/JsonExpression/Operator/MatchOperator.php`:
- Around line 27-36: The code in runRegexBounded uses trim($pattern, '/') which
removes all leading/trailing slashes; change it to strip only a single wrapping
delimiter pair: if (substr($pattern, 0, 1) === '/' && substr($pattern, -1) ===
'/') { $pattern = substr($pattern, 1, -1); } so patterns like //test// or paths
like /api/ are not mangled; update runRegexBounded to use this check before
calling preg_match and keep the rest of the error handling unchanged (and
confirm consistent behaviour with other SDKs if cross-SDK consistency was
intended).
In `@tests/Client/ClientAsyncTest.php`:
- Around line 55-69: The test currently only verifies that getContextDataAsync()
returns a PromiseInterface but not that the async branch or response mapping
runs; update testGetContextDataAsyncWithAsyncClient to set expectations on the
mock from createMockAsyncHttpClient() that getAsync() is invoked (e.g. once with
the expected endpoint) and returns the prepared response promise, then resolve
the returned PromiseInterface and assert the resolved value is a ContextData
instance with the expected fields; apply the same pattern to the publish test
(ensure putAsync() is expected and that the resolved promise indicates
successful publish/fulfilment).
In `@tests/Context/AsyncContextDataProviderTest.php`:
- Around line 35-52: The test testGetContextDataAsyncResolvesToContextData
should use React\Async\await() instead of manually attaching a then callback to
the promise to make the assertion robust; update the test so the mock client
still returns resolve($contextData) and then call
await($provider->getContextDataAsync()) to obtain $result synchronously before
asserting assertSame($contextData, $result), referencing
AsyncContextDataProvider::getContextDataAsync and the resolve() usage when
making the change.
In `@tests/Context/ContextTest.php`:
- Around line 1220-1245: The test only verifies the provider was called twice
but not that the context recovered; after calling Context::refresh() in
testRecoveryFromFailedState you should assert the context is no longer failed
(call Context::isFailed() returns false) and then exercise Context::publish(...)
and verify ContextEventHandlerMock recorded/received the event (e.g. check the
mock's published event list or invocation count) to prove the context actually
recovered and can handle publishes.
In `@tests/JsonExpression/Operator/InOperatorTest.php`:
- Around line 64-67: Rename the test method testArrayDiffNull to a clearer name
that reflects its behavior (e.g., testArrayNotFoundAsElement or
testNestedArrayNotContained) and update any references to it; specifically
change the method name in the test class containing the method testArrayDiffNull
and ensure any test runner or annotations still recognize it (the body calling
$this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]]) etc. should
remain unchanged).
In `@tests/MD5Test.php`:
- Around line 30-37: The test testShouldMatchKnownHashes is exercising PHP
primitives instead of the SDK; replace the manual hash computation with a call
to PublishEvent::hashUnit($input) inside the test (keep the same $input and
$expectedHash variables), and assert that the returned value equals
$expectedHash using self::assertSame; locate this change in the
testShouldMatchKnownHashes method so it directly validates
PublishEvent::hashUnit rather than using hash/base64_encode logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 991f6102-50ca-4a47-b7d7-befe3f8de1c3
📒 Files selected for processing (41)
.gitignoreREADME.mdcomposer.jsonsrc/ABsmartly.phpsrc/Assignment.phpsrc/Client/AsyncClientInterface.phpsrc/Client/Client.phpsrc/Client/ClientConfig.phpsrc/Client/ClientInterface.phpsrc/Context/AsyncContextDataProvider.phpsrc/Context/AsyncContextEventHandler.phpsrc/Context/Context.phpsrc/Context/ContextConfig.phpsrc/Context/ContextDataProvider.phpsrc/Context/ContextEventHandler.phpsrc/Context/ContextEventLoggerEvent.phpsrc/Exception/RuntimeException.phpsrc/Experiment.phpsrc/Http/AsyncHttpClientInterface.phpsrc/Http/HTTPClient.phpsrc/Http/HttpClientInterface.phpsrc/Http/ReactHttpClient.phpsrc/JsonExpression/Operator/InOperator.phpsrc/JsonExpression/Operator/MatchOperator.phpsrc/SDK.phpsrc/VariableParser.phptests/ABsmartlyTest.phptests/AudienceMatcherTest.phptests/Client/ClientAsyncTest.phptests/Client/ClientConfigTest.phptests/Client/ClientInterfaceTest.phptests/Context/AsyncContextDataProviderTest.phptests/Context/AsyncContextEventHandlerTest.phptests/Context/ContextTest.phptests/Fixtures/json/context_custom_fields.jsontests/Fixtures/json/refreshed_iteration.jsontests/Http/HttpClientInterfaceTest.phptests/JsonExpression/Operator/InOperatorTest.phptests/MD5Test.phptests/Murmur3Test.phptests/VariantAssignerTest.php
Comment on lines
+11
to
20
| "phpunit/phpunit": "^9.5.26", | ||
| "react/http": "^1.9", | ||
| "react/async": "^4.0", | ||
| "react/promise": "^3.0" | ||
| }, | ||
| "suggest": { | ||
| "react/http": "Required for async HTTP support with ReactPHP", | ||
| "react/async": "Required for sync-over-async operations with ReactPHP", | ||
| "react/promise": "Required for async operations" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether ReactPHP symbols are used by runtime source code.
# Expected:
# - If matches exist in src/, these are runtime dependencies and should not be dev-only.
rg -n --type=php -C2 '\bReact\\' src
rg -n --type=php -C2 '\bPromiseInterface\b|\bDeferred\b|\bawait\s*\(' src
python - <<'PY'
import json
with open("composer.json") as f:
c = json.load(f)
print("require keys:", sorted(c.get("require", {}).keys()))
print("require-dev keys:", sorted(c.get("require-dev", {}).keys()))
print("suggest keys:", sorted(c.get("suggest", {}).keys()))
PYRepository: absmartly/php-sdk
Length of output: 9434
ReactPHP packages must be moved from require-dev to require.
Multiple runtime classes in src/ have hard dependencies on ReactPHP packages: ReactHttpClient imports React\Http\Browser and React\Async\await; Client, AsyncClientInterface, ABsmartly, and async context classes all use React\Promise\PromiseInterface in their public method signatures. Consumers installing this library without the --dev flag will encounter fatal class/interface-not-found errors when instantiating async components. Move react/http, react/async, and react/promise to the require section, or fully hard-isolate async code so autoloaded runtime classes do not depend on these packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` around lines 11 - 20, The Composer manifest currently lists
react/http, react/async, and react/promise as dev dependencies but multiple
runtime classes (e.g., ReactHttpClient which imports React\Http\Browser and
React\Async\await, public APIs like Client, AsyncClientInterface, ABsmartly, and
your async context classes that reference React\Promise\PromiseInterface)
require them at runtime; update composer.json by moving "react/http",
"react/async", and "react/promise" from require-dev into the "require" section
(or alternatively fully isolate async-only code behind a separate package or
lazy-loaded wrapper so none of the autoloaded runtime classes reference these
symbols), ensuring consumers installing without --dev will have these packages
available.
Comment on lines
+379
to
+387
| | Event | When | Data | | ||
| | ---------- | ----------------------------------------------------------- | ----------------------------------------------------------- | | ||
| | `Error` | `Context` receives an error | `Exception` object thrown | | ||
| | `Ready` | `Context` turns ready | `ContextData` object used to initialize the context | | ||
| | `Refresh` | `Context->refresh()` method succeeds | `ContextData` used to refresh the context | | ||
| | `Publish` | `Context->publish()` method succeeds | `PublishEvent` data sent to the A/B Smartly event collector | | ||
| | `Exposure` | `Context->getTreatment()` method succeeds on first exposure | `Exposure` data enqueued for publishing | | ||
| | `Goal` | `Context->track()` method succeeds | `GoalAchievement` goal data enqueued for publishing | | ||
| | `Close` | `Context->close()` method succeeds the first time | `null` | |
There was a problem hiding this comment.
Use Finalize here, not Close.
Context::close() now emits ContextEventLoggerEvent::Finalize, so this table is advertising the wrong event name for integrators wiring loggers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 379 - 387, Update the table row that currently reads
"Close" / `Context->close()` to use the correct event name `Finalize` to match
the emitted event type; specifically change the Event column label from `Close`
to `Finalize` and indicate that `Context::close()` emits
`ContextEventLoggerEvent::Finalize` (so integrators see the correct event name
to wire up).
Comment on lines
+119
to
+125
| public function createContextAsync(ContextConfig $contextConfig): PromiseInterface { | ||
| if (!$this->provider instanceof AsyncContextDataProvider) { | ||
| return resolve($this->createContext($contextConfig)); | ||
| } | ||
|
|
||
| return $this->provider->getContextDataAsync() | ||
| ->then(fn($data) => $this->createContextWithData($contextConfig, $data)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP, are function arguments evaluated before React\Promise\resolve()is called, and if the argument expression throws, does the exception escape before aPromiseInterface is returned?
💡 Result:
Yes.
- In PHP, the expressions in a function’s argument list are evaluated (left-to-right) before the function is actually called (“eager evaluation”). So the argument to
React\Promise\resolve(...)is computed first. If that argument expression throws, the call toresolve()never happens. [1] - Therefore, if the argument expression throws, the exception propagates immediately, and no
PromiseInterfaceis returned, becauseReact\Promise\resolve(mixed $promiseOrValue)is only entered after its argument has been successfully evaluated. [1][2]
Sources:
[1] PHP manual: Function parameters and arguments (argument expressions evaluated before the function call) (php.net)
[2] ReactPHP Promise docs: React\Promise\resolve(mixed $promiseOrValue) creates/returns a promise for the supplied value (reactphp.org)
Citations:
- 1: https://www.php.net/manual/en/functions.arguments.php?utm_source=openai
- 2: https://reactphp.org/promise/?utm_source=openai
🏁 Script executed:
cat -n src/ABsmartly.php | sed -n '110,135p'Repository: absmartly/php-sdk
Length of output: 1380
🏁 Script executed:
cat -n src/ABsmartly.php | sed -n '128,145p'Repository: absmartly/php-sdk
Length of output: 831
Return a rejected promise on sync-fallback failures.
$this->createContext($contextConfig) on line 121 executes before resolve() is called. If context creation fails, the exception escapes synchronously instead of being wrapped in a rejected promise. This breaks the PromiseInterface contract for callers. The safer pattern already exists in createContextPending() at line 132, which uses resolve(null)->then(...) to defer execution.
🛠️ Proposed fix
public function createContextAsync(ContextConfig $contextConfig): PromiseInterface {
if (!$this->provider instanceof AsyncContextDataProvider) {
- return resolve($this->createContext($contextConfig));
+ return resolve(null)->then(fn() => $this->createContext($contextConfig));
}
return $this->provider->getContextDataAsync()
->then(fn($data) => $this->createContextWithData($contextConfig, $data));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ABsmartly.php` around lines 119 - 125, createContextAsync currently calls
$this->createContext($contextConfig) directly inside resolve(), which runs
synchronously and lets exceptions escape instead of producing a rejected
Promise; change the sync-fallback to defer execution like createContextPending
does by wrapping the call in resolve(null)->then(fn() =>
$this->createContext($contextConfig)) (or equivalently resolve()->then(...)) so
any thrown exception becomes a rejected Promise and the method always returns a
proper PromiseInterface from createContextAsync.
Comment on lines
+71
to
+95
| public function getContextDataAsync(): PromiseInterface { | ||
| if (!$this->httpClient instanceof AsyncHttpClientInterface) { | ||
| return resolve($this->getContextData()); | ||
| } | ||
|
|
||
| $this->authRequest(); | ||
| return $this->httpClient | ||
| ->getAsync($this->url, $this->query, $this->headers) | ||
| ->then(fn($response) => new ContextData($this->decode($response->content)->experiments)); | ||
| } | ||
|
|
||
| public function publish(PublishEvent $publishEvent): void { | ||
| $data = $this->encode($publishEvent); | ||
| $this->httpClient->put($this->url, $this->query, $this->headers, $data); | ||
| } | ||
|
|
||
| public function publishAsync(PublishEvent $publishEvent): PromiseInterface { | ||
| if (!$this->httpClient instanceof AsyncHttpClientInterface) { | ||
| $this->publish($publishEvent); | ||
| return resolve(null); | ||
| } | ||
|
|
||
| $data = $this->encode($publishEvent); | ||
| return $this->httpClient->putAsync($this->url, $this->query, $this->headers, $data); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ReactPHP's Promise library, if PHP code throws before React\Promise\resolve(...)returns, does the caller receive a rejectedPromiseInterface or a synchronous exception?
💡 Result:
You get a synchronous exception, not a rejected PromiseInterface.
React\Promise\resolve($promiseOrValue) does not wrap the caller’s code in a try/catch; it just inspects the value and returns either the original promise/thenable-wrapping promise or a fulfilled promise. So if something throws before resolve(...) can return (e.g. while computing the argument expression), there is no promise to reject—the exception bubbles normally. [1]
Only exceptions thrown inside a promise resolver/canceller (i.e., inside new React\Promise\Promise(function ($resolve, $reject) { ... })) are caught and turned into a rejected promise. [2]
Sources: [1] React\Promise\resolve() implementation, [2] ReactPHP Promise docs on resolver/canceller exceptions.
🏁 Script executed:
cd src/Client && head -100 Client.php | cat -nRepository: absmartly/php-sdk
Length of output: 3848
🏁 Script executed:
rg "authRequest" --type php -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 852
🏁 Script executed:
rg "publish|getContextData" --type php -A 3 | head -80Repository: absmartly/php-sdk
Length of output: 4776
🏁 Script executed:
rg "publish\(\)" tests/ --type php -B 3 -A 3 | head -100Repository: absmartly/php-sdk
Length of output: 5623
🏁 Script executed:
cat -n tests/Client/ClientAsyncTest.phpRepository: absmartly/php-sdk
Length of output: 4835
🏁 Script executed:
rg "authRequest|X-API-Key|API key|RuntimeException" tests/ --type php -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 5421
🏁 Script executed:
rg "publish" src/ --type php -B 3 -A 3 | grep -A 5 -B 5 "authRequest"Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
rg "getContextData|publish" src/Client/Client.php -B 2 -A 8Repository: absmartly/php-sdk
Length of output: 1386
🏁 Script executed:
rg "API key|getApiKey" tests/ --type php -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 580
🏁 Script executed:
rg "publish.*auth|auth.*publish" src/ tests/ --type php -i -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
cat -n src/Client/AsyncClientInterface.phpRepository: absmartly/php-sdk
Length of output: 440
🏁 Script executed:
grep -r "getApiKey\|API key" src/Client/ --include="*.php" -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 1310
Wrap synchronous operations in try/catch to prevent promise contract violations.
Both async methods execute synchronous code before returning a PromiseInterface: getContextDataAsync() calls authRequest() directly, and publishAsync() calls encode() and potentially publish(). If these operations throw, the caller receives a synchronous exception rather than a rejected promise, breaking the contract for code expecting promise-based error handling.
Additionally, publish() lacks the authRequest() validation that getContextData() performs. The proposed fix addresses the promise wrapping but only adds authRequest() to publishAsync(), leaving the synchronous publish() method without API key validation—creating an inconsistency across the client interface.
Ensure both sync and async publish methods validate authentication, and wrap all sync operations in try/catch blocks that return rejected promises on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Client/Client.php` around lines 71 - 95, getContextDataAsync and
publishAsync currently run synchronous code (authRequest, encode, or publish)
that can throw, causing sync exceptions instead of rejected promises, and
publish() lacks auth validation; wrap the synchronous parts of
getContextDataAsync (the authRequest call) and publishAsync (authRequest +
encode or the fallback publish call) in try/catch and return a rejected
PromiseInterface on exception, and add authRequest() at the start of the
synchronous publish(PublishEvent) method so both sync and async publish paths
validate auth; update references: getContextDataAsync, publishAsync, publish,
authRequest, encode, AsyncHttpClientInterface, and PromiseInterface to implement
these changes.
Comment on lines
346
to
348
| } 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 |
There was a problem hiding this comment.
Custom assignments can reuse a stale natural assignment.
If a custom assignment is set to the same variant that the cached natural bucketing already produced, this branch returns the old cached Assignment unchanged. That leaves $assignment->custom false, so subsequent exposures are published as natural traffic instead of custom traffic.
Proposed fix
- } else if (!isset($this->cassignments[$experimentName]) || $this->cassignments[$experimentName] === $assignment->variant) {
+ } else if (
+ !array_key_exists($experimentName, $this->cassignments) ||
+ ($assignment->custom && $this->cassignments[$experimentName] === $assignment->variant)
+ ) {
if ($this->experimentMatches($experiment->data, $assignment) && $this->audienceMatches($experiment->data, $assignment)) {
// assignment up-to-date
return $assignment;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Context/Context.php` around lines 346 - 348, The branch using
$this->cassignments can return a cached natural Assignment without marking it as
custom, so exposures are misreported; when a custom assignment exists for
$experimentName and matches $assignment->variant, set $assignment->custom = true
(and update any relevant metadata like timestamp if present) before returning.
Locate the logic around $this->cassignments, experimentMatches(...), and
audienceMatches(...) and ensure that whenever a custom assignment is applied
(even if the variant equals the cached natural one) the returned Assignment
object has its custom flag set to true.
Comment on lines
+15
to
+16
| public int $retries = 5; | ||
| public int $timeout = 3000; |
There was a problem hiding this comment.
$retries property is declared but never used.
The $retries property is exposed publicly and can be configured via Client::__construct (which sets $this->httpClient->retries), but the async methods don't implement any retry logic. This creates inconsistent behaviour between HTTPClient (which retries) and ReactHttpClient (which doesn't).
Consider either:
- Implementing retry logic in the async methods to match
HTTPClientbehaviour, or - Removing the property if retries are intentionally not supported for async operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/ReactHttpClient.php` around lines 15 - 16, ReactHttpClient exposes a
public $retries property but never uses it, causing async methods to lack retry
behavior; update the async request methods (e.g., getAsync, postAsync,
requestAsync or sendAsync in ReactHttpClient) to implement retry logic that uses
$this->retries and respects $this->timeout (loop attempts, catch network/timeout
errors, backoff or immediate retry as HTTPClient does), or if async retries are
intentionally unsupported remove the public $retries property and stop setting
httpClient->retries in the constructor; ensure whichever path you choose keeps
behavior consistent with HTTPClient and updates any constructor assignment that
references $retries.
Comment on lines
29
to
36
| public function testReturnFalseOnEmptyArray(): void { | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[], false])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[], "1"])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[], true])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[], false])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [false, []])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, ["1", []])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [true, []])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [false, []])); | ||
|
|
||
| self::assertNull($this->operator->evaluate($this->evaluator, [[], null])); | ||
| self::assertNull($this->operator->evaluate($this->evaluator, [null, []])); | ||
| } |
There was a problem hiding this comment.
Duplicate test case detected.
Lines 30 and 33 are identical - both evaluate [false, []]. This appears to be a copy-paste oversight.
🧹 Proposed fix to remove duplicate
public function testReturnFalseOnEmptyArray(): void {
self::assertFalse($this->operator->evaluate($this->evaluator, [false, []]));
self::assertFalse($this->operator->evaluate($this->evaluator, ["1", []]));
self::assertTrue($this->operator->evaluate($this->evaluator, [true, []]));
- self::assertFalse($this->operator->evaluate($this->evaluator, [false, []]));
self::assertNull($this->operator->evaluate($this->evaluator, [null, []]));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/JsonExpression/Operator/InOperatorTest.php` around lines 29 - 36, In
the test method testReturnFalseOnEmptyArray remove the duplicate assertion that
calls $this->operator->evaluate($this->evaluator, [false, []]) (it's repeated
twice); keep one instance and ensure the method still asserts the distinct cases
for false, "1", true and null against an empty array; update only the duplicate
assertion in the test (method name: testReturnFalseOnEmptyArray, symbol:
$this->operator->evaluate, $this->evaluator) so the test no longer contains the
duplicated line.
…Error, and isFinalizing to Context
… messages Adds isFinalized() and finalize() as aliases for isClosed() and close() to align with JS SDK terminology. Standardizes error messages to include trailing periods matching JS SDK format.
…eady or finalized
Use lowercase 'ABsmartly' prefix and standard unit error formats with single quotes and trailing periods.
…ct wording - Match expected format: Unit "x" UID must not be blank (no trailing period) - Match expected format: Unit "x" UID is already set (was "already set.")
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/Client/Client.php (3)
82-85:⚠️ Potential issue | 🟠 MajorMissing authentication validation in
publish().Unlike
getContextData(), thepublish()method does not callauthRequest(), allowing requests with a blank API key to be sent. Contexts built from pre-fetched data can reachpublish()without ever validating credentials.Proposed fix
public function publish(PublishEvent $publishEvent): void { + $this->authRequest(); $data = $this->encode($publishEvent); $this->httpClient->put($this->url, $this->query, $this->headers, $data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 82 - 85, The publish() method in Client (which currently encodes PublishEvent and calls $this->httpClient->put($this->url, $this->query, $this->headers, $data)) lacks authentication validation; add a call to $this->authRequest() at the start of publish() (same pattern used in getContextData()) so that credentials are validated and requests with a blank API key are prevented before calling httpClient->put; ensure the method aborts or throws if authRequest() indicates invalid credentials so no publish is sent with missing auth.
71-80:⚠️ Potential issue | 🟠 MajorSynchronous exceptions break promise contract.
When
getContextDataAsync()falls back to sync mode (line 73), ifgetContextData()throws, the caller receives a synchronous exception rather than a rejected promise. Similarly,authRequest()on line 76 can throw before the promise chain starts.Proposed fix: wrap in try/catch
public function getContextDataAsync(): PromiseInterface { + try { if (!$this->httpClient instanceof AsyncHttpClientInterface) { return resolve($this->getContextData()); } $this->authRequest(); return $this->httpClient ->getAsync($this->url, $this->query, $this->headers) ->then(fn($response) => new ContextData($this->decode($response->content)->experiments)); + } catch (\Throwable $e) { + return \React\Promise\reject($e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 71 - 80, getContextDataAsync currently throws synchronously when getContextData() or authRequest() raises, breaking the PromiseInterface contract; wrap the synchronous path and the authRequest() call in a try/catch and return a rejected promise on exception instead of letting it bubble. Specifically, in getContextDataAsync surround the resolve($this->getContextData()) and the call to $this->authRequest() with try/catch and on catch return a rejected PromiseInterface (using the same promise helper family as resolve/reject used elsewhere) so callers always receive a promise that rejects on error; keep the existing httpClient->getAsync(...)->then(...) chain unchanged.
87-95:⚠️ Potential issue | 🟠 MajorSame promise contract issue in
publishAsync().Synchronous operations (
encode(),publish()) can throw before returning a promise. Wrap in try/catch for consistent error handling.Proposed fix
public function publishAsync(PublishEvent $publishEvent): PromiseInterface { + try { + $this->authRequest(); if (!$this->httpClient instanceof AsyncHttpClientInterface) { $this->publish($publishEvent); return resolve(null); } $data = $this->encode($publishEvent); return $this->httpClient->putAsync($this->url, $this->query, $this->headers, $data); + } catch (\Throwable $e) { + return \React\Promise\reject($e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 87 - 95, The publishAsync method can throw synchronously from encode() or publish(), violating the promise contract; wrap the whole body in a try/catch and return a rejected promise on error. Concretely, in publishAsync(PublishEvent $publishEvent): PromiseInterface wrap the logic (the instanceof AsyncHttpClientInterface check, the $this->publish($publishEvent) call, and the $data = $this->encode($publishEvent) + $this->httpClient->putAsync(...) call) in a try block and catch \Throwable $e to return reject($e) so callers always receive a PromiseInterface (use resolve(null) for the synchronous-success path and reject($e) on exceptions).src/Http/HTTPClient.php (1)
72-115:⚠️ Potential issue | 🟠 MajorPotential null throw when
retriesis 0.If
$this->retriesis0, thewhileloop never executes,$lastExceptionremainsnull, and line 114 throwsnull, causing aTypeErrorin PHP 8+. This is inconsistent withClientConfig::setRetries(0)being explicitly allowed (tested intestSetRetriesZeroIsAllowed).Proposed fix: ensure at least one attempt
private function fetchResponse(): Response { $attempt = 0; $lastException = null; + $maxAttempts = max(1, $this->retries); - while ($attempt < $this->retries) { + while ($attempt < $maxAttempts) { try { // ... existing code ... } catch (HttpClientError $e) { $lastException = $e; // ... - if (!$isRetryable || $attempt >= $this->retries - 1) { + if (!$isRetryable || $attempt >= $maxAttempts - 1) { throw $e; } // ... } } throw $lastException; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/HTTPClient.php` around lines 72 - 115, fetchResponse can throw null when $this->retries is 0 because the while loop is skipped and $lastException remains null; change the loop to guarantee at least one attempt by computing $maxAttempts = max(1, $this->retries) and use while ($attempt < $maxAttempts) (or convert to a do/while) so fetchResponse always performs an HTTP attempt and $lastException will be set before throwing; update references to $this->retries in the backoff logging to still show the configured value if desired.
🧹 Nitpick comments (6)
src/Experiment.php (1)
34-40: Consider importing RuntimeException.The exception class is used with its fully qualified name. For consistency with the rest of the codebase, consider adding an import statement.
Suggested change
Add to imports at the top of the file:
use ABSmartly\SDK\Exception\RuntimeException;Then update the throw statement:
- throw new \ABSmartly\SDK\Exception\RuntimeException( + throw new RuntimeException(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Experiment.php` around lines 34 - 40, The throw uses the fully qualified \ABSmartly\SDK\Exception\RuntimeException; add a use statement for ABSmartly\SDK\Exception\RuntimeException at the top of the file and update the throw in the Experiment class (the foreach checking $requiredFields and the throw site) to throw RuntimeException instead of the FQN to match project import style.tests/Http/ReactHttpClientTest.php (1)
9-37: Consider expanding test coverage for ReactHttpClient.The current tests only cover the
flattenHeadershelper method. Consider adding tests for the core async HTTP methods (getAsync,putAsync) and error handling scenarios, even if they require mocking the ReactPHP Browser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Http/ReactHttpClientTest.php` around lines 9 - 37, Tests only cover flattenHeaders; add unit tests for ReactHttpClient's async methods and error paths: create tests for getAsync and putAsync on ReactHttpClient that mock the ReactPHP Browser and Response/RequestPromise to simulate successful responses (verify status, body handling and headers via flattenHeaders) and failing requests (simulate promise rejection to assert exception handling or error propagation). Use mocks/doubles for BrowserInterface/Browser, ResponseInterface and the promise returned by Browser->get()/put(), and reference the ReactHttpClient::getAsync, ReactHttpClient::putAsync, and flattenHeaders methods to locate where to call and assert behaviors; include cases for non-2xx responses, JSON bodies and empty bodies.src/Client/Client.php (1)
32-37: Property existence checks are fragile.Using
property_exists()to settimeoutandretriesrelies on implementation details of the HTTP client. Consider defining these inHttpClientInterfaceor using a configuration object pattern instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 32 - 37, The current use of property_exists($this->httpClient, 'timeout') / 'retries' is fragile; change the design so the HTTP client exposes explicit methods (e.g. add setTimeout(int $seconds) and setRetries(int $count) to HttpClientInterface) and update Client (where $this->httpClient is used) to call $this->httpClient->setTimeout($clientConfig->getTimeout()) and ->setRetries($clientConfig->getRetries()) instead of manipulating properties; implement those methods on all concrete HTTP client classes (or alternatively accept a configuration object via a single setConfig(Config $cfg) method on the interface) and remove the property_exists checks from Client.php.tests/Context/ContextTest.php (1)
814-814: Consider usingassertEmptyorassertArrayNotHasKeyinstead ofproperty_existsfor clearer intent.Using
assertFalse(property_exists($event, 'exposures'), ...)works but PHPUnit's built-in assertions provide clearer failure messages. However, if the object structure genuinely uses dynamic properties, this approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/ContextTest.php` at line 814, Replace the assertion that currently uses property_exists($event, 'exposures') with a clearer PHPUnit assertion: if $event is expected to be an array or array-like, use assertArrayNotHasKey('exposures', (array)$event, ...) to assert the key is absent; if $event is an object that may have a public exposures property, use assertEmpty($event->exposures ?? null, ...) to assert there are no exposures. Locate the line containing property_exists($event, 'exposures') and swap it for the appropriate assertion based on whether $event is an array or object.src/ABsmartly.php (1)
71-112: Deprecated methodcreateWithDefaultsduplicates logic fromcreateSimple.Both factory methods contain nearly identical code. Consider having
createWithDefaultsdelegate tocreateSimplewith swapped parameters to reduce duplication and maintenance burden.Suggested refactor
public static function createWithDefaults( string $endpoint, string $apiKey, string $environment, string $application, int $retries = 5, int $timeout = 3000, ?callable $eventLogger = null ): ABsmartly { - $clientConfig = new ClientConfig( - $endpoint, - $apiKey, - $application, - $environment, - ); - $clientConfig->setRetries($retries); - $clientConfig->setTimeout($timeout); - - $client = new Client($clientConfig, new HTTPClient()); - $sdkConfig = new Config($client); - if ($eventLogger !== null) { - $sdkConfig->setContextEventLogger(new \ABSmartly\SDK\Context\ContextEventLoggerCallback($eventLogger)); - } - return new ABsmartly($sdkConfig); + return self::createSimple($endpoint, $apiKey, $application, $environment, $retries, $timeout, $eventLogger); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ABsmartly.php` around lines 71 - 112, The createWithDefaults method duplicates createSimple and uses the wrong parameter order for ClientConfig; refactor createWithDefaults to delegate to createSimple: call createSimple with parameters reordered to match createSimple's signature (endpoint, apiKey, application, environment, retries, timeout, eventLogger) so you keep the same defaults and behavior, including preserving setRetries/setTimeout and the ContextEventLoggerCallback handling inside createSimple (or ensure createSimple already performs those steps and simply return its result from createWithDefaults); update createWithDefaults to return the value from createSimple and remove the duplicated ClientConfig/Client/Config construction.tests/JsonExpression/Operator/MatchOperatorTest.php (1)
33-37: Nice coverage for literal~; add one modifier case to lock behaviour down.This test validates the tilde delimiter path well. I’d also add a
/.../iassertion so delimiter normalisation keeps parity for case-insensitive matches.Suggested test addition
public function testRegexWithTildeDelimiterInPattern(): void { self::assertTrue($this->operator->evaluate($this->evaluator, ["hello~world", "hello~world"])); self::assertTrue($this->operator->evaluate($this->evaluator, ["test~value", "~"])); self::assertFalse($this->operator->evaluate($this->evaluator, ["helloworld", "hello~world"])); + self::assertTrue($this->operator->evaluate($this->evaluator, ["ABC", "/abc/i"])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/JsonExpression/Operator/MatchOperatorTest.php` around lines 33 - 37, Add a case-insensitive modifier assertion to the testRegexWithTildeDelimiterInPattern test: use the existing test method (testRegexWithTildeDelimiterInPattern) and add an assertion calling $this->operator->evaluate($this->evaluator, ...) that verifies a pattern using tilde delimiters with the "i" modifier performs a case-insensitive match (e.g., different-cased subject should still return true); keep the same call shape as the other assertions so it exercises delimiter normalisation with modifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Context/Context.php`:
- Around line 115-118: Replace PHP 8-only string helpers with PHP 7.4-safe
equivalents: in src/Context/Context.php (the block using str_ends_with($k,
'_type')) swap the call to a substr_compare/substr-based check (or compare the
tail via substr and ===) so it behaves identically; in
src/JsonExpression/Operator/InOperator.php replace str_contains($haystack,
$needle) with a strpos($haystack, $needle) !== false check (or an equivalent
strict !== false test) to preserve semantics on PHP 7.4. Ensure both
replacements preserve exact boolean logic and handle edge cases (empty needle)
the same as the originals.
In `@src/JsonExpression/Operator/MatchOperator.php`:
- Around line 28-31: The current code trims leading/trailing slashes from
$pattern and then escapes tildes but fails to handle regex modifiers (e.g.
/abc/i) — detect when $pattern is slash-delimited with trailing modifiers, split
into the pattern body and modifier string (keep the modifiers separate), apply
escaping only to the body (e.g. escape '~' in the body), and then call
preg_match using the delimiter and append the modifiers outside the closing
delimiter (i.e. preg_match('~' . $body . '~' . $modifiers, $text)). Update the
logic around $pattern and the preg_match call in MatchOperator.php so that
slash-delimited modifiers are preserved and passed to preg_match.
In `@tests/Context/ContextTest.php`:
- Around line 1545-1564: The test method testAttributeUpdatesInTemplates assigns
$treatmentBefore and $treatmentAfter in
ContextTest::testAttributeUpdatesInTemplates but never asserts their values;
either remove these unused variables or add explicit assertions comparing them
(e.g., assertNotEquals or assertEquals depending on intended behavior) to verify
that getTreatment('exp_test_ab') changed (or stayed the same) after
setAttribute('age', 25) and publish(); update the test to assert the expected
relationship between $treatmentBefore and $treatmentAfter and keep the existing
event assertions.
---
Duplicate comments:
In `@src/Client/Client.php`:
- Around line 82-85: The publish() method in Client (which currently encodes
PublishEvent and calls $this->httpClient->put($this->url, $this->query,
$this->headers, $data)) lacks authentication validation; add a call to
$this->authRequest() at the start of publish() (same pattern used in
getContextData()) so that credentials are validated and requests with a blank
API key are prevented before calling httpClient->put; ensure the method aborts
or throws if authRequest() indicates invalid credentials so no publish is sent
with missing auth.
- Around line 71-80: getContextDataAsync currently throws synchronously when
getContextData() or authRequest() raises, breaking the PromiseInterface
contract; wrap the synchronous path and the authRequest() call in a try/catch
and return a rejected promise on exception instead of letting it bubble.
Specifically, in getContextDataAsync surround the
resolve($this->getContextData()) and the call to $this->authRequest() with
try/catch and on catch return a rejected PromiseInterface (using the same
promise helper family as resolve/reject used elsewhere) so callers always
receive a promise that rejects on error; keep the existing
httpClient->getAsync(...)->then(...) chain unchanged.
- Around line 87-95: The publishAsync method can throw synchronously from
encode() or publish(), violating the promise contract; wrap the whole body in a
try/catch and return a rejected promise on error. Concretely, in
publishAsync(PublishEvent $publishEvent): PromiseInterface wrap the logic (the
instanceof AsyncHttpClientInterface check, the $this->publish($publishEvent)
call, and the $data = $this->encode($publishEvent) +
$this->httpClient->putAsync(...) call) in a try block and catch \Throwable $e to
return reject($e) so callers always receive a PromiseInterface (use
resolve(null) for the synchronous-success path and reject($e) on exceptions).
In `@src/Http/HTTPClient.php`:
- Around line 72-115: fetchResponse can throw null when $this->retries is 0
because the while loop is skipped and $lastException remains null; change the
loop to guarantee at least one attempt by computing $maxAttempts = max(1,
$this->retries) and use while ($attempt < $maxAttempts) (or convert to a
do/while) so fetchResponse always performs an HTTP attempt and $lastException
will be set before throwing; update references to $this->retries in the backoff
logging to still show the configured value if desired.
---
Nitpick comments:
In `@src/ABsmartly.php`:
- Around line 71-112: The createWithDefaults method duplicates createSimple and
uses the wrong parameter order for ClientConfig; refactor createWithDefaults to
delegate to createSimple: call createSimple with parameters reordered to match
createSimple's signature (endpoint, apiKey, application, environment, retries,
timeout, eventLogger) so you keep the same defaults and behavior, including
preserving setRetries/setTimeout and the ContextEventLoggerCallback handling
inside createSimple (or ensure createSimple already performs those steps and
simply return its result from createWithDefaults); update createWithDefaults to
return the value from createSimple and remove the duplicated
ClientConfig/Client/Config construction.
In `@src/Client/Client.php`:
- Around line 32-37: The current use of property_exists($this->httpClient,
'timeout') / 'retries' is fragile; change the design so the HTTP client exposes
explicit methods (e.g. add setTimeout(int $seconds) and setRetries(int $count)
to HttpClientInterface) and update Client (where $this->httpClient is used) to
call $this->httpClient->setTimeout($clientConfig->getTimeout()) and
->setRetries($clientConfig->getRetries()) instead of manipulating properties;
implement those methods on all concrete HTTP client classes (or alternatively
accept a configuration object via a single setConfig(Config $cfg) method on the
interface) and remove the property_exists checks from Client.php.
In `@src/Experiment.php`:
- Around line 34-40: The throw uses the fully qualified
\ABSmartly\SDK\Exception\RuntimeException; add a use statement for
ABSmartly\SDK\Exception\RuntimeException at the top of the file and update the
throw in the Experiment class (the foreach checking $requiredFields and the
throw site) to throw RuntimeException instead of the FQN to match project import
style.
In `@tests/Context/ContextTest.php`:
- Line 814: Replace the assertion that currently uses property_exists($event,
'exposures') with a clearer PHPUnit assertion: if $event is expected to be an
array or array-like, use assertArrayNotHasKey('exposures', (array)$event, ...)
to assert the key is absent; if $event is an object that may have a public
exposures property, use assertEmpty($event->exposures ?? null, ...) to assert
there are no exposures. Locate the line containing property_exists($event,
'exposures') and swap it for the appropriate assertion based on whether $event
is an array or object.
In `@tests/Http/ReactHttpClientTest.php`:
- Around line 9-37: Tests only cover flattenHeaders; add unit tests for
ReactHttpClient's async methods and error paths: create tests for getAsync and
putAsync on ReactHttpClient that mock the ReactPHP Browser and
Response/RequestPromise to simulate successful responses (verify status, body
handling and headers via flattenHeaders) and failing requests (simulate promise
rejection to assert exception handling or error propagation). Use mocks/doubles
for BrowserInterface/Browser, ResponseInterface and the promise returned by
Browser->get()/put(), and reference the ReactHttpClient::getAsync,
ReactHttpClient::putAsync, and flattenHeaders methods to locate where to call
and assert behaviors; include cases for non-2xx responses, JSON bodies and empty
bodies.
In `@tests/JsonExpression/Operator/MatchOperatorTest.php`:
- Around line 33-37: Add a case-insensitive modifier assertion to the
testRegexWithTildeDelimiterInPattern test: use the existing test method
(testRegexWithTildeDelimiterInPattern) and add an assertion calling
$this->operator->evaluate($this->evaluator, ...) that verifies a pattern using
tilde delimiters with the "i" modifier performs a case-insensitive match (e.g.,
different-cased subject should still return true); keep the same call shape as
the other assertions so it exercises delimiter normalisation with modifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd579d2-dc34-46e4-ac0d-58a150e427df
📒 Files selected for processing (20)
src/ABsmartly.phpsrc/Client/Client.phpsrc/Client/ClientConfig.phpsrc/Config.phpsrc/Context/Context.phpsrc/Context/ContextEventHandler.phpsrc/Context/ContextPublisher.phpsrc/Experiment.phpsrc/Http/HTTPClient.phpsrc/Http/ReactHttpClient.phpsrc/JsonExpression/Operator/MatchOperator.phptests/ABsmartlyTest.phptests/Client/ClientConfigTest.phptests/Client/ClientTest.phptests/ConfigTest.phptests/Context/ContextTest.phptests/ExperimentTest.phptests/Fixtures/json/context_custom_fields.jsontests/Http/ReactHttpClientTest.phptests/JsonExpression/Operator/MatchOperatorTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Fixtures/json/context_custom_fields.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ABsmartlyTest.php
- src/Http/ReactHttpClient.php
src/Context/Context.php
Outdated
Comment on lines
+115
to
+118
| if (!str_ends_with($k, '_type')) { | ||
| $keys[$k] = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check PHP version requirement in composer.json and search for other str_ends_with usages
jq '.require.php, .require["symfony/polyfill-php80"]' composer.json
rg -n --type=php '\bstr_ends_with\s*\(' src testsRepository: absmartly/php-sdk
Length of output: 138
🏁 Script executed:
cat composer.jsonRepository: absmartly/php-sdk
Length of output: 1085
🏁 Script executed:
rg -n --type=php '\bstr_starts_with\s*\(' src testsRepository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
rg -n --type=php '\bstr_contains\s*\(' src testsRepository: absmartly/php-sdk
Length of output: 143
🏁 Script executed:
# Check for other PHP 8.0+ exclusive functions in the codebase
rg -n --type=php '\b(fdiv|get_debug_type|get_resource_type|match|str_contains|str_starts_with|str_ends_with)\s*\(' src testsRepository: absmartly/php-sdk
Length of output: 211
str_ends_with() and str_contains() require PHP 8.0+, breaking stated PHP 7.4 support.
The codebase supports PHP 7.4 per composer.json (php-64bit: ^7.4 || ^8.0), but two PHP 8.0-exclusive functions are used:
str_ends_with()atsrc/Context/Context.php:115str_contains()atsrc/JsonExpression/Operator/InOperator.php:34
Replace with PHP 7.4-compatible alternatives:
PHP 7.4-safe replacements
For str_ends_with():
- if (!str_ends_with($k, '_type')) {
+ if (substr($k, -5) !== '_type') {
$keys[$k] = true;
}For str_contains() (in InOperator.php):
- return str_contains($rhs, $lhs);
+ return strpos($rhs, $lhs) !== false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!str_ends_with($k, '_type')) { | |
| $keys[$k] = true; | |
| } | |
| } | |
| if (substr($k, -5) !== '_type') { | |
| $keys[$k] = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Context/Context.php` around lines 115 - 118, Replace PHP 8-only string
helpers with PHP 7.4-safe equivalents: in src/Context/Context.php (the block
using str_ends_with($k, '_type')) swap the call to a substr_compare/substr-based
check (or compare the tail via substr and ===) so it behaves identically; in
src/JsonExpression/Operator/InOperator.php replace str_contains($haystack,
$needle) with a strpos($haystack, $needle) !== false check (or an equivalent
strict !== false test) to preserve semantics on PHP 7.4. Ensure both
replacements preserve exact boolean logic and handle edge cases (empty needle)
the same as the originals.
Comment on lines
+28
to
+31
| $pattern = trim($pattern, '/'); | ||
| $pattern = str_replace('~', '\~', $pattern); | ||
|
|
||
| $matches = @preg_match('~'. $pattern . '~', $text); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MatchOperator.php" -type fRepository: absmartly/php-sdk
Length of output: 108
🏁 Script executed:
cat -n "src/JsonExpression/Operator/MatchOperator.php"Repository: absmartly/php-sdk
Length of output: 1184
🏁 Script executed:
rg -i "runRegexBounded\|MatchOperator" --type php -A 3 -B 3Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
rg -i "preg_match|regex" "src/JsonExpression/Operator/MatchOperator.php" -A 2 -B 2Repository: absmartly/php-sdk
Length of output: 454
🏁 Script executed:
find . -name "*test*" -o -name "*Test*" | grep -i match | head -20Repository: absmartly/php-sdk
Length of output: 146
🏁 Script executed:
find . -type f -name "*.php" | xargs rg -l "MatchOperator" 2>/dev/nullRepository: absmartly/php-sdk
Length of output: 215
🏁 Script executed:
find . -type f \( -name "*.php" -o -name "*.md" \) | xargs rg -i "regex|pattern.*delimiter|modifier" 2>/dev/null | head -30Repository: absmartly/php-sdk
Length of output: 601
🏁 Script executed:
rg -i "test.*match\|matchoperator" --type php -B 2 -A 10 2>/dev/null | head -50Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
cat -n "tests/JsonExpression/Operator/MatchOperatorTest.php"Repository: absmartly/php-sdk
Length of output: 3130
🌐 Web query:
PHP preg_match regex modifiers syntax tilde delimiter examples
💡 Result:
preg_match() pattern syntax (delimiter + pattern + modifiers)
General form:
preg_match('<delim>pattern<delim>modifiers', $subject, $matches);- The pattern must be enclosed by delimiters. A delimiter can be any non-alphanumeric, non-backslash, non-whitespace character (leading whitespace is ignored). Common delimiters include
/,#, and~(tilde). Bracket pairs like{...}or(...)are also allowed. [1] - Modifiers (aka flags) go after the closing delimiter, e.g.
.../i. [1][3]
Why use ~ (tilde) as the delimiter?
It’s not a “tilde operator”; it’s just a delimiter choice. People often use ~ when the regex contains many / characters (e.g., URLs), to avoid escaping them. [1]
Example (URL-ish pattern without escaping /):
preg_match('~^https?://([^/]+)~i', $url, $m);Common modifiers (examples)
From PHP’s PCRE modifiers list: [2]
icase-insensitivemmultiline:^/$match line boundariessdotall:.matches newlines tooxextended: allow whitespace/comments in patternutreat pattern/subject as UTF-8Uungreedy by defaultnno auto-capture (plain( ... )becomes non-capturing) (PHP 8.2+)
Example with multiple modifiers:
preg_match('~^foo.*bar$~imsu', $text, $m);Delimiter escaping rule
If your chosen delimiter appears inside the pattern, escape it (or pick a different delimiter). [1]
preg_match('/http:\/\//', $s); // escape /
preg_match('#http://#', $s); // or switch delimiterSources: PHP manual on delimiters [1], pattern modifiers [2], and preg_match() examples (including non-/ delimiter usage like @...@i) [3].
[1] (php.net)
[2] (php.net)
[3] (php.net)
Citations:
- 1: https://www.php.net/manual/en/regexp.reference.delimiters.php
- 2: https://www.php.net/manual/en/reference.pcre.pattern.modifiers.php
- 3: https://www.php.net/manual/en/function.preg-match.php
🏁 Script executed:
rg -i "pattern.*format|delimiter|modifier" "src/JsonExpression" --type php -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
rg -i "preg_match.*modifier|regex.*flag" --type phpRepository: absmartly/php-sdk
Length of output: 43
Handle regex modifiers in slash-delimited patterns correctly.
When patterns with modifiers are passed (e.g. /abc/i), the modifiers are currently treated as literal pattern text rather than being appended to the preg_match call. For example, /abc/i becomes ~abc/i~ instead of ~abc~i, which changes regex matching behaviour.
The tests cover slash-delimited patterns without modifiers, but not cases where modifiers are present.
Suggested fix
private function runRegexBounded(string $text, string $pattern): ?bool {
- $pattern = trim($pattern, '/');
+ $modifiers = '';
+ if (preg_match('/^\/((?:\\\\\/|[^\/])*)\/([a-zA-Z]*)$/', $pattern, $parts) === 1) {
+ $pattern = $parts[1];
+ $modifiers = $parts[2];
+ } else {
+ $pattern = trim($pattern, '/');
+ }
$pattern = str_replace('~', '\~', $pattern);
- $matches = `@preg_match`('~'. $pattern . '~', $text);
+ $matches = `@preg_match`('~' . $pattern . '~' . $modifiers, $text);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/JsonExpression/Operator/MatchOperator.php` around lines 28 - 31, The
current code trims leading/trailing slashes from $pattern and then escapes
tildes but fails to handle regex modifiers (e.g. /abc/i) — detect when $pattern
is slash-delimited with trailing modifiers, split into the pattern body and
modifier string (keep the modifiers separate), apply escaping only to the body
(e.g. escape '~' in the body), and then call preg_match using the delimiter and
append the modifiers outside the closing delimiter (i.e. preg_match('~' . $body
. '~' . $modifiers, $text)). Update the logic around $pattern and the preg_match
call in MatchOperator.php so that slash-delimited modifiers are preserved and
passed to preg_match.
Comment on lines
+1545
to
+1564
| public function testAttributeUpdatesInTemplates(): void { | ||
| $context = $this->createReadyContext('audience_context.json'); | ||
|
|
||
| $context->setAttribute('age', 15); | ||
| $treatmentBefore = $context->getTreatment('exp_test_ab'); | ||
|
|
||
| $context->publish(); | ||
| $this->eventHandler->submitted = []; | ||
|
|
||
| $context->setAttribute('age', 25); | ||
| $treatmentAfter = $context->getTreatment('exp_test_ab'); | ||
|
|
||
| $context->publish(); | ||
|
|
||
| $events = $this->eventHandler->submitted; | ||
| self::assertCount(1, $events); | ||
|
|
||
| $attributeNames = array_map(fn($attr) => $attr->name, $events[0]->attributes); | ||
| self::assertContains('age', $attributeNames); | ||
| } |
There was a problem hiding this comment.
Test captures $treatmentBefore and $treatmentAfter but never asserts on them.
The variables are assigned but not used, suggesting the test may be incomplete. If the intent is to verify that treatment values change (or don't change) after attribute updates, consider adding assertions. Otherwise, remove the unused variables.
Suggested fix if assertion is intended
$context->setAttribute('age', 15);
$treatmentBefore = $context->getTreatment('exp_test_ab');
$context->publish();
$this->eventHandler->submitted = [];
$context->setAttribute('age', 25);
$treatmentAfter = $context->getTreatment('exp_test_ab');
+
+ // Assert treatment consistency or change as expected
+ self::assertSame($treatmentBefore, $treatmentAfter, 'Treatment should remain consistent despite attribute change');
$context->publish();🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 1549-1549: Avoid unused local variables such as '$treatmentBefore'. (undefined)
(UnusedLocalVariable)
[warning] 1555-1555: Avoid unused local variables such as '$treatmentAfter'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Context/ContextTest.php` around lines 1545 - 1564, The test method
testAttributeUpdatesInTemplates assigns $treatmentBefore and $treatmentAfter in
ContextTest::testAttributeUpdatesInTemplates but never asserts their values;
either remove these unused variables or add explicit assertions comparing them
(e.g., assertNotEquals or assertEquals depending on intended behavior) to verify
that getTreatment('exp_test_ab') changed (or stayed the same) after
setAttribute('age', 25) and publish(); update the test to assert the expected
relationship between $treatmentBefore and $treatmentAfter and keep the existing
event assertions.
- Replace str_ends_with() with substr() for PHP 7.4 compatibility - Fix null $lastException when retries=0 by ensuring at least one attempt - Remove duplicate test case in InOperatorTest
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Context/Context.php (1)
740-759:⚠️ Potential issue | 🟡 MinorUse the custom
RuntimeExceptionclass for consistency with the SDK.Line 751 uses the PHP built-in
\RuntimeException, but the SDK defines a customRuntimeExceptionclass insrc/Exception/RuntimeException.phpthat extends the built-in class and implementsABSmartlyException. Other exception classes in the SDK (TimeoutException, HttpClientError) properly import and use the custom exception. Adduse ABSmartly\SDK\Exception\RuntimeException;to the imports and changenew \RuntimeException($errorMsg)tonew RuntimeException($errorMsg)to maintain consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 740 - 759, In Context::flush(), replace the global PHP exception with the SDK's custom one: add an import for ABSmartly\SDK\Exception\RuntimeException and change the instantiation new \RuntimeException($errorMsg) to new RuntimeException($errorMsg) where ContextEventLoggerEvent is constructed so the SDK uses its custom RuntimeException class consistently with other exceptions like TimeoutException and HttpClientError.
♻️ Duplicate comments (1)
src/Context/Context.php (1)
815-818:⚠️ Potential issue | 🟠 MajorSuccessful refresh does not clear the failed state.
Similar to
setContextData, if the context was previously in a failed state, a successfulrefresh()call does not reset$this->failed. Subsequentflush()/close()calls will still discard data.🐛 Proposed fix
try { $data = $this->dataProvider->getContextData(); $this->setData($data); + $this->failed = false; + $this->readyError = null; $this->logEvent(ContextEventLoggerEvent::Refresh, $data); } catch (Exception $exception) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 815 - 818, refresh() currently sets data and logs success but does not clear the previous failed state, so later flush()/close() still discard data; update the refresh() implementation (the try block that calls $this->dataProvider->getContextData(), $this->setData($data), $this->logEvent(ContextEventLoggerEvent::Refresh, $data)) to reset $this->failed to false on successful refresh (mirroring setContextData behavior), ensuring subsequent flush() and close() treat the context as healthy.
🧹 Nitpick comments (2)
src/Http/HTTPClient.php (1)
104-110: Minor log message inconsistency with retry count.The log message uses
$this->retriesfor the denominator, but$maxAttemptsis used to control the loop. When$this->retries = 5, we getmaxAttempts = 5total attempts, meaning 4 retries. The log would show "attempt 4/5" on the last retry, suggesting a 5th retry exists when it doesn't.Consider using
$maxAttempts - 1for the retry count in the log message for accuracy.♻️ Proposed fix
error_log(sprintf( 'ABsmartly SDK: Retrying HTTP request (attempt %d/%d) after %dms due to error: %s', $attempt, - $this->retries, + $maxAttempts - 1, $backoffMs, $e->getMessage() ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/HTTPClient.php` around lines 104 - 110, The retry log currently uses $this->retries as the denominator while the loop is controlled by $maxAttempts, causing an off-by-one message; update the error_log call in HTTPClient::(where the retry loop runs) to display the total retry attempts as ($maxAttempts - 1) instead of $this->retries (and guard so you never print a negative value by using max(0, $maxAttempts - 1)), keeping $attempt as the numerator so the message correctly reads "attempt X/Y" for retries.src/Context/Context.php (1)
124-142: Consider extracting helper for parsing customFieldValues.The logic for parsing
customFieldValues(handling string/object/array) is duplicated ingetCustomFieldKeys(),getCustomFieldValueType(), andcustomFieldValue(). A private helper would reduce repetition.♻️ Proposed helper extraction
private function parseCustomFieldValues($customFieldValues): ?array { if (is_string($customFieldValues)) { $customFieldValues = json_decode($customFieldValues, true) ?? []; } if (is_object($customFieldValues)) { $customFieldValues = get_object_vars($customFieldValues); } return is_array($customFieldValues) ? $customFieldValues : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 124 - 142, Introduce a private helper (e.g., parseCustomFieldValues) that normalizes the incoming $customFieldValues (handles string -> json decode, object -> get_object_vars, and returns an array or null) and replace the duplicated parsing logic in getCustomFieldValueType, getCustomFieldKeys, and customFieldValue to call this helper; ensure each method returns early when the helper returns null and otherwise uses the normalized array to access keys like $key . '_type' or enumerated keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Context/Context.php`:
- Around line 740-759: In Context::flush(), replace the global PHP exception
with the SDK's custom one: add an import for
ABSmartly\SDK\Exception\RuntimeException and change the instantiation new
\RuntimeException($errorMsg) to new RuntimeException($errorMsg) where
ContextEventLoggerEvent is constructed so the SDK uses its custom
RuntimeException class consistently with other exceptions like TimeoutException
and HttpClientError.
---
Duplicate comments:
In `@src/Context/Context.php`:
- Around line 815-818: refresh() currently sets data and logs success but does
not clear the previous failed state, so later flush()/close() still discard
data; update the refresh() implementation (the try block that calls
$this->dataProvider->getContextData(), $this->setData($data),
$this->logEvent(ContextEventLoggerEvent::Refresh, $data)) to reset $this->failed
to false on successful refresh (mirroring setContextData behavior), ensuring
subsequent flush() and close() treat the context as healthy.
---
Nitpick comments:
In `@src/Context/Context.php`:
- Around line 124-142: Introduce a private helper (e.g., parseCustomFieldValues)
that normalizes the incoming $customFieldValues (handles string -> json decode,
object -> get_object_vars, and returns an array or null) and replace the
duplicated parsing logic in getCustomFieldValueType, getCustomFieldKeys, and
customFieldValue to call this helper; ensure each method returns early when the
helper returns null and otherwise uses the normalized array to access keys like
$key . '_type' or enumerated keys.
In `@src/Http/HTTPClient.php`:
- Around line 104-110: The retry log currently uses $this->retries as the
denominator while the loop is controlled by $maxAttempts, causing an off-by-one
message; update the error_log call in HTTPClient::(where the retry loop runs) to
display the total retry attempts as ($maxAttempts - 1) instead of $this->retries
(and guard so you never print a negative value by using max(0, $maxAttempts -
1)), keeping $attempt as the numerator so the message correctly reads "attempt
X/Y" for retries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a83fcab-d27a-461a-b63a-ed4d5ddee0d5
📒 Files selected for processing (3)
src/Context/Context.phpsrc/Http/HTTPClient.phptests/JsonExpression/Operator/InOperatorTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/JsonExpression/Operator/InOperatorTest.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Config.php (1)
34-40: Delegate deprecated setter to the new setter.Line 37-Line 39 currently duplicates assignment logic. Routing through
setContextPublisher()keeps one update path and avoids future drift.♻️ Proposed refactor
/** * `@deprecated` Use setContextPublisher() instead. */ public function setContextEventHandler(ContextPublisher $contextEventHandler): Config { - $this->contextEventHandler = $contextEventHandler; - return $this; + return $this->setContextPublisher($contextEventHandler); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Config.php` around lines 34 - 40, The deprecated setter setContextEventHandler currently duplicates assignment logic; change it to delegate to the new setter by calling setContextPublisher($contextEventHandler) (or returning its result) instead of directly assigning $this->contextEventHandler, so all updates use the single code path in setContextPublisher and avoid future drift between setters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Config.php`:
- Around line 34-40: The deprecated setter setContextEventHandler currently
duplicates assignment logic; change it to delegate to the new setter by calling
setContextPublisher($contextEventHandler) (or returning its result) instead of
directly assigning $this->contextEventHandler, so all updates use the single
code path in setContextPublisher and avoid future drift between setters.
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
1 participant
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
Test plan
Summary by CodeRabbit
New Features
Improvements
Deprecations
Tests / Chores