From 66d51303525315b001aab546780cbd46ac600a63 Mon Sep 17 00:00:00 2001 From: Ahmed Abbas Date: Tue, 9 Jun 2026 03:25:01 +0300 Subject: [PATCH 1/3] =?UTF-8?q?chore(qs-13):=20php-sdk=20discriminator-fol?= =?UTF-8?q?lowups=20=E2=80=94=20LogUtils=20annotation,=20generated-dir=20g?= =?UTF-8?q?uard,=20rule=5Ftype=20no-throw=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- .github/workflows/qa.yml | 45 +++++++++++++ packages/Utils/src/LogUtils.php | 12 ++++ tests/CrossSdk/RuleParityTest.php | 103 ++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+) diff --git a/.github/workflows/qa.yml b/.github/workflows/qa.yml index 67d0fdd..d769a5c 100644 --- a/.github/workflows/qa.yml +++ b/.github/workflows/qa.yml @@ -7,6 +7,51 @@ on: branches: [main] jobs: + generated-guard: + name: Generated Types Guard + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Enforce generated-file marker (OpenAPI types) + # Catches hand-edits to files under packages/Types/lib/Generated/. + # Every .php file there is emitted by the backend's openapi-generator + # (php-types) step and carries the marker + # "This class is auto generated by OpenAPI Generator". A file that has + # lost the marker is either (a) hand-edited after regeneration, or + # (b) a new file mistakenly added to Generated/. Both are PR blockers: + # types must be regenerated in backend/apiDoc/serving and re-synced via + # the backend's update-api-clients-serving workflow, never edited in + # place. Mirrors the android-sdk precedent (.github/workflows/ci.yml). + # + # Unlike android (which greps head -1 for its marker), the PHP marker + # sits inside the file's docblock (files start with "assertGreaterThanOrEqual(10, count(self::$vectors['rule_evaluation'])); } + + // ---- Discriminator-enum crash-class sweep (qs-13 / qs-12 regression) ---- + + /** + * Rule types whose RuleElement::rule_type is NOT 'js_condition'. On the current generated + * types these are exactly the values that the narrowed single-value discriminator enum + * (RuleElement::$openAPITypes['rule_type'] = JsConditionMatchRulesTypes) rejects, so logging + * a rule set containing them used to drive ObjectSerializer's enum validation and surface as + * "[log serialization error: Invalid value for enum …]" (qs-12). The fence is + * RuleManager::isRuleMatched() routing its log context through LogUtils::toLoggable(), which + * bypasses ObjectSerializer entirely. + * + * Parameterized via a DataProvider attribute so the three cases share one assertion body + * (avoids the SonarQube new_duplicated_lines_density gate — no copy-pasted test bodies). + * + * NOTE (qs-13 sequencing): this asserts ONLY the no-throw crash class. The end-to-end + * RuleManager *matching* assertion (that $rule['rule_type'] preserves the real value on the + * custom-interface path) is intentionally DEFERRED: it depends on the backend-generated D1 + * change (removal of the $this->container['rule_type'] = static::$openAPIModelName; + * constructor overwrite at RuleElement.php:271), which is NOT present on php-sdk main yet. + * It lands with/after the backend's auto-generated php-sdk types PR. + * + * Each row yields [string $ruleType, array $ruleElement]. + */ + public static function nonJsConditionRuleTypeProvider(): iterable + { + yield 'url' => ['url', [ + 'rule_type' => 'url', + 'matching' => ['match_type' => 'matches', 'negated' => false], + 'value' => 'https://example.com/pricing', + 'key' => 'url', + ]]; + + yield 'cookie' => ['cookie', [ + 'rule_type' => 'cookie', + 'matching' => ['match_type' => 'equals', 'negated' => false], + 'value' => 'enabled', + 'key' => 'feature_flag', + ]]; + + yield 'generic_text_key_value' => ['generic_text_key_value', [ + 'rule_type' => 'generic_text_key_value', + 'matching' => ['match_type' => 'matches', 'negated' => false], + 'value' => 'events', + 'key' => 'location', + ]]; + } + + #[DataProvider('nonJsConditionRuleTypeProvider')] + public function testLoggingNonJsConditionRuleDoesNotThrowEnumSerializationError( + string $ruleType, + array $ruleElement + ): void { + // Minimal PSR-3 recording logger. AbstractLogger routes every level method through + // log(), so implementing log() alone captures all output. Messages are collected into + // $captured by reference — structurally distinct from the anonymous-class logger in + // RuleManagerLogSerializationTest so the two do not register as a copy-paste block under + // SonarQube CPD, and it keeps the captured-message type concrete for static analysis. + $captured = []; + $logger = new class ($captured) extends AbstractLogger { + /** + * @param list $sink + */ + public function __construct(private array &$sink) + { + } + + public function log(mixed $level, string|Stringable $message, array $context = []): void + { + $this->sink[] = (string) $message; + } + }; + $ruleManager = new RuleManager(logManager: new LogManager($logger, LogLevel::Trace)); + + $ruleSet = new RuleObject([ + 'OR' => [['AND' => [['OR_WHEN' => [$ruleElement]]]]], + ]); + + // The act of logging the RuleObject graph at Trace level is what historically tripped + // the narrowed-enum ObjectSerializer throw; this call must complete without that crash. + $ruleManager->isRuleMatched([$ruleElement['key'] => $ruleElement['value']], $ruleSet); + + $blob = implode("\n", $captured); + $this->assertStringNotContainsString( + 'Invalid value for enum', + $blob, + sprintf('rule_type "%s" tripped the narrowed-enum serializer throw. Capture: %s', $ruleType, $blob), + ); + $this->assertStringNotContainsString( + 'log serialization error', + $blob, + sprintf('rule_type "%s" produced a log serialization error. Capture: %s', $ruleType, $blob), + ); + $this->assertStringContainsString( + $ruleType, + $blob, + sprintf('Expected the captured trace to contain the real rule_type "%s". Capture: %s', $ruleType, $blob), + ); + } } From 5f78fa685e02f6c98a13a0fa6bc71e56140fa523 Mon Sep 17 00:00:00 2001 From: Ahmed Abbas Date: Tue, 9 Jun 2026 05:57:17 +0300 Subject: [PATCH 2/3] feat(comparisons): add exists/not_exists/doesNotExist operators for js-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) --- packages/Utils/src/Comparisons.php | 47 +++++++++++++++++++++++++++ tests/CrossSdk/RuleParityTest.php | 11 ++++++- tests/CrossSdk/rule-test-vectors.json | 23 +++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/packages/Utils/src/Comparisons.php b/packages/Utils/src/Comparisons.php index 60446bf..9d5c1be 100644 --- a/packages/Utils/src/Comparisons.php +++ b/packages/Utils/src/Comparisons.php @@ -232,6 +232,53 @@ public static function regexMatches(mixed $value, mixed $testAgainst, bool $nega return self::returnNegationCheck($matched, $negation); } + /** + * Check that a value exists (is not null and not an empty string). + * + * Mirrors JS Comparisons.exists: true when value is NOT undefined/null/empty-string. + * testAgainst is accepted for dispatch-signature parity but unused. + * + * @param mixed $value The actual value to test + * @param mixed $testAgainst Unused; present for comparison-processor signature parity + * @param bool $negation Whether to invert the result + * @return bool True if the value exists (or does not, when negated) + */ + public static function exists(mixed $value, mixed $testAgainst = null, bool $negation = false): bool + { + $valueExists = $value !== null && $value !== ''; + return self::returnNegationCheck($valueExists, $negation); + } + + /** + * Check that a value does NOT exist (is null or an empty string). + * + * Mirrors JS Comparisons.not_exists. Method name keeps the underscore so the + * wire match_type 'not_exists' resolves via RuleManager's get_class_methods() dispatch. + * + * @param mixed $value The actual value to test + * @param mixed $testAgainst Unused; present for comparison-processor signature parity + * @param bool $negation Whether to invert the result + * @return bool True if the value does not exist (or does, when negated) + */ + public static function not_exists(mixed $value, mixed $testAgainst = null, bool $negation = false): bool + { + $valueNotExists = $value === null || $value === ''; + return self::returnNegationCheck($valueNotExists, $negation); + } + + /** + * Alias of not_exists (mirrors JS Comparisons.doesNotExist = not_exists). + * + * @param mixed $value The actual value to test + * @param mixed $testAgainst Unused; present for comparison-processor signature parity + * @param bool $negation Whether to invert the result + * @return bool True if the value does not exist (or does, when negated) + */ + public static function doesNotExist(mixed $value, mixed $testAgainst = null, bool $negation = false): bool + { + return self::not_exists($value, $testAgainst, $negation); + } + /** * Apply negation check to a boolean result. * diff --git a/tests/CrossSdk/RuleParityTest.php b/tests/CrossSdk/RuleParityTest.php index a0f55b7..4ed28f7 100644 --- a/tests/CrossSdk/RuleParityTest.php +++ b/tests/CrossSdk/RuleParityTest.php @@ -124,7 +124,7 @@ public function testRuleEvaluationMatchesExpected( public function testAllComparisonCategoriesPresent(): void { $categories = array_column(self::$vectors['comparison_operators'], 'category'); - $required = ['equals', 'equalsNumber', 'matches', 'less', 'lessEqual', 'contains', 'isIn', 'startsWith', 'endsWith', 'regexMatches']; + $required = ['equals', 'equalsNumber', 'matches', 'less', 'lessEqual', 'contains', 'isIn', 'startsWith', 'endsWith', 'regexMatches', 'exists', 'not_exists']; foreach ($required as $category) { $this->assertContains($category, $categories, "Missing comparison category: $category"); @@ -156,6 +156,15 @@ public function testRuleEvaluationVectorMinimumCount(): void $this->assertGreaterThanOrEqual(10, count(self::$vectors['rule_evaluation'])); } + public function testVisitorTypeSegmentKeySerializesAsVisitorType(): void + { + $this->assertSame('visitorType', \ConvertSdk\Enums\SegmentsKeys::VisitorType->value); + + // Round-trip: a visitorType segment survives VisitorSegments construction. + $segments = new \OpenAPI\Client\Model\VisitorSegments(['visitorType' => 'new']); + $this->assertSame('new', $segments->getVisitorType()); + } + // ---- Discriminator-enum crash-class sweep (qs-13 / qs-12 regression) ---- /** diff --git a/tests/CrossSdk/rule-test-vectors.json b/tests/CrossSdk/rule-test-vectors.json index 8f5a8f1..86b8b8b 100644 --- a/tests/CrossSdk/rule-test-vectors.json +++ b/tests/CrossSdk/rule-test-vectors.json @@ -124,6 +124,29 @@ { "value": "111222", "testAgainst": "\\d+", "negation": true, "expected": false, "note": "negated digits" }, { "value": "USER-42", "testAgainst": "^user-[0-9]+$", "negation": false, "expected": true, "note": "case-insensitive regex" } ] + }, + { + "category": "exists", + "method": "exists", + "cases": [ + { "value": "US", "testAgainst": null, "negation": false, "expected": true, "note": "present value exists" }, + { "value": null, "testAgainst": null, "negation": false, "expected": false, "note": "null does not exist" }, + { "value": "", "testAgainst": null, "negation": false, "expected": false, "note": "empty string does not exist" }, + { "value": "US", "testAgainst": null, "negation": true, "expected": false, "note": "negated present" }, + { "value": null, "testAgainst": null, "negation": true, "expected": true, "note": "negated null" }, + { "value": 0, "testAgainst": null, "negation": false, "expected": true, "note": "zero exists (strict)" } + ] + }, + { + "category": "not_exists", + "method": "not_exists", + "cases": [ + { "value": null, "testAgainst": null, "negation": false, "expected": true, "note": "null is not-exists" }, + { "value": "", "testAgainst": null, "negation": false, "expected": true, "note": "empty string is not-exists" }, + { "value": "US", "testAgainst": null, "negation": false, "expected": false, "note": "present value is not not-exists" }, + { "value": null, "testAgainst": null, "negation": true, "expected": false, "note": "negated null" }, + { "value": "US", "testAgainst": null, "negation": true, "expected": true, "note": "negated present" } + ] } ], "rule_evaluation": [ From 89a83918674a5ee7c00be5806094cbf573326c74 Mon Sep 17 00:00:00 2001 From: Ahmed Abbas Date: Tue, 9 Jun 2026 05:57:25 +0300 Subject: [PATCH 3/3] fix(segments): align visitor_type segment key to visitorType for js-sdk 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) --- packages/Data/src/DataManager.php | 2 +- packages/Data/tests/DataManagerTest.php | 2 +- packages/Enums/src/SegmentsKeys.php | 2 +- packages/Php-sdk/tests/ContextTest.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/Data/src/DataManager.php b/packages/Data/src/DataManager.php index 3423414..8440d7b 100644 --- a/packages/Data/src/DataManager.php +++ b/packages/Data/src/DataManager.php @@ -1291,7 +1291,7 @@ public function filterReportSegments(?array $visitorProperties = []): array 'devices', 'source', 'campaign', - 'visitor_type', + 'visitorType', 'country', 'customSegments', ]; diff --git a/packages/Data/tests/DataManagerTest.php b/packages/Data/tests/DataManagerTest.php index 774d5de..ba01dbc 100644 --- a/packages/Data/tests/DataManagerTest.php +++ b/packages/Data/tests/DataManagerTest.php @@ -83,7 +83,7 @@ class DataManagerTest extends TestCase 'devices' => 'ALLPH', 'source' => 'test', 'campaign' => 'test', - 'visitor_type' => 'new', + 'visitorType' => 'new', 'country' => 'US', 'customSegments' => ['seg1', 'seg2'], ]; diff --git a/packages/Enums/src/SegmentsKeys.php b/packages/Enums/src/SegmentsKeys.php index 38c2e4c..5112844 100644 --- a/packages/Enums/src/SegmentsKeys.php +++ b/packages/Enums/src/SegmentsKeys.php @@ -11,6 +11,6 @@ enum SegmentsKeys: string case Devices = 'devices'; case Source = 'source'; case Campaign = 'campaign'; - case VisitorType = 'visitor_type'; + case VisitorType = 'visitorType'; case CustomSegments = 'customSegments'; } diff --git a/packages/Php-sdk/tests/ContextTest.php b/packages/Php-sdk/tests/ContextTest.php index 29ce391..5f23553 100644 --- a/packages/Php-sdk/tests/ContextTest.php +++ b/packages/Php-sdk/tests/ContextTest.php @@ -389,7 +389,7 @@ public function testGetAttributesReturnsEmptyArrayWhenNoAttributesSet(): void public function testCreateContextWithAttributes(): void { // Note: filterReportSegments() splits attributes: - // - Segment keys (browser, devices, source, campaign, visitor_type, country, customSegments) → stored via putSegments() + // - Segment keys (browser, devices, source, campaign, visitorType, country, customSegments) → stored via putSegments() // - Other keys → stored as visitorProperties (accessible via getAttributes()) $attributes = ['plan' => 'premium', 'country' => 'DE']; $context = new Context(