Kicking off the PHP SDK - #2
Merged
Merged
Conversation
…tion in LogManager + test cases adjustments
…into chore/project-setup
…, DataManager code refactor
…API models Provides a recursive normalisation helper that bypasses OpenAPI's ObjectSerializer::sanitizeForSerialization path for logging. Uses ModelInterface::attributeMap() + ::getters() directly, avoiding the narrow-enum validation that throws InvalidArgumentException for any rule_type outside the single-value JsConditionMatchRulesTypes enum. Also declares the Utils -> Types package dependency that this helper requires, matching the documented dependency graph (Layer 2: Utils, Logger, Event -> Enums, Types). Refs PR #11 comment discussion_r3120320108 (C6)
Full coverage of LogUtils::toLoggable(): scalars, arrays, DateTimeInterface, OpenAPI ModelInterface (via real RuleElement with narrow-enum mismatch), nested models, Traversable, JsonSerializable non-models, plain objects, and a regression test proving json_encode() on toLoggable() result does not trigger the enum-validation throw.
RuleManager.isRuleMatched() at L115-118 was the primary symptom site reported on PR #11 by @DmytroConvert: the Laravel demo Events page emitted "[log serialization error: Invalid value for enum '\OpenAPI\Client\Model\JsConditionMatchRulesTypes', must be one of: 'js_condition']" for any rule with rule_type other than 'js_condition'. Root cause: LogManager._log() json_encodes the context array, which cascades into OpenAPI models' jsonSerialize() -> ObjectSerializer::sanitizeForSerialization(), which throws on narrow-enum mismatch. Route all four non-scalar context sites (constructor L57, isRuleMatched L115-118, isValidRule L168, processRuleItem L326-329) through LogUtils::toLoggable(). Fixes PR #11 comment discussion_r3120320108 (C6)
… bug Pins the fix with a test that fails on chore/project-setup-2 head and passes after the LogUtils::toLoggable wrapping. Builds a RuleObject with rule_type 'generic_text_key_value', attaches a capturing PSR-3 logger via LogManager, invokes RuleManager.isRuleMatched() and asserts: - No "log serialization error" string appears in captured log messages - The real rule payload (including rule_type 'generic_text_key_value') is present in the captured trace Refs PR #11 comment discussion_r3120320108 (C6)
…ogUtils::toLoggable Sweep across the remaining NEEDS_WRAP log-context sites identified during the PR #11 C6 fix analysis: - DataManager.php: 26 call sites across matchRulesByField, _getBucketingByField, _retrieveBucketing, convert, filterMatchedRecordsWithRule, filterMatchedCustomSegments, selectLocations, getEntitiesList, _getEntityByField, getItemsByIds, getSubItem. Every context payload passed through ($this->_mapper)(...) or json_encode(...) is now normalised first. - ExperienceManager.php: 2 sites (selectVariation L147, selectVariationById L200) where $logData may carry variation arrays or enum instances. - BucketingManager.php: 1 site (constructor L34 $this self-reference) for consistency. - ApiManager.php: 3 sites (enqueue L245 -> VisitorTrackingEvents model directly, releaseQueue L332 HTTP-4xx branch, releaseQueue L379 all-retries-exhausted). No behaviour change beyond log-path safety; pre-existing SAFE sites (pure string messages) remain untouched. Refs PR #11 comment discussion_r3120320108 (C6)
Addresses findings from self-review: - F1 (High): DataManager.php line 619 used the pre-existing-but-broken $this->_mapper([...]) pattern (method-call syntax on a Closure property). Parenthesise to ($this->_mapper)([...]) so the Closure is actually invoked. Every other mapper call site in the file already uses the correct form. - F3 (Medium): Reorder LogUtils branches so JsonSerializable (explicit serialization intent) takes precedence over Traversable (raw iteration). Models still hit the earlier ModelInterface branch; the change affects only non-model objects that declare both. - F4 (Medium): Revert LogUtils::toLoggable($this) at the two constructor log sites (RuleManager, BucketingManager). get_object_vars from a static external method only sees public props; both managers have only private ones, so toLoggable() was returning []. The old pre-fix flow rendered $this as get_class($this) (more informative), so unwrap it — the bug is only about array contexts, not bare object args. - F5 (Low): Capturing logger in RuleManagerLogSerializationTest now records the PSR-3 $context parameter too, with an assertion that it remains []. Pins LogManager's current PSR-3 behaviour so future structural-context changes surface clearly. - F6 (Low): Add test covering Traversable containing an OpenAPI model — closes a gap where the ModelInterface-inside-Traversable recursion was untested. F2 (upper-bound on Types dep constraint) and F7 (repository key casing) declined: both are pre-existing monorepo-wide conventions; changing only this package would create inconsistency worse than the original nit.
fix(logger): normalise log context to avoid OpenAPI enum serialization error (PR #11 C6)
chore/project-setup-2
abbaseya
marked this pull request as ready for review
April 24, 2026 05:39
abbaseya
changed the title
DmytroConvert
previously approved these changes
Apr 24, 2026
Fix/update maine base
…kflow CodeQL does not support PHP and the previous matrix.include was empty, so the workflow produced zero jobs. Run PHPStan against the existing phpstan.neon and upload findings as SARIF to GitHub Code Scanning via github/codeql-action/upload-sarif@v4. The SARIF formatter is installed in CI only (no changes to composer.json / phpstan.neon). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ci(code-scanning): replace empty CodeQL matrix with PHPStan SARIF workflow
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Remove the duplicated Static Analysis job from ci.yml. The code-scanning workflow now both gates PRs and uploads findings to the Security tab: the PHPStan step runs with continue-on-error so the SARIF upload always runs, then a final step fails the workflow if PHPStan reported findings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The workflow now runs code style, monorepo validation, and the test matrix — no longer a generic "CI" umbrella since static analysis moved to the code-scanning workflow. Rename clarifies intent in the Actions tab alongside "Code Scanning (PHP)". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves modify/delete conflict on .github/workflows/codeql.yml by keeping the PHPStan SARIF workflow from chore/project-setup over the deletion from main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
clllaur
approved these changes
Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.