fix: all tests passing (317/317 unit, 180/183 cross-SDK)#7
fix: all tests passing (317/317 unit, 180/183 cross-SDK)#7
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.
WalkthroughThis pull request renames the public SDK entry to ABsmartly (keeps deprecated SDK alias); updates docs and examples to use ABsmartly::createWithDefaults and camelCase context methods (track(), close()); adds attrsSeq attribute/versioning to Assignment and Context, plus pending(), customFieldValue(), getUnit(), getUnits() accessors; changes assignment caching and audience re-evaluation logic; replaces Close event with Finalize (added constant); allows nullable assignment variables and HTTP request data; adds ReactHttpClient examples; and expands test coverage and fixtures (audience matcher, MD5, Murmur3, refreshed fixtures, many Context tests). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
src/Http/HTTPClient.php (2)
123-133: Consider migrating toCURLOPT_PROTOCOLS_STR/CURLOPT_REDIR_PROTOCOLS_STR.
CURLOPT_PROTOCOLShas been deprecated since libcurl 7.85; PHP does not emit deprecation notices, but on PHP builds with libcurl 7.85 or later,CURLOPT_PROTOCOLS_STR(available since PHP 8.3) is the recommended replacement. The code comment on line 123 even links to the php.watch article that documents exactly this migration. Since the PR already targets PHP 8.4, the replacement constants are unconditionally available.Similarly,
CURLOPT_REDIR_PROTOCOLSis also deprecated since libcurl 7.85, and on PHP 8.3+,CURLOPT_REDIR_PROTOCOLS_STRis the replacement.♻️ Proposed migration to string-based protocol constants
- CURLOPT_PROTOCOLS => CURLPROTO_HTTPS, // Always use HTTPS, never HTTP - CURLOPT_REDIR_PROTOCOLS => CURLPROTO_HTTPS, + CURLOPT_PROTOCOLS_STR => 'HTTPS', // Always use HTTPS, never HTTP + CURLOPT_REDIR_PROTOCOLS_STR => 'HTTPS',The corresponding
use constimports at the top of the file should also be updated accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/HTTPClient.php` around lines 123 - 133, The code uses deprecated numeric protocol options CURLOPT_PROTOCOLS and CURLOPT_REDIR_PROTOCOLS in curl_setopt_array; replace them with the string-based constants CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR (available in PHP 8.3+) and set their values to 'https' (or the equivalent string protocol list you need), while keeping the other curl options intact in the curl_setopt_array call (referencing the curl_setopt_array call and these constant names). Also update the corresponding use const imports at the top of the file to import CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR instead of the deprecated constants.
27-27: Deaduse constimport —CURLOPT_FAILONERRORis never referenced.The constant is imported but not passed to any
curl_setopt*call; error detection is handled manually inthrowOnError(). The import can be removed to keep the namespace tidy.♻️ Proposed cleanup
-use const CURLOPT_FAILONERROR;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/HTTPClient.php` at line 27, Remove the dead import of the CURL constant: delete the `use const CURLOPT_FAILONERROR;` line from HTTPClient.php (the class HTTPClient) since `CURLOPT_FAILONERROR` is never used; confirm there are no `curl_setopt*` calls referencing that constant and that error handling remains in `throwOnError()` so behavior is unchanged.README.md (2)
573-575: Timeout callback does not cancel the in-flight context fetch.The
addTimercallback only prints a message; the underlying HTTP request for context data continues regardless. Users following this example should understand that the timer provides a UI/flow timeout, not a network-level cancellation. A brief inline comment would prevent misuse.♻️ Suggested clarification
$timeout = Loop::addTimer(1.5, function() use ($context) { - echo "Context creation timed out\n"; + // Note: this cancels the *wait*, not the in-flight HTTP request. + // The request will still complete in the background. + echo "Context creation timed out\n"; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 573 - 575, Add an inline clarification next to the Loop::addTimer call (and mention the $timeout variable/closure) that this timer only provides a UI/flow timeout and does not cancel any in-flight HTTP request used to fetch $context; indicate that to actually cancel a network request the example must use a cancellable HTTP client or abortable request handle (e.g., cancel token/abort controller or closing the request handle) rather than relying on the timer alone.
213-214: Callback parameter$contextshadows the outer variable.The
$contextclosure parameter reuses the same name as the enclosing scope's$context, which can confuse readers.♻️ Suggested rename
- function($context) { + function($ctx) { // Context is ready - $treatment = $context->getTreatment('exp_test_experiment'); + $treatment = $ctx->getTreatment('exp_test_experiment'); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 213 - 214, The closure passed to $context->ready()->then currently declares a parameter named $context which shadows the outer $context; rename the closure parameter (e.g., to $ctx or $readyContext) in the function signature and update all references inside that closure to the new name so the outer $context variable is not shadowed (ensure consistent rename of the parameter in the anonymous function passed to ready()->then and any uses within that function body).tests/AudienceMatcherTest.php (1)
21-27: Redundant assertion intestShouldReturnNullIfFilterNotObjectOrArray.The second assertion (line 25–26,
new stdClass()with nofilterproperty) is identical to the body oftestShouldReturnNullOnEmptyAudience. The stated purpose of this test is a null/non-objectfiltervalue; removing the duplicate assertion keeps each test focused on a single invariant.♻️ Proposed cleanup
public function testShouldReturnNullIfFilterNotObjectOrArray(): void { $audience = (object) ['filter' => null]; self::assertNull($this->matcher->evaluate($audience, [])); - - $audience2 = new stdClass(); - self::assertNull($this->matcher->evaluate($audience2, [])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AudienceMatcherTest.php` around lines 21 - 27, The test method testShouldReturnNullIfFilterNotObjectOrArray contains a redundant assertion: remove the second assertion that creates $audience2 = new stdClass() and asserts null on $this->matcher->evaluate($audience2, []), because that case is already covered by testShouldReturnNullOnEmptyAudience; keep only the first assertion using (object) ['filter' => null] so the test focuses on a null/non-object filter value.src/Context/Context.php (3)
77-79: Duplicate public accessors:pending()andgetPendingCount()both return$this->pendingCount.If
pending()is added for cross-SDK API parity, consider deprecatinggetPendingCount()(or vice versa) so the public surface doesn't offer two identical methods indefinitely.Also applies to: 597-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 77 - 79, The class exposes two identical public accessors pending() and getPendingCount() that both return $this->pendingCount; choose one canonical API (e.g., keep pending() for cross-SDK parity) and mark the other as deprecated: update the non-canonical method (getPendingCount) with a `@deprecated` PHPDoc pointing callers to pending(), have it simply return $this->pendingCount (or call pending()) to preserve behavior, and apply the same change to the duplicate pair mentioned (the other copy around the getPendingCount/pending definitions). Ensure both methods remain consistent and add a TODO or ticket reference if you want to remove the deprecated method later.
323-327: Indentation is inconsistent on the$eligibleassignment.Line 323 has an extra level of indentation compared to its surroundings.
Fix indentation
- $eligible = $assigner->assign( + $eligible = $assigner->assign( $experiment->data->trafficSplit, $experiment->data->trafficSeedHi, $experiment->data->trafficSeedLo );Note: the diff above is hard to show purely in text — the fix is to remove the leading extra tab on line 323 so it aligns with the surrounding code at the same nesting level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 323 - 327, Fix the inconsistent indentation on the $eligible assignment: remove the extra leading tab/spaces before " $eligible = $assigner->assign(...)" so that the $eligible line aligns with the surrounding statements at the same nesting level; locate the block where $assigner->assign($experiment->data->trafficSplit, $experiment->data->trafficSeedHi, $experiment->data->trafficSeedLo) is called and adjust indentation to match its neighboring lines.
202-240:customFieldValuelacks a return type declaration and silently swallows malformed JSON.Two observations:
- Missing return type — the method can return
string,int,float,bool,array, ornull. Amixedreturn type would make the contract explicit.- Line 225 —
json_decode($value, true)returnsnullon invalid JSON with no indication of failure. Consider checkingjson_last_error()or usingJSON_THROW_ON_ERROR(as done inExperiment::__construct) so callers can distinguish "field not found" (null) from "unparseable JSON".Proposed improvement
- public function customFieldValue(string $experimentName, string $fieldName) { + public function customFieldValue(string $experimentName, string $fieldName): mixed { $experiment = $this->getExperiment($experimentName); ... if ($type === 'json' && is_string($value)) { - return json_decode($value, true); + return json_decode($value, true, 512, JSON_THROW_ON_ERROR); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 202 - 240, Add an explicit return type to customFieldValue (use ": mixed") and make JSON decoding strict: when decoding $experiment->data->customFieldValues and when decoding a field value of type 'json' in customFieldValue, call json_decode with the JSON_THROW_ON_ERROR flag (or use the equivalent options) inside a try/catch for \JsonException so malformed JSON doesn't silently return null; allow the exception to propagate or rethrow it so callers can distinguish "not found" (null) from "unparseable JSON". Ensure you reference the customFieldValue method and use \JsonException (or import JsonException) when updating the json_decode calls.tests/Context/ContextTest.php (1)
1625-1632: Minor:$contextis assigned but never used.The return value of
createReadyContext()is not referenced after assignment. You can call the method without capturing the result to silence the static analysis warning.Proposed fix
public function testCallsEventLoggerOnSuccess(): void { $eventLogger = new MockContextEventLoggerProxy(); - $context = $this->createReadyContext('context.json', true, $eventLogger); + $this->createReadyContext('context.json', true, $eventLogger); self::assertSame(1, $eventLogger->called); self::assertSame(ContextEventLoggerEvent::Ready, $eventLogger->events[0]->getEvent()); self::assertInstanceOf(\ABSmartly\SDK\Context\ContextData::class, $eventLogger->events[0]->getData()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/ContextTest.php` around lines 1625 - 1632, In testCallsEventLoggerOnSuccess the variable $context is assigned but never used causing a static analysis warning; remove the unused assignment by calling createReadyContext('context.json', true, $eventLogger) without capturing its return value (i.e., drop "$context =") inside the testCallsEventLoggerOnSuccess method so the call still triggers side effects on MockContextEventLoggerProxy while eliminating the unused variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 384-401: In handle(), ensure the Context created by
$this->sdk->createContext($contextConfig) is always closed by wrapping the call
to $next($request) in a try/finally: call $response = $next($request) inside try
and invoke $context->close() in finally so buffered exposures/goals are flushed
even if $next throws, then return or rethrow as appropriate; update references
around handle, createContext and $context->close() to guarantee deterministic
cleanup.
- Line 270: The sentence about overriding a unit type is missing a comma before
the coordinating conjunction; update the README sentence mentioning
Context->setUnit() and Context->setUnits() to read "...as that would be a change
of identity, and would throw an exception." so the comma is inserted before
"and" to fix the grammar.
- Around line 176-184: The README event table is missing the newly added
Finalize event from ContextEventLoggerEvent; add a new table row for the
`Finalize` event showing when it fires (e.g., when `Context->finalize()` method
succeeds) and what data it carries (e.g., `null` or the relevant finalization
payload), so the table documents the `Finalize` constant alongside the existing
`Close` row.
- Around line 199-223: The async README example uses the nonexistent
Context::ready() promise method and must be replaced or removed; update the
snippet that calls $context->ready()->then(...) to use the synchronous API:
after $context = $sdk->createContext($contextConfig) poll or check
$context->isReady() and then call $context->getTreatment('exp_test_experiment')
once isReady() returns true (or show a loop/conditional example), and rename the
inner callback parameter (e.g., from $context to $readyContext) to avoid
shadowing the outer $context variable; alternatively remove the entire async
example and keep only synchronous usage demonstrating createContext, isReady(),
and getTreatment.
In `@src/Context/ContextEventLoggerEvent.php`:
- Around line 15-16: The Close constant in ContextEventLoggerEvent is now unused
and should either be removed or marked deprecated; update the Close constant by
adding a PHP docblock with `@deprecated` pointing consumers to use Finalize
instead (e.g., above the Close constant add "@deprecated Use
ContextEventLoggerEvent::Finalize"), or if you accept breaking changes, remove
the public const Close declaration entirely and ensure no references remain;
keep the Finalize constant unchanged.
In `@src/Http/HTTPClient.php`:
- Line 46: The class currently exposes public int $timeout = 3000 but only
applies it to CURLOPT_CONNECTTIMEOUT_MS (TCP handshake) — add a total-transfer
timeout by setting CURLOPT_TIMEOUT_MS using the same (or a new) property;
specifically, in the code path where CURLOPT_CONNECTTIMEOUT_MS is configured
(and in the other block referenced around lines 124-134) add CURLOPT_TIMEOUT_MS
=> $this->timeout (or introduce separate $connectTimeout and $transferTimeout
properties and set CURLOPT_CONNECTTIMEOUT_MS => $this->connectTimeout and
CURLOPT_TIMEOUT_MS => $this->transferTimeout), and update the property
name/comments to clearly state the unit is milliseconds and that it bounds the
full request (DNS+connect+send+receive).
- Line 45: The public property $retries on class HTTPClient is dead API
surface—remove it (delete the property declaration "public int $retries = 5;")
and any related references or docblocks in the HTTPClient class so callers don't
get a misleading configurable field; alternatively if you prefer to keep
retries, implement retry logic in the request/sending path (e.g., in sendRequest
or request methods) that respects a configured retry count and backoff, but do
not leave the public $retries field declared without behavior.
In `@tests/Context/ContextTest.php`:
- Around line 1545-1564: The test testAttributeUpdatesInTemplates currently
assigns $treatmentBefore and $treatmentAfter from getTreatment but never uses
them; either remove those unused local variables or (if the intent is to verify
the template behavior when age changes) add an assertion comparing them—e.g.,
after calling setAttribute('age', 15) and later setAttribute('age', 25) and
calling getTreatment('exp_test_ab'), assertNotEquals($treatmentBefore,
$treatmentAfter) (or assertEquals if you expect no change) so the variables are
meaningfully asserted; update the test accordingly around the
getTreatment/setAttribute/publish calls.
In `@tests/Murmur3Test.php`:
- Around line 7-10: Add a PHPUnit class-level annotation to skip this test on
PHP < 8.1: update the Murmur3Test test class (class Murmur3Test) so that the
PHPUnit docblock includes "@requires PHP 8.1" above the class declaration; this
ensures the murmur3Hash(string $input, int $seed = 0) helper (which calls
hash('murmur3a', …) with the ['seed' => $seed] option) does not run on pre-8.1
environments and avoids the ValueError.
---
Nitpick comments:
In `@README.md`:
- Around line 573-575: Add an inline clarification next to the Loop::addTimer
call (and mention the $timeout variable/closure) that this timer only provides a
UI/flow timeout and does not cancel any in-flight HTTP request used to fetch
$context; indicate that to actually cancel a network request the example must
use a cancellable HTTP client or abortable request handle (e.g., cancel
token/abort controller or closing the request handle) rather than relying on the
timer alone.
- Around line 213-214: The closure passed to $context->ready()->then currently
declares a parameter named $context which shadows the outer $context; rename the
closure parameter (e.g., to $ctx or $readyContext) in the function signature and
update all references inside that closure to the new name so the outer $context
variable is not shadowed (ensure consistent rename of the parameter in the
anonymous function passed to ready()->then and any uses within that function
body).
In `@src/Context/Context.php`:
- Around line 77-79: The class exposes two identical public accessors pending()
and getPendingCount() that both return $this->pendingCount; choose one canonical
API (e.g., keep pending() for cross-SDK parity) and mark the other as
deprecated: update the non-canonical method (getPendingCount) with a `@deprecated`
PHPDoc pointing callers to pending(), have it simply return $this->pendingCount
(or call pending()) to preserve behavior, and apply the same change to the
duplicate pair mentioned (the other copy around the getPendingCount/pending
definitions). Ensure both methods remain consistent and add a TODO or ticket
reference if you want to remove the deprecated method later.
- Around line 323-327: Fix the inconsistent indentation on the $eligible
assignment: remove the extra leading tab/spaces before " $eligible =
$assigner->assign(...)" so that the $eligible line aligns with the surrounding
statements at the same nesting level; locate the block where
$assigner->assign($experiment->data->trafficSplit,
$experiment->data->trafficSeedHi, $experiment->data->trafficSeedLo) is called
and adjust indentation to match its neighboring lines.
- Around line 202-240: Add an explicit return type to customFieldValue (use ":
mixed") and make JSON decoding strict: when decoding
$experiment->data->customFieldValues and when decoding a field value of type
'json' in customFieldValue, call json_decode with the JSON_THROW_ON_ERROR flag
(or use the equivalent options) inside a try/catch for \JsonException so
malformed JSON doesn't silently return null; allow the exception to propagate or
rethrow it so callers can distinguish "not found" (null) from "unparseable
JSON". Ensure you reference the customFieldValue method and use \JsonException
(or import JsonException) when updating the json_decode calls.
In `@src/Http/HTTPClient.php`:
- Around line 123-133: The code uses deprecated numeric protocol options
CURLOPT_PROTOCOLS and CURLOPT_REDIR_PROTOCOLS in curl_setopt_array; replace them
with the string-based constants CURLOPT_PROTOCOLS_STR and
CURLOPT_REDIR_PROTOCOLS_STR (available in PHP 8.3+) and set their values to
'https' (or the equivalent string protocol list you need), while keeping the
other curl options intact in the curl_setopt_array call (referencing the
curl_setopt_array call and these constant names). Also update the corresponding
use const imports at the top of the file to import CURLOPT_PROTOCOLS_STR and
CURLOPT_REDIR_PROTOCOLS_STR instead of the deprecated constants.
- Line 27: Remove the dead import of the CURL constant: delete the `use const
CURLOPT_FAILONERROR;` line from HTTPClient.php (the class HTTPClient) since
`CURLOPT_FAILONERROR` is never used; confirm there are no `curl_setopt*` calls
referencing that constant and that error handling remains in `throwOnError()` so
behavior is unchanged.
In `@tests/AudienceMatcherTest.php`:
- Around line 21-27: The test method
testShouldReturnNullIfFilterNotObjectOrArray contains a redundant assertion:
remove the second assertion that creates $audience2 = new stdClass() and asserts
null on $this->matcher->evaluate($audience2, []), because that case is already
covered by testShouldReturnNullOnEmptyAudience; keep only the first assertion
using (object) ['filter' => null] so the test focuses on a null/non-object
filter value.
In `@tests/Context/ContextTest.php`:
- Around line 1625-1632: In testCallsEventLoggerOnSuccess the variable $context
is assigned but never used causing a static analysis warning; remove the unused
assignment by calling createReadyContext('context.json', true, $eventLogger)
without capturing its return value (i.e., drop "$context =") inside the
testCallsEventLoggerOnSuccess method so the call still triggers side effects on
MockContextEventLoggerProxy while eliminating the unused variable.
| | 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.
Finalize event type is absent from the event table.
The PR objective and AI summary both state that a Finalize event type was added to ContextEventLoggerEvent. The event table only documents Close and does not include Finalize, leaving the new constant undocumented.
Add the missing row:
📝 Proposed addition to the event table
| `Close` | `Context->close()` method succeeds the first time | `null` |
+| `Finalize` | `Context->close()` seals the context after publishing | `null` |📝 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.
| | 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` | | |
| | 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` | | |
| | `Finalize` | `Context->close()` seals the context after publishing | `null` | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 176 - 184, The README event table is missing the
newly added Finalize event from ContextEventLoggerEvent; add a new table row for
the `Finalize` event showing when it fires (e.g., when `Context->finalize()`
method succeeds) and what data it carries (e.g., `null` or the relevant
finalization payload), so the table documents the `Finalize` constant alongside
the existing `Close` row.
| ### Asynchronously (with ReactPHP) | ||
|
|
||
| **With Prefetched Data** | ||
| 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); | ||
|
|
||
| // Use promises for async operations | ||
| $context->ready()->then( | ||
| function($context) { | ||
| // Context is ready | ||
| $treatment = $context->getTreatment('exp_test_experiment'); | ||
| }, | ||
| function($error) { | ||
| // Handle error | ||
| error_log('Context failed: ' . $error->getMessage()); | ||
| } | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the ready() method definition in the Context class
ast-grep --pattern 'public function ready($$$)'Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
# Also search for any ready() method, not just public
rg 'function ready\(' --type php -A 5Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
# Find the Context class file to examine its structure
fd -e php -t f | xargs rg -l 'class Context'Repository: absmartly/php-sdk
Length of output: 443
🏁 Script executed:
# Read the Context.php file to examine available methods
cat -n src/Context/Context.php | head -150Repository: absmartly/php-sdk
Length of output: 4894
🏁 Script executed:
# Search for Promise or async-related code in Context
rg 'Promise|async|ready' src/Context/Context.php -iRepository: absmartly/php-sdk
Length of output: 516
🏁 Script executed:
# Check the entire Context class for public methods
ast-grep --pattern 'class Context { $$$ }'Repository: absmartly/php-sdk
Length of output: 36142
Remove the async example or replace with synchronous API.
The ready() method does not exist on the Context class. The code at lines 213 and 577 calling $context->ready()->then(...) will fail at runtime. The Context class only provides isReady(): bool, which returns a boolean instead of a Promise. Either remove the async example entirely, or rewrite it using the synchronous API (isReady() checks) without promise-based callbacks.
As a secondary note, the callback parameter $context at line 213 shadows the outer $context variable; rename the parameter for clarity (e.g., $readyContext).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 199 - 223, The async README example uses the
nonexistent Context::ready() promise method and must be replaced or removed;
update the snippet that calls $context->ready()->then(...) to use the
synchronous API: after $context = $sdk->createContext($contextConfig) poll or
check $context->isReady() and then call
$context->getTreatment('exp_test_experiment') once isReady() returns true (or
show a loop/conditional example), and rename the inner callback parameter (e.g.,
from $context to $readyContext) to avoid shadowing the outer $context variable;
alternatively remove the entire async example and keep only synchronous usage
demonstrating createContext, isReady(), and getTreatment.
| ]); | ||
| ``` | ||
|
|
||
| > **Note:** You cannot override an already set unit type as that would be a change of identity and would throw an exception. In this case, you must create a new context instead. The `Context->setUnit()` and `Context->setUnits()` methods can be called before the context is ready. |
There was a problem hiding this comment.
Missing comma before the conjunction.
✏️ Proposed fix
-> **Note:** You cannot override an already set unit type as that would be a change of identity and would throw an exception.
+> **Note:** You cannot override an already set unit type as that would be a change of identity, and would throw an exception.🧰 Tools
🪛 LanguageTool
[uncategorized] ~270-~270: Possible missing comma found.
Context: ...You cannot override an already set unit type as that would be a change of identity a...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 270, The sentence about overriding a unit type is missing
a comma before the coordinating conjunction; update the README sentence
mentioning Context->setUnit() and Context->setUnits() to read "...as that would
be a change of identity, and would throw an exception." so the comma is inserted
before "and" to fix the grammar.
| public function handle($request, Closure $next) | ||
| { | ||
| $contextConfig = new ContextConfig(); | ||
| $contextConfig->setUnit('session_id', $request->session()->getId()); | ||
|
|
||
| if (auth()->check()) { | ||
| $contextConfig->setUnit('user_id', auth()->id()); | ||
| } | ||
|
|
||
| $context = $this->sdk->createContext($contextConfig); | ||
| $request->attributes->set('absmartly_context', $context); | ||
|
|
||
| $response = $next($request); | ||
|
|
||
| $context->close(); | ||
|
|
||
| return $response; | ||
| } |
There was a problem hiding this comment.
$context->close() is not guarded with try/finally in the Laravel middleware.
If $next($request) throws, the context is never closed and any buffered exposures or goal events will be silently lost.
🛡️ Proposed fix
- $response = $next($request);
-
- $context->close();
-
- return $response;
+ try {
+ $response = $next($request);
+ } finally {
+ $context->close();
+ }
+
+ return $response;📝 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.
| public function handle($request, Closure $next) | |
| { | |
| $contextConfig = new ContextConfig(); | |
| $contextConfig->setUnit('session_id', $request->session()->getId()); | |
| if (auth()->check()) { | |
| $contextConfig->setUnit('user_id', auth()->id()); | |
| } | |
| $context = $this->sdk->createContext($contextConfig); | |
| $request->attributes->set('absmartly_context', $context); | |
| $response = $next($request); | |
| $context->close(); | |
| return $response; | |
| } | |
| public function handle($request, Closure $next) | |
| { | |
| $contextConfig = new ContextConfig(); | |
| $contextConfig->setUnit('session_id', $request->session()->getId()); | |
| if (auth()->check()) { | |
| $contextConfig->setUnit('user_id', auth()->id()); | |
| } | |
| $context = $this->sdk->createContext($contextConfig); | |
| $request->attributes->set('absmartly_context', $context); | |
| try { | |
| $response = $next($request); | |
| } finally { | |
| $context->close(); | |
| } | |
| return $response; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 384 - 401, In handle(), ensure the Context created by
$this->sdk->createContext($contextConfig) is always closed by wrapping the call
to $next($request) in a try/finally: call $response = $next($request) inside try
and invoke $context->close() in finally so buffered exposures/goals are flushed
even if $next throws, then return or rethrow as appropriate; update references
around handle, createContext and $context->close() to guarantee deterministic
cleanup.
| public const Close = 'Close'; | ||
| public const Finalize = 'Finalize'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=php 'ContextEventLoggerEvent::Close\b' -A2 -B2Repository: absmartly/php-sdk
Length of output: 43
🏁 Script executed:
cat -n src/Context/ContextEventLoggerEvent.phpRepository: absmartly/php-sdk
Length of output: 902
🏁 Script executed:
rg -n --type=php 'ContextEventLoggerEvent::Finalize\b' -A2 -B2Repository: absmartly/php-sdk
Length of output: 3030
Add deprecation docblock to Close constant or remove it.
The Close constant is not used anywhere in the codebase; Context.php now emits Finalize instead. To avoid confusing SDK consumers, add a @deprecated docblock directing them to Finalize, or remove the constant entirely if backwards compatibility is not a concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Context/ContextEventLoggerEvent.php` around lines 15 - 16, The Close
constant in ContextEventLoggerEvent is now unused and should either be removed
or marked deprecated; update the Close constant by adding a PHP docblock with
`@deprecated` pointing consumers to use Finalize instead (e.g., above the Close
constant add "@deprecated Use ContextEventLoggerEvent::Finalize"), or if you
accept breaking changes, remove the public const Close declaration entirely and
ensure no references remain; keep the Finalize constant unchanged.
| @@ -45,7 +45,7 @@ class HTTPClient { | |||
| public int $retries = 5; | |||
There was a problem hiding this comment.
$retries is public but has no effect — either implement or remove it.
No retry logic exists anywhere in the class; the property is dead API surface. Callers who configure it will silently get no retries, contrary to expectation.
♻️ Option A — remove the property until retry logic is implemented
- public int $retries = 5;
public int $timeout = 3000;📝 Committable suggestion
‼️ 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.
| public int $retries = 5; | |
| public int $timeout = 3000; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/HTTPClient.php` at line 45, The public property $retries on class
HTTPClient is dead API surface—remove it (delete the property declaration
"public int $retries = 5;") and any related references or docblocks in the
HTTPClient class so callers don't get a misleading configurable field;
alternatively if you prefer to keep retries, implement retry logic in the
request/sending path (e.g., in sendRequest or request methods) that respects a
configured retry count and backoff, but do not leave the public $retries field
declared without behavior.
| @@ -45,7 +45,7 @@ class HTTPClient { | |||
| public int $retries = 5; | |||
| public int $timeout = 3000; | |||
There was a problem hiding this comment.
Only connection timeout is set — total transfer timeout is missing.
CURLOPT_CONNECTTIMEOUT_MS caps only the TCP handshake phase. There is no CURLOPT_TIMEOUT_MS to bound the full transfer (DNS + connect + send + receive), so a server that accepts the connection but sends a slow or never-ending response will stall the calling process indefinitely. The public $timeout property implies full-request coverage, which is misleading.
🛡️ Proposed fix — add total transfer timeout
use const CURLOPT_CONNECTTIMEOUT_MS;
+use const CURLOPT_TIMEOUT_MS; CURLOPT_CONNECTTIMEOUT_MS => $this->timeout,
+ CURLOPT_TIMEOUT_MS => $this->timeout,Also applies to: 124-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/HTTPClient.php` at line 46, The class currently exposes public int
$timeout = 3000 but only applies it to CURLOPT_CONNECTTIMEOUT_MS (TCP handshake)
— add a total-transfer timeout by setting CURLOPT_TIMEOUT_MS using the same (or
a new) property; specifically, in the code path where CURLOPT_CONNECTTIMEOUT_MS
is configured (and in the other block referenced around lines 124-134) add
CURLOPT_TIMEOUT_MS => $this->timeout (or introduce separate $connectTimeout and
$transferTimeout properties and set CURLOPT_CONNECTTIMEOUT_MS =>
$this->connectTimeout and CURLOPT_TIMEOUT_MS => $this->transferTimeout), and
update the property name/comments to clearly state the unit is milliseconds and
that it bounds the full request (DNS+connect+send+receive).
| 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.
Unused local variables $treatmentBefore and $treatmentAfter.
Both variables are assigned but never asserted or used. If the intent is to verify that treatments differ (or remain the same) after an attribute change, add assertions. Otherwise, remove them to keep the test's intent clear.
Proposed fix — either assert or remove
public function testAttributeUpdatesInTemplates(): void {
$context = $this->createReadyContext('audience_context.json');
$context->setAttribute('age', 15);
- $treatmentBefore = $context->getTreatment('exp_test_ab');
+ $context->getTreatment('exp_test_ab');
$context->publish();
$this->eventHandler->submitted = [];
$context->setAttribute('age', 25);
- $treatmentAfter = $context->getTreatment('exp_test_ab');
+ $context->getTreatment('exp_test_ab');
$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
testAttributeUpdatesInTemplates currently assigns $treatmentBefore and
$treatmentAfter from getTreatment but never uses them; either remove those
unused local variables or (if the intent is to verify the template behavior when
age changes) add an assertion comparing them—e.g., after calling
setAttribute('age', 15) and later setAttribute('age', 25) and calling
getTreatment('exp_test_ab'), assertNotEquals($treatmentBefore, $treatmentAfter)
(or assertEquals if you expect no change) so the variables are meaningfully
asserted; update the test accordingly around the
getTreatment/setAttribute/publish calls.
| class Murmur3Test extends TestCase { | ||
| private function murmur3Hash(string $input, int $seed = 0): int { | ||
| return hexdec(hash('murmur3a', $input, false, ['seed' => $seed])); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP hash murmur3a algorithm minimum version requirement
💡 Result:
murmur3a is available in PHP’s built-in hash() / hash_algos() starting with PHP 8.1.0 (MurmurHash3 support was added in 8.1). [1] [2]
🏁 Script executed:
# Check the test file for annotations and find the production code
find . -name "Murmur3Test.php" -o -name "*VariantAssigner*" | head -20Repository: absmartly/php-sdk
Length of output: 142
🏁 Script executed:
# Search for VariantAssigner class and digest method
rg "class VariantAssigner|function digest" -A 5 -B 2Repository: absmartly/php-sdk
Length of output: 1309
🏁 Script executed:
# Get full content of VariantAssigner.php digest() method and surrounding context
cat -n src/VariantAssigner.php | head -50Repository: absmartly/php-sdk
Length of output: 1449
🏁 Script executed:
# Get full content of Murmur3Test.php to see annotations and code
cat -n tests/Murmur3Test.phpRepository: absmartly/php-sdk
Length of output: 2840
🌐 Web query:
PHP hash() function $options parameter minimum version requirement
💡 Result:
The $options parameter for PHP’s hash() function was added in PHP 8.1.0 (so the minimum required version is PHP 8.1.0). [1]
(It was introduced alongside the same new $options parameter for hash_file() and hash_init().) [2]
Sources: [1], [2]
🏁 Script executed:
# Get the complete digest() method in VariantAssigner.php
sed -n '50,60p' src/VariantAssigner.phpRepository: absmartly/php-sdk
Length of output: 405
Add @requires PHP 8.1 class-level annotation to guard against pre-8.1 environments.
The test's murmur3Hash() helper uses hash('murmur3a', …) and its ['seed' => $seed] option, both introduced in PHP 8.1. The production code in VariantAssigner::digest() guards this with PHP_VERSION_ID >= 80100 and provides a fallback; without a matching PHPUnit @requires annotation, the test class will throw a ValueError (unknown algorithm) rather than be skipped on PHP 8.0 or earlier runners.
Proposed fix
+/**
+ * `@requires` PHP 8.1
+ */
class Murmur3Test extends TestCase {📝 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.
| class Murmur3Test extends TestCase { | |
| private function murmur3Hash(string $input, int $seed = 0): int { | |
| return hexdec(hash('murmur3a', $input, false, ['seed' => $seed])); | |
| } | |
| /** | |
| * `@requires` PHP 8.1 | |
| */ | |
| class Murmur3Test extends TestCase { | |
| private function murmur3Hash(string $input, int $seed = 0): int { | |
| return hexdec(hash('murmur3a', $input, false, ['seed' => $seed])); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Murmur3Test.php` around lines 7 - 10, Add a PHPUnit class-level
annotation to skip this test on PHP < 8.1: update the Murmur3Test test class
(class Murmur3Test) so that the PHPUnit docblock includes "@requires PHP 8.1"
above the class declaration; this ensures the murmur3Hash(string $input, int
$seed = 0) helper (which calls hash('murmur3a', …) with the ['seed' => $seed]
option) does not run on pre-8.1 environments and avoids the ValueError.
- InOperator: fix argument order (lhs=needle, rhs=haystack) matching cross-SDK spec, use loose comparison for mixed types - MatchOperator: use ~ as regex delimiter instead of / to avoid conflicts with patterns containing forward slashes - Context: change error message from "closed" to "finalized" to match cross-SDK error_matches expectations - Update InOperatorTest to use correct [needle, haystack] argument order
joalves
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/JsonExpression/Operator/InOperator.php (2)
23-24: Document the intentional loose comparison inin_array.
in_array($lhs, $rhs, false)uses non-strict comparison, which meansin_array(false, [0], false)returnstruedue to PHP's bool/int coercion. This is intentional per the PR's cross-SDK compatibility requirement, but the absence of a comment makes it a maintenance trap.📝 Suggested inline comment
if (is_array($rhs)) { - return in_array($lhs, $rhs, false); + // Loose comparison is intentional for cross-SDK mixed-type compatibility. + return in_array($lhs, $rhs, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/JsonExpression/Operator/InOperator.php` around lines 23 - 24, Add an inline comment next to the non-strict in_array call in InOperator::evaluate (the line using in_array($lhs, $rhs, false)) explaining that the third parameter is intentionally false to allow loose comparisons for cross-SDK compatibility and noting the known coercion edge (e.g., false == 0) so future maintainers won't change it inadvertently; place the comment immediately above or beside the in_array(...) call in src/JsonExpression/Operator/InOperator.php and reference the cross-SDK compatibility requirement and the specific coercion example.
18-18: Consider suppressing the PHPMDUnusedFormalParameterwarning for$evaluator.The parameter is required by
BinaryOperator::binary()but is not used by this specific implementation. Whilst the warning is a false positive in context, leaving it unaddressed will clutter static-analysis reports over time.🔧 Suggested suppression
- public function binary(Evaluator $evaluator, $lhs, $rhs): ?bool { + /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ + public function binary(Evaluator $evaluator, $lhs, $rhs): ?bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/JsonExpression/Operator/InOperator.php` at line 18, Add a PHPMD suppression for the unused `$evaluator` parameter on InOperator::binary: annotate the method (public function binary(Evaluator $evaluator, $lhs, $rhs): ?bool) with a PHPMD suppression such as /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ so the implementation (which must keep the signature required by BinaryOperator::binary) does not trigger the UnusedFormalParameter warning.src/Context/Context.php (2)
532-534:getUnitis missing a?stringreturn-type declaration.
ContextConfig::getUnit(the equivalent method in the config class) declares?string. The corresponding method here omits the return type entirely, which weakens static analysis.♻️ Proposed fix
-public function getUnit(string $unitType) { +public function getUnit(string $unitType): ?string { return $this->units[$unitType] ?? 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 532 - 534, The getUnit method on the Context class is missing a nullable string return-type; update the method signature for public function getUnit(string $unitType) to declare a return type of ?string so it matches ContextConfig::getUnit and improves static analysis (ensure the method still returns $this->units[$unitType] ?? null).
77-79:pending()duplicates the existinggetPendingCount()at line 597.Both methods return
$this->pendingCountunchanged. Ifpending()is required for cross-SDK API parity,getPendingCount()should be annotated@deprecatedand proxied throughpending()to avoid two sources of truth.♻️ Proposed consolidation
public function pending(): int { - return $this->pendingCount; + return $this->pendingCount; // canonical implementation stays here } // … +/** `@deprecated` Use pending() instead */ public function getPendingCount(): int { - return $this->pendingCount; + return $this->pending(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 77 - 79, Duplicate accessors: make pending() the single source of truth and convert getPendingCount() into a deprecated proxy. Add a `@deprecated` docblock to getPendingCount(), change its body to simply return $this->pending() (or return $this->pendingCount via pending()) so all callers funnel through pending(), and ensure pending() remains the authoritative method returning $this->pendingCount.
🤖 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 250-265: audienceMatches currently treats a null from
AudienceMatcher::evaluate() as a mismatch due to using !$result and also mutates
$assignment->attrsSeq inside what appears to be a boolean check; add a brief
inline comment in audienceMatches by the evaluate() call clarifying that
evaluate() returns ?bool and null is intentionally treated as a mismatch here
(i.e. "null => no filter => treat as mismatch for cache invalidation"), and
remove the side-effect by moving the assignment of $assignment->attrsSeq =
$this->attrsSeq out of audienceMatches into the getAssignment call site (call
audienceMatches to decide match, then update $assignment->attrsSeq in
getAssignment when appropriate) so audienceMatches remains a pure predicate
while preserving cache-coherence via getAssignment.
- Around line 209-211: The json_decode call that converts $customFieldValues
should use JSON_THROW_ON_ERROR (and appropriate flags) so malformed JSON doesn't
silently become null; update the decode in the Context class (the
$customFieldValues conversion) to call json_decode(..., true, 512,
JSON_THROW_ON_ERROR) and wrap it in a try/catch to surface or rethrow the
JsonException (matching the pattern used in Experiment::__construct); apply the
same change to the other json_decode occurrence around the later check (the
similar call near line 225) so both spots consistently throw on parse errors.
- Around line 202-207: The customFieldValue method currently skips lifecycle
checks; add a call to checkReady() at the start of customFieldValue(string
$experimentName, string $fieldName) so it enforces the same not-ready/not-closed
guard as other public data-accessors (checkReady() itself calls
checkNotClosed()). Place the checkReady() invocation before getExperiment(...)
to ensure the method throws when the Context is not ready or already finalised,
matching the class's lifecycle contract.
In `@src/JsonExpression/Operator/MatchOperator.php`:
- Line 28: The current trim($pattern, '/') call can leave trailing PCRE flags
attached to the body (e.g. '/foo/i' -> 'foo/i'), so update the handling in
MatchOperator to detect slash-delimited patterns and split the pattern body and
flags before re-wrapping: if $pattern starts with '/' find the last '/'
position, take the substring between first and last slash as the body and the
substring after the last slash as flags, escape any new delimiter characters in
the body, then build the final regex using the chosen delimiter (e.g. '~') and
append the flags; apply this logic where $pattern is processed in the
MatchOperator class to ensure flags are preserved correctly.
- Around line 28-30: The current MatchOperator implementation trims slashes then
builds a regex using '~' as the delimiter and calls
preg_match('@preg_match(\'~'. $pattern . '~\', $text)@'), which breaks when the
user pattern contains '~'; change this to escape occurrences of the chosen
delimiter inside $pattern (or call preg_quote($pattern, '~')) before
concatenation so embedded '~' do not produce malformed delimiters — update the
code that sets $pattern (and the preg_match invocation in MatchOperator.php) to
use the escaped pattern (or a preg_quote-based variant) when wrapping with the
'~' delimiters.
In `@tests/JsonExpression/Operator/InOperatorTest.php`:
- Around line 64-67: Rename the misleading test method testArrayDiffNull to
reflect it asserts false (e.g., testArrayDiffFalse or testArrayNotInArray) and
update any references to that method; the assertions call
$this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]]) and [[5, 6],
[1, 2, 3]] and expect false, so only the test method name needs to change
(ensure any test annotations or usages are updated to the new name).
- Around line 29-36: In testReturnFalseOnEmptyArray replace the duplicated
assertion that calls $this->operator->evaluate($this->evaluator, [false, []])
with an assertion that checks the integer 0 case (e.g. invoke
$this->operator->evaluate($this->evaluator, [0, []])) so the test covers the
edge-case of loose-comparison/in_array behavior; keep the other assertions
(false, "1", true, null) and ensure you still assertFalse for the 0 invocation
to match the test intent.
---
Nitpick comments:
In `@src/Context/Context.php`:
- Around line 532-534: The getUnit method on the Context class is missing a
nullable string return-type; update the method signature for public function
getUnit(string $unitType) to declare a return type of ?string so it matches
ContextConfig::getUnit and improves static analysis (ensure the method still
returns $this->units[$unitType] ?? null).
- Around line 77-79: Duplicate accessors: make pending() the single source of
truth and convert getPendingCount() into a deprecated proxy. Add a `@deprecated`
docblock to getPendingCount(), change its body to simply return $this->pending()
(or return $this->pendingCount via pending()) so all callers funnel through
pending(), and ensure pending() remains the authoritative method returning
$this->pendingCount.
In `@src/JsonExpression/Operator/InOperator.php`:
- Around line 23-24: Add an inline comment next to the non-strict in_array call
in InOperator::evaluate (the line using in_array($lhs, $rhs, false)) explaining
that the third parameter is intentionally false to allow loose comparisons for
cross-SDK compatibility and noting the known coercion edge (e.g., false == 0) so
future maintainers won't change it inadvertently; place the comment immediately
above or beside the in_array(...) call in
src/JsonExpression/Operator/InOperator.php and reference the cross-SDK
compatibility requirement and the specific coercion example.
- Line 18: Add a PHPMD suppression for the unused `$evaluator` parameter on
InOperator::binary: annotate the method (public function binary(Evaluator
$evaluator, $lhs, $rhs): ?bool) with a PHPMD suppression such as /**
`@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ so the implementation (which
must keep the signature required by BinaryOperator::binary) does not trigger the
UnusedFormalParameter warning.
| public function customFieldValue(string $experimentName, string $fieldName) { | ||
| $experiment = $this->getExperiment($experimentName); | ||
| if ($experiment === null || !isset($experiment->data->customFieldValues)) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
customFieldValue must guard against a not-ready or finalised context.
Every other public data-access method calls checkReady() (which in turn calls checkNotClosed()). Without this guard, customFieldValue silently succeeds on a finalised context, diverging from the lifecycle contract enforced everywhere else.
🛡️ Proposed fix
public function customFieldValue(string $experimentName, string $fieldName) {
+ $this->checkReady();
$experiment = $this->getExperiment($experimentName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Context/Context.php` around lines 202 - 207, The customFieldValue method
currently skips lifecycle checks; add a call to checkReady() at the start of
customFieldValue(string $experimentName, string $fieldName) so it enforces the
same not-ready/not-closed guard as other public data-accessors (checkReady()
itself calls checkNotClosed()). Place the checkReady() invocation before
getExperiment(...) to ensure the method throws when the Context is not ready or
already finalised, matching the class's lifecycle contract.
| if (is_string($customFieldValues)) { | ||
| $customFieldValues = json_decode($customFieldValues, true); | ||
| } |
There was a problem hiding this comment.
json_decode without JSON_THROW_ON_ERROR silently discards malformed JSON.
If customFieldValues is an invalid JSON string, json_decode returns null; is_object(null) is false, isset(null[$fieldName]) is false, and the method returns null — masking the parse error entirely. Experiment::__construct already uses JSON_THROW_ON_ERROR for consistency.
🐛 Proposed fix (apply same pattern on line 225 too)
- $customFieldValues = json_decode($customFieldValues, true);
+ $customFieldValues = json_decode($customFieldValues, true, 512, JSON_THROW_ON_ERROR);- return json_decode($value, true);
+ return json_decode($value, true, 512, JSON_THROW_ON_ERROR);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Context/Context.php` around lines 209 - 211, The json_decode call that
converts $customFieldValues should use JSON_THROW_ON_ERROR (and appropriate
flags) so malformed JSON doesn't silently become null; update the decode in the
Context class (the $customFieldValues conversion) to call json_decode(..., true,
512, JSON_THROW_ON_ERROR) and wrap it in a try/catch to surface or rethrow the
JsonException (matching the pattern used in Experiment::__construct); apply the
same change to the other json_decode occurrence around the later check (the
similar call near line 225) so both spots consistently throw on parse errors.
| private function audienceMatches(Experiment $experiment, Assignment $assignment): bool { | ||
| if (!empty($experiment->audience) && !empty((array) $experiment->audience)) { | ||
| if ($this->attrsSeq > ($assignment->attrsSeq ?? 0)) { | ||
| $attrs = $this->getAttributes(); | ||
| $result = $this->audienceMatcher->evaluate($experiment->audience, $attrs); | ||
| $newAudienceMismatch = !$result; | ||
|
|
||
| if ($newAudienceMismatch !== $assignment->audienceMismatch) { | ||
| return false; | ||
| } | ||
|
|
||
| $assignment->attrsSeq = $this->attrsSeq; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
audienceMatches silently treats a null result from evaluate() as a mismatch.
AudienceMatcher::evaluate() returns ?bool — null signals "no filter defined". Because !null === true in PHP, $newAudienceMismatch is set to true when there is no filter. This is consistent with the initial-assignment path at line 313, so the cache-invalidation comparison remains coherent. However, adding a brief inline comment here would make the contract explicit for future readers.
Additionally, line 261 mutates $assignment->attrsSeq as a side effect of what looks like a pure boolean query. The mutation is intentional for cache coherence but is a mild code smell — consider extracting it into the call site in getAssignment to keep this method free of side effects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Context/Context.php` around lines 250 - 265, audienceMatches currently
treats a null from AudienceMatcher::evaluate() as a mismatch due to using
!$result and also mutates $assignment->attrsSeq inside what appears to be a
boolean check; add a brief inline comment in audienceMatches by the evaluate()
call clarifying that evaluate() returns ?bool and null is intentionally treated
as a mismatch here (i.e. "null => no filter => treat as mismatch for cache
invalidation"), and remove the side-effect by moving the assignment of
$assignment->attrsSeq = $this->attrsSeq out of audienceMatches into the
getAssignment call site (call audienceMatches to decide match, then update
$assignment->attrsSeq in getAssignment when appropriate) so audienceMatches
remains a pure predicate while preserving cache-coherence via getAssignment.
| * at source. | ||
| */ | ||
| $matches = preg_match('/'. trim($pattern, '/') . '/', $text); | ||
| $pattern = trim($pattern, '/'); |
There was a problem hiding this comment.
trim($pattern, '/') discards PCRE flags from slash-delimited patterns.
trim('/foo/i', '/') yields foo/i (only the leading / is stripped because the last character is i, not /). The resulting regex ~foo/i~ matches the four-character literal foo/i rather than applying the i (case-insensitive) flag. If callers can supply patterns in the conventional PHP/PCRE form /pattern/flags, the flags must be split off before re-wrapping with the new delimiter.
🛡️ Proposed fix — separate pattern body from flags before re-wrapping
private function runRegexBounded(string $text, string $pattern): ?bool {
- $pattern = trim($pattern, '/');
-
- $matches = `@preg_match`('~'. $pattern . '~', $text);
+ // Strip a single leading/trailing slash delimiter and capture any trailing flags.
+ $flags = '';
+ if (preg_match('/^\\/(.+)\\/([imsuxADSUXJ]*)$/', $pattern, $parts) === 1) {
+ $pattern = $parts[1];
+ $flags = $parts[2];
+ } else {
+ $pattern = trim($pattern, '/');
+ }
+
+ $safePattern = str_replace('~', '\~', $pattern);
+ $matches = `@preg_match`('~' . $safePattern . '~' . $flags, $text);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/JsonExpression/Operator/MatchOperator.php` at line 28, The current
trim($pattern, '/') call can leave trailing PCRE flags attached to the body
(e.g. '/foo/i' -> 'foo/i'), so update the handling in MatchOperator to detect
slash-delimited patterns and split the pattern body and flags before
re-wrapping: if $pattern starts with '/' find the last '/' position, take the
substring between first and last slash as the body and the substring after the
last slash as flags, escape any new delimiter characters in the body, then build
the final regex using the chosen delimiter (e.g. '~') and append the flags;
apply this logic where $pattern is processed in the MatchOperator class to
ensure flags are preserved correctly.
| $pattern = trim($pattern, '/'); | ||
|
|
||
| $matches = @preg_match('~'. $pattern . '~', $text); |
There was a problem hiding this comment.
Patterns containing ~ will silently fail to evaluate correctly.
Using ~ as the PCRE delimiter while concatenating the raw user-supplied pattern string means any pattern that itself contains ~ (e.g. foo~bar) produces a malformed regex string like ~foo~bar~, which PCRE interprets as delimiter ~, pattern foo, flags bar~ — an invalid flag sequence. The @-suppression + preg_last_error() guard correctly returns null rather than crashing, but valid patterns with ~ are permanently broken. Consider escaping ~ in the pattern or choosing a delimiter that is less likely to collide (e.g. using preg_quote with the chosen delimiter, or selecting a non-printable delimiter).
🛡️ Proposed fix — escape the chosen delimiter before wrapping
private function runRegexBounded(string $text, string $pattern): ?bool {
- $pattern = trim($pattern, '/');
-
- $matches = `@preg_match`('~'. $pattern . '~', $text);
+ $pattern = trim($pattern, '/');
+ // Escape the tilde delimiter so user-supplied patterns containing `~`
+ // do not break the PCRE boundary.
+ $safePattern = str_replace('~', '\~', $pattern);
+
+ $matches = `@preg_match`('~' . $safePattern . '~', $text);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/JsonExpression/Operator/MatchOperator.php` around lines 28 - 30, The
current MatchOperator implementation trims slashes then builds a regex using '~'
as the delimiter and calls preg_match('@preg_match(\'~'. $pattern . '~\',
$text)@'), which breaks when the user pattern contains '~'; change this to
escape occurrences of the chosen delimiter inside $pattern (or call
preg_quote($pattern, '~')) before concatenation so embedded '~' do not produce
malformed delimiters — update the code that sets $pattern (and the preg_match
invocation in MatchOperator.php) to use the escaped pattern (or a
preg_quote-based variant) when wrapping with the '~' delimiters.
| 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 assertion on line 33 — likely a copy/paste error.
Line 33 is identical to line 30 ([false, []]). This was probably meant to cover the integer 0 case, which is particularly worth testing given the loose-comparison behaviour of in_array.
🐛 Proposed fix
- self::assertFalse($this->operator->evaluate($this->evaluator, [false, []]));
+ self::assertFalse($this->operator->evaluate($this->evaluator, [0, []]));📝 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.
| 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, []])); | |
| } | |
| 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, [0, []])); | |
| 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
testReturnFalseOnEmptyArray replace the duplicated assertion that calls
$this->operator->evaluate($this->evaluator, [false, []]) with an assertion that
checks the integer 0 case (e.g. invoke
$this->operator->evaluate($this->evaluator, [0, []])) so the test covers the
edge-case of loose-comparison/in_array behavior; keep the other assertions
(false, "1", true, null) and ensure you still assertFalse for the 0 invocation
to match the test intent.
| public function testArrayDiffNull(): void { | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[1, 2, 3], [2, 3]])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[1, 2, 3], [5, 6]])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]])); | ||
| self::assertFalse($this->operator->evaluate($this->evaluator, [[5, 6], [1, 2, 3]])); | ||
| } |
There was a problem hiding this comment.
Test name testArrayDiffNull is misleading — assertions use assertFalse, not assertNull.
The name implies the operator returns null for array-in-array mismatches, but the current implementation returns false via in_array. Renaming it avoids confusion for future contributors.
📝 Suggested rename
- public function testArrayDiffNull(): void {
+ public function testArrayNotContainedInArray(): void {📝 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.
| public function testArrayDiffNull(): void { | |
| self::assertFalse($this->operator->evaluate($this->evaluator, [[1, 2, 3], [2, 3]])); | |
| self::assertFalse($this->operator->evaluate($this->evaluator, [[1, 2, 3], [5, 6]])); | |
| self::assertFalse($this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]])); | |
| self::assertFalse($this->operator->evaluate($this->evaluator, [[5, 6], [1, 2, 3]])); | |
| } | |
| public function testArrayNotContainedInArray(): void { | |
| self::assertFalse($this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]])); | |
| self::assertFalse($this->operator->evaluate($this->evaluator, [[5, 6], [1, 2, 3]])); | |
| } |
🤖 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 misleading test method testArrayDiffNull to reflect it asserts false
(e.g., testArrayDiffFalse or testArrayNotInArray) and update any references to
that method; the assertions call $this->operator->evaluate($this->evaluator,
[[2, 3], [1, 2, 3]]) and [[5, 6], [1, 2, 3]] and expect false, so only the test
method name needs to change (ensure any test annotations or usages are updated
to the new name).
1 participant
Summary
Test Results
Changes
src/Assignment.php- Default property initialization for PHP 8.4src/Context/Context.php- Fix exposure queuing, traffic seed, publish flush, finalize error messagesrc/Context/ContextEventLoggerEvent.php- Add missing event typesrc/Experiment.php- Fix experiment data parsingsrc/Http/HTTPClient.php- Fix class namingsrc/JsonExpression/Operator/InOperator.php- Fix argument order (lhs=needle, rhs=haystack), loose comparisonsrc/JsonExpression/Operator/MatchOperator.php- Use ~ as regex delimiter instead of /tests/Context/ContextTest.php- Major test expansion (169 -> 317 tests)tests/JsonExpression/Operator/InOperatorTest.php- Updated for correct argument ordertests/AudienceMatcherTest.php- New teststests/Client/ClientConfigTest.php- New teststests/MD5Test.php- New teststests/Murmur3Test.php- New teststests/VariantAssignerTest.php- Updated teststests/Fixtures/json/- New test fixture dataTest plan
Summary by CodeRabbit
New Features
API Updates
Tests