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 pull request introduces a substantial refactoring of the ABSmartly PHP SDK. A new facade class 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 unit tests (beta)
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
| "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.
| ### Asynchronously (with ReactPHP) | ||
|
|
||
| When using the async HTTP client, context creation is non-blocking: | ||
|
|
||
| ```php | ||
| $contextConfig = new ContextConfig(); | ||
| $contextConfig->setUnit('session_id', 'session_id5ebf06d8cb5d8137290c4abb64155584fbdb64d8'); // a unique id identifying the user | ||
| use ABSmartly\SDK\Context\ContextConfig; | ||
| use React\Promise\PromiseInterface; | ||
|
|
||
| $contextConfig = new ContextConfig(); | ||
| $contextConfig->setUnit('session_id', '5ebf06d8cb5d8137290c4abb64155584fbdb64d8'); | ||
|
|
||
| $context = $sdk->createContext($contextConfig); | ||
|
|
||
| $context->ready()->then( | ||
| function($context) { | ||
| $treatment = $context->getTreatment('exp_test_experiment'); | ||
| }, | ||
| function($error) { | ||
| error_log('Context failed: ' . $error->getMessage()); | ||
| } | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=php '\bfunction\s+ready\s*\(' src tests
rg -n --type=php '\bfunction\s+(publish|close)\s*\(' src/Context/Context.php
rg -n --type=php 'createContextAsync|createContextPending|publishAsync|closeAsync' src tests README.mdRepository: absmartly/php-sdk
Length of output: 1227
🏁 Script executed:
#!/bin/bash
# Check Context.php for ready() method (account for whitespace variations)
rg -n 'ready' src/Context/Context.php -A 2 -B 2
# Check what createContextAsync returns
rg -n 'createContextAsync' src/ABsmartly.php -A 5
# Look for AsyncContext class or similar
fd -e php src | xargs grep -l 'class.*Async.*Context' | head -5Repository: absmartly/php-sdk
Length of output: 1589
🏁 Script executed:
#!/bin/bash
# Read the specific README sections mentioned in the review
sed -n '124,145p' README.md
echo "---"
sed -n '301,307p' README.md
echo "---"
sed -n '317,323p' README.md
echo "---"
sed -n '640,649p' README.mdRepository: absmartly/php-sdk
Length of output: 1201
🏁 Script executed:
#!/bin/bash
# Search for ready() in all php files (broader search)
rg -n 'ready\s*\(' src --type=php -B 2 -A 2Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Get full createContextAsync implementation
sed -n '119,130p' src/ABsmartly.php
# Check if there's an AsyncContext class
find src -name "*Async*Context*" -type f
# Check what types the promises resolve to
rg -n 'createContextAsync|AsyncContext' src --type=php -B 2 -A 5 | head -60Repository: absmartly/php-sdk
Length of output: 3436
🏁 Script executed:
#!/bin/bash
# Check for __call magic method in Context
rg -n '__call|__get|__invoke' src/Context/Context.php
# Check if Context might extend something else
head -20 src/Context/Context.php | grep -E '^(namespace|use|class)'Repository: absmartly/php-sdk
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Check createContextPending to understand the pending context pattern
sed -n '128,145p' src/ABsmartly.php
# Look for any examples or tests showing correct async usage
find . -name "*test*" -o -name "*example*" | xargs grep -l "createContextAsync\|createContextPending" 2>/dev/null | head -3
# Check if there are any tests showing the correct async pattern
rg -n 'createContextAsync' tests --type=php -A 10 | head -40Repository: absmartly/php-sdk
Length of output: 705
🏁 Script executed:
#!/bin/bash
# Check if AsyncContextEventHandler has async methods for publish/close
rg -n 'publish|close' src/Context/AsyncContextEventHandler.php -B 2 -A 3
# Get the complete AsyncContextEventHandler class
wc -l src/Context/AsyncContextEventHandler.phpRepository: absmartly/php-sdk
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Check the Context class more thoroughly for any method proxying or magic
rg -n 'function (ready|publish|close)' src/Context/Context.php
# Look at the class declaration and parent class
sed -n '1,100p' src/Context/Context.php | grep -A 50 'class Context'
# Check if there's any async wrapper in tests
rg -n 'ready\(\).*then\|createContextAsync' tests --type=php -B 3 -A 5 | head -50Repository: absmartly/php-sdk
Length of output: 1177
These async examples do not work with the current Context API.
The examples at lines 124–145, 301–307, 317–323, and 640–649 contain the following issues:
• $context->ready() does not exist as a method; Context has isReady(): bool only.
• $context->publish() and $context->close() return void, so chaining .then() on them is invalid.
• The examples call synchronous createContext() but then attempt asynchronous Promise chains, which cannot work.
The correct async API is:
• $sdk->createContextAsync($config) returns a PromiseInterface that resolves to a ready Context.
• Alternatively, $sdk->createContextPending($config) returns ['context' => $context, 'promise' => $promise], where the promise resolves when data loading is complete.
Please update these examples to reflect the correct async usage pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 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).
| | 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).
| 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.
| 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.
| } 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.
| $requiredFields = ['id', 'name', 'unitType', 'iteration', 'seedHi', 'seedLo', 'split', | ||
| 'trafficSeedHi', 'trafficSeedLo', 'trafficSplit', 'fullOnVariant', 'applications', 'variants']; | ||
|
|
||
| foreach ($requiredFields as $field) { | ||
| if (!property_exists($data, $field)) { | ||
| throw new \ABSmartly\SDK\Exception\RuntimeException( | ||
| sprintf('Missing required field "%s" in experiment data', $field) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Experiment.php | head -60Repository: absmartly/php-sdk
Length of output: 1889
🏁 Script executed:
find . -type f -name "*.php" | xargs grep -l "Experiment" | grep -i testRepository: absmartly/php-sdk
Length of output: 92
🏁 Script executed:
find . -type f \( -name "*.json" -o -name "*.php" \) | xargs grep -l "audienceStrict" 2>/dev/null | head -20Repository: absmartly/php-sdk
Length of output: 225
🏁 Script executed:
cat tests/Fixtures/json/audience_strict_context.jsonRepository: absmartly/php-sdk
Length of output: 4456
🏁 Script executed:
grep -A 10 -B 5 "audienceStrict" src/Context/Context.php | head -40Repository: absmartly/php-sdk
Length of output: 708
🏁 Script executed:
grep -A 5 -B 5 "audienceStrict" tests/Context/ContextTest.php | head -50Repository: absmartly/php-sdk
Length of output: 371
🏁 Script executed:
grep -r "new Experiment" tests/ --include="*.php" | head -5Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
cat composer.json | grep -A 5 "php"Repository: absmartly/php-sdk
Length of output: 481
🏁 Script executed:
grep -B 5 -A 15 "audienceStrict" tests/Client/ClientAsyncTest.php | head -50Repository: absmartly/php-sdk
Length of output: 638
Add default value to audienceStrict property to prevent uninitialized property errors in PHP 8+.
The audienceStrict property is declared without a default value but is not included in the required fields list. The test fixture and client tests show that this field is optional and may be absent from experiment data. When the property is not present in the input, it remains uninitialized, which causes a TypeError: Typed property must not be accessed before initialization error in PHP 8+ when the property is accessed by the Context class.
public ?object $audience;
- public bool $audienceStrict;
+ public bool $audienceStrict = false;
public array $applications;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Experiment.php` around lines 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.
| private function fetchResponse(): Response { | ||
| $returnedResponse = curl_exec($this->curlHandle); | ||
| $this->throwOnError($returnedResponse); | ||
| $attempt = 0; | ||
| $lastException = null; | ||
|
|
||
| $response = new Response(); | ||
| $response->content = (string) $returnedResponse; | ||
| $response->status = (int) curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE); | ||
| while ($attempt < $this->retries) { | ||
| try { | ||
| $returnedResponse = curl_exec($this->curlHandle); | ||
| $this->throwOnError($returnedResponse); | ||
|
|
||
| return $response; | ||
| $response = new Response(); | ||
| $response->content = (string) $returnedResponse; | ||
| $response->status = (int) curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE); | ||
|
|
||
| return $response; | ||
| } | ||
| catch (HttpClientError $e) { | ||
| $lastException = $e; | ||
| $httpCode = curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE); | ||
| $curlError = curl_errno($this->curlHandle); | ||
|
|
||
| $isRetryable = ($curlError !== 0) || | ||
| ($httpCode >= 500 && $httpCode < 600) || | ||
| $httpCode === 408 || | ||
| $httpCode === 429; | ||
|
|
||
| if (!$isRetryable || $attempt >= $this->retries - 1) { | ||
| throw $e; | ||
| } | ||
|
|
||
| $attempt++; | ||
| $backoffMs = min(1000 * pow(2, $attempt - 1), 10000); | ||
| error_log(sprintf( | ||
| 'ABsmartly SDK: Retrying HTTP request (attempt %d/%d) after %dms due to error: %s', | ||
| $attempt, | ||
| $this->retries, | ||
| $backoffMs, | ||
| $e->getMessage() | ||
| )); | ||
| usleep($backoffMs * 1000); | ||
| } | ||
| } | ||
|
|
||
| throw $lastException; | ||
| } |
There was a problem hiding this comment.
Potential null exception when $retries is 0.
If $this->retries is set to 0, the while loop never executes, $lastException remains null, and line 115 throws null instead of an exception. This would cause a TypeError in PHP 8+.
Consider either:
- Throwing a meaningful exception when retries is 0 and no request is attempted, or
- Ensuring at least one attempt is always made regardless of the
$retriessetting.
Proposed fix
private function fetchResponse(): Response {
$attempt = 0;
$lastException = null;
- while ($attempt < $this->retries) {
+ $maxAttempts = max(1, $this->retries);
+ while ($attempt < $maxAttempts) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/HTTPClient.php` around lines 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.
| 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.
| 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.
1 participant
Summary
Test plan
Summary by CodeRabbit
Release Notes
New Features
Improvements
Deprecations