chore(qs-13): php-sdk discriminator follow-ups — LogUtils annotation, generated-dir guard, rule_type no-throw sweep - #42
Merged
Conversation
…generated-dir guard, rule_type no-throw sweep
Companion php-sdk follow-ups (a)/(b)/(c) for qs-13 (Option-A root-cause fix
for the RuleElement narrowed-discriminator-enum crash; backend portion lands
on PR #6340). php-sdk scope only — no OpenAPI spec or backend changes here.
(a) LogUtils::toLoggable — annotate as belt-and-braces. STILL load-bearing on
current main (the discriminator bases are still narrowed to single-value
enums); becomes redundant for the crash only once the backend regenerates
those bases to `string`. No logic changed.
(b) qa.yml — add a Generated Types Guard job that fails if any .php file under
packages/Types/lib/Generated/ has lost its OpenAPI generator marker. The
PHP marker ("This class is auto generated by OpenAPI Generator") sits in the
file docblock (files start with <?php), so the grep scans the whole file
(unlike android's head -1) and whitelists ALL generated files incl. Api/,
ObjectSerializer.php, Configuration.php, HeaderSelector.php, ApiException.php.
Verified: passes against the current 267-file Generated/ tree; fails on a
marker-less hand-edit.
(c) RuleParityTest — add a @dataProvider-driven sweep over url / cookie /
generic_text_key_value asserting that logging such a rule via
RuleManager::isRuleMatched() (which routes through LogUtils::toLoggable)
emits neither "Invalid value for enum" nor "log serialization error".
Parameterized to avoid the SonarQube duplication gate.
DEFERRED (not shipped): the D1-dependent end-to-end RuleManager *matching*
assertion. On current main RuleElement.php:271 still overwrites rule_type with
the model name, corrupting the custom-interface dispatch in RuleManager — so a
matching assertion would fail CI. D1 (removal of that overwrite) is a backend-
generated-types change absent from php-sdk main; the matching assertion lands
with/after the backend auto-generated php-sdk types PR.
Tests: vendor/bin/phpunit --testsuite cross-sdk -> 403 passed; qs-12 regression
RuleManagerLogSerializationTest green; phpstan + php-cs-fixer clean on all
touched files.
Refs: qs-13, backend PR #6340
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…s-sdk parity Port the three existence comparison methods from js-sdk comparisons.ts to php-sdk Comparisons.php, matching JS strict-equality semantics (null/empty string => not exists; 0 exists). not_exists keeps the underscore so the wire match_type 'not_exists' resolves via RuleManager get_class_methods() dispatch; doesNotExist delegates to not_exists (faithful equivalent of JS's static alias). Extend the cross-SDK parity surface: add exists/not_exists vector groups (present/null/empty-string/negation/strict-zero) flowing through the existing #[DataProvider] (no copy-pasted test bodies), and add both to the required comparison-category guard. Ref: qs-14 php-jssdk-parity-comparisons-exists-segment-keys Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dk parity Change SegmentsKeys::VisitorType from 'visitor_type' to 'visitorType' to match js-sdk VISITOR_TYPE, and update the hardcoded routing literal in DataManager::filterReportSegments() plus the two test/doc surfaces. Fixes a latent round-trip bug: filterReportSegments() emitted 'visitor_type' while the generated VisitorSegments model keys strictly on 'visitorType', so SegmentsManager::getSegments() silently dropped the visitor-type value. A new parity test locks the enum value and the VisitorSegments round-trip. Ref: qs-14 php-jssdk-parity-comparisons-exists-segment-keys Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DmytroConvert
previously approved these changes
Jun 9, 2026
…ts-segment-keys-qs14 feat(parity): add exists/not_exists comparison operators + align visitor_type segment key
DmytroConvert
approved these changes
Jun 9, 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
2 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.
Summary
Companion php-sdk follow-ups (a)/(b)/(c) for qs-13 — the Option-A root-cause fix for the
RuleElementnarrowed-discriminator-enum crash (qs-12). This PR is php-sdk scope only: no OpenAPI spec edits and no backend changes. The backend portion (thefix-php-discriminants.jsdist-php post-process,package.json/openapitools.jsonwiring, the build-time assertion, and workflow gating) lands separately on backend PR #6340.(a)
packages/Utils/src/LogUtils.php— comment onlyAnnotated
toLoggable()as belt-and-braces. It is still load-bearing on currentmain: the discriminator bases (RuleElement::rule_type,RuleElementNoUrl::rule_type) are still narrowed to single-value enums, so bypassingObjectSerializerhere is what prevents theInvalid value for enumcrash when logging real-worldrule_typevalues. It becomes redundant for the crash only once the backend regenerates those bases tostring(PR #6340). It is retained as belt-and-braces thereafter. No logic changed.(b)
.github/workflows/qa.yml— generated-dir guardNew
generated-guardjob that fails if any.phpfile underpackages/Types/lib/Generated/has lost its OpenAPI generator marker (i.e. was hand-edited or wrongly added). Mirrors the android-sdk precedent (.github/workflows/ci.yml), with the PHP-specific differences:"This class is auto generated by OpenAPI Generator"sits inside the file docblock (files start with<?php), so the guard scans the whole file (nothead -1like android).Model/and the non-Model/infra (Api/,ObjectSerializer.php,Configuration.php,HeaderSelector.php,ApiException.php).Verified locally: passes against the current 267-file
Generated/tree; fails when a marker-less file is introduced.(c)
tests/CrossSdk/RuleParityTest.php—rule_typeno-throw sweep (re-scoped)Added a
DataProvider-driven sweep overurl/cookie/generic_text_key_value(non-js_condition) asserting that logging such a rule viaRuleManager::isRuleMatched()— which routes its log context throughLogUtils::toLoggable()— emits neitherInvalid value for enumnorlog serialization error. Modeled on the greenRuleManagerLogSerializationTestlogging path, parameterized via a single data provider to avoid the SonarQubenew_duplicated_lines_densitygate (no copy-pasted test bodies).Deferred (intentionally NOT in this PR): the D1 matching assertion
The spec's "with D1 taken, ALSO assert RuleManager end-to-end matching on these types (
$rule['rule_type']preserves the real value)" assertion is deferred — it cannot pass on currentmain:RuleElement.php:271still runs$this->container['rule_type'] = static::$openAPIModelName;(="RuleElement"), overwriting the input value (D1).RuleManager.php:235buildsnew RuleElement($ruleItem), and the custom-interface path (:268–285) dispatches on$rule['rule_type']— which the overwrite corrupts.url/cookievia the custom interface would fail CI on currentmain.D1 (removal of that constructor overwrite) is a backend-generated-types change that is not present on php-sdk
main. Per AC2, (c) must merge tomainbefore the backend regenerates the types; at that point D1 is absent. The matching assertion lands with/after the backend's auto-generated php-sdk types PR — it is explicitly not part of this PR.Tests run
vendor/bin/phpunit --testsuite cross-sdk→ OK (403 tests, 534 assertions) (incl. the 3 new sweep cases)vendor/bin/phpunit --filter RuleManagerLogSerializationTest(qs-12 regression, AC2 must-stay-green) → OK (1 test, 9 assertions)vendor/bin/phpstan analyseon touched files → No errorsvendor/bin/php-cs-fixer fix --dry-runon touched files → cleanRefs
qs-13(_bmad-output/implementation-artifacts/2026-03-13-convert-php-sdk/qs-13-php-serving-types-discriminator-postprocess.md)api-docs/auttomate-php-config-types)🤖 Generated with Claude Code