You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "sveneld (via GitHub)" <gi...@apache.org> on 2026/09/02 16:43:45 UTC
[PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
sveneld opened a new pull request, #3787:
URL: https://github.com/apache/thrift/pull/3787
## Summary
- accept scalar PHP sets passed as sequential lists without breaking legacy keyed-set input
- apply the compatibility fix in both generated PHP serialization and the shared runtime helpers
- add regression coverage for generated JSON serialization plus TBase and TException round-trips
## Testing
- targeted PHP unit and integration tests in Docker for TBase, TException, TJSONProtocol, TProtocolInlined, JsonSerialize, Validator, and ThriftClassLoader
Generated-by: OpenAI Codex GPT-5 <no...@openai.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3787:
URL: https://github.com/apache/thrift/pull/3787#discussion_r3917302605
##########
compiler/cpp/src/thrift/generate/t_php_generator.cc:
##########
@@ -2681,13 +2681,18 @@ void t_php_generator::generate_serialize_container(ostream& out, t_type* ttype,
} else if (ttype->is_set()) {
string iter = tmp("iter");
string iter_val = tmp("iter");
- indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n';
- indent_up();
-
t_type* elem_type = ((t_set*)ttype)->get_elem_type();
- if(php_is_scalar(elem_type)) {
- generate_serialize_set_element(out, (t_set*)ttype, iter);
+ if (php_is_scalar(elem_type)) {
+ string is_list = tmp("isList");
+ string iter_elem = tmp("iter");
+ indent(out) << "$" << is_list << " = array_is_list($" << prefix << ");" << '\n';
+ indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n';
+ indent_up();
+ indent(out) << "$" << iter_elem << " = $" << is_list << " ? $" << iter_val << " : $" << iter << ";" << '\n';
+ generate_serialize_set_element(out, (t_set*)ttype, iter_elem);
Review Comment:
The generated scalar-set serializer uses `array_is_list($set)` to decide whether to serialize keys vs values. That breaks legacy keyed-set input for sets whose elements are sequential (e.g. `{0,1}` represented as `[0 => true, 1 => true]`), because `array_is_list()` is still true and the code will serialize `true` values. Consider treating a list-like array as a value-list only when at least one value is not `true` (the legacy keyed-set marker).
##########
lib/php/lib/Base/TBase.php:
##########
@@ -349,8 +349,9 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = $set && array_is_list($var);
Review Comment:
`array_is_list($var)` is not a safe discriminator between a legacy keyed-set (element=>true) and a value-list when the set’s elements happen to be 0..n-1 (e.g. set<i32> {0,1} is represented as `[0 => true, 1 => true]`, which is still a list per `array_is_list`). In that case this change would serialize the *values* (`true`) rather than the keys, breaking legacy keyed-set input.
##########
lib/php/lib/Exception/TException.php:
##########
@@ -348,8 +348,9 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = $set && array_is_list($var);
Review Comment:
Same issue as in TBase::writeList(): `array_is_list($var)` will return true for legacy keyed-set input when the element keys are sequential (0..n-1), causing the serializer to write `true` values instead of the element keys. This breaks backward compatibility for sets containing 0/1/etc.
##########
lib/php/test/Unit/Lib/Base/TBaseTest.php:
##########
@@ -74,6 +74,20 @@ public function testReadAndWriteRoundTripNestedContainers(): void
$this->assertNull($restored->optionalField);
}
+ public function testWriteSetAcceptsSequentialValues(): void
+ {
+ $struct = new ComplexStruct(
+ ComplexStruct::$tspec,
+ [
+ 'setField' => [10, 20],
+ ]
+ );
+
+ $restored = $this->roundTrip($struct);
+
+ $this->assertSame([10 => true, 20 => true], $restored->setField);
+ }
+
Review Comment:
This test covers the new sequential-values input, but it doesn’t guard the stated compatibility requirement for legacy keyed sets where elements are sequential (e.g. setField `{0,1}` represented as `[0 => true, 1 => true]`). Adding a regression case here would catch the `array_is_list()` misclassification scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3787:
URL: https://github.com/apache/thrift/pull/3787#discussion_r3918106342
##########
compiler/cpp/src/thrift/generate/t_php_generator.cc:
##########
@@ -2681,13 +2681,30 @@ void t_php_generator::generate_serialize_container(ostream& out, t_type* ttype,
} else if (ttype->is_set()) {
string iter = tmp("iter");
string iter_val = tmp("iter");
- indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n';
- indent_up();
-
t_type* elem_type = ((t_set*)ttype)->get_elem_type();
- if(php_is_scalar(elem_type)) {
- generate_serialize_set_element(out, (t_set*)ttype, iter);
+ if (php_is_scalar(elem_type)) {
+ string set_uses_values = tmp("setUsesValues");
+ string list_val = tmp("iter");
+ string iter_elem = tmp("iter");
+ indent(out) << "$" << set_uses_values << " = false;" << '\n';
+ indent(out) << "if (array_is_list($" << prefix << ")) {" << '\n';
+ indent_up();
+ indent(out) << "foreach ($" << prefix << " as $" << list_val << ") {" << '\n';
+ indent_up();
+ indent(out) << "if ($" << list_val << " !== true) {" << '\n';
+ indent_up();
+ indent(out) << "$" << set_uses_values << " = true;" << '\n';
+ indent(out) << "break;" << '\n';
+ scope_down(out);
+ scope_down(out);
+ scope_down(out);
+ indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n';
+ indent_up();
+ indent(out) << "$" << iter_elem << " = $" << set_uses_values << " ? $" << iter_val << " : $" << iter << ";" << '\n';
+ generate_serialize_set_element(out, (t_set*)ttype, iter_elem);
Review Comment:
The generated scalar-set serialization uses the same `array_is_list` + “any element !== true” heuristic. For `set<bool>`, a sequential values input like `[true]` will serialize the key index (`0`) rather than the value (`true`), so boolean scalar sets don’t fully benefit from the “accept sequential list” compatibility change. Please clarify intended behavior (likely via a generator/runtime comment + regression test) to avoid surprising inconsistencies.
##########
lib/php/lib/Exception/TException.php:
##########
@@ -348,8 +348,17 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = false;
+ if ($set && array_is_list($var)) {
+ foreach ($var as $candidate) {
+ if ($candidate !== true) {
+ $setUsesValues = true;
+ break;
+ }
+ }
+ }
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
Review Comment:
Same `set` list-vs-legacy heuristic as in `TBase::writeList`: for `set<bool>`, a sequential values input like `[true]` is interpreted as legacy marker form (all values are `true`), causing serialization of key `0` (`false`) instead of the intended `true`. Please clarify intended semantics for `set<bool>` and add a regression test so both `TException` and `TBase` stay consistent.
##########
lib/php/lib/Base/TBase.php:
##########
@@ -349,8 +349,17 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = false;
+ if ($set && array_is_list($var)) {
+ foreach ($var as $candidate) {
+ if ($candidate !== true) {
+ $setUsesValues = true;
+ break;
+ }
+ }
+ }
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
Review Comment:
The list-vs-legacy detection for sets (`array_is_list` + “any value !== true”) has an ambiguous/incorrect case for `set<bool>`: a sequential values input like `[true]` will be treated as the legacy “key => true marker” form (because all values are `true`), so the element serialized becomes the key `0` (i.e., `false`) rather than `true`. This means the PR’s stated “accept scalar sets provided as sequential lists” does not fully hold for boolean sets unless callers use the keyed form `[1 => true]`. Please decide and document the intended behavior for `set<bool>` (and add a regression test to lock it in).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3787:
URL: https://github.com/apache/thrift/pull/3787#discussion_r3986957459
##########
compiler/cpp/src/thrift/generate/t_php_generator.cc:
##########
@@ -2681,13 +2681,39 @@ void t_php_generator::generate_serialize_container(ostream& out, t_type* ttype,
} else if (ttype->is_set()) {
string iter = tmp("iter");
string iter_val = tmp("iter");
- indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n';
- indent_up();
-
t_type* elem_type = ((t_set*)ttype)->get_elem_type();
- if(php_is_scalar(elem_type)) {
- generate_serialize_set_element(out, (t_set*)ttype, iter);
+ if (php_is_scalar(elem_type)) {
+ string set_uses_values = tmp("setUsesValues");
+ // Preserve the legacy `element => true` marker form when every value is
+ // `true`. This keeps ambiguous `set<bool>` inputs such as `[true]` on
+ // the backward-compatible path instead of guessing they are value lists.
+ string list_val = tmp("iter");
+ string iter_elem = tmp("iter");
+ indent(out) << "$" << set_uses_values << " = false;" << '\n';
+ indent(out) << "if (array_is_list($" << prefix << ")) {" << '\n';
+ indent_up();
+ indent(out) << "foreach ($" << prefix << " as $" << list_val << ") {" << '\n';
+ indent_up();
+ indent(out) << "if ($" << list_val << " !== true) {" << '\n';
+ indent_up();
+ indent(out) << "$" << set_uses_values << " = true;" << '\n';
+ indent(out) << "break;" << '\n';
+ scope_down(out);
+ scope_down(out);
+ scope_down(out);
+ indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n';
+ indent_up();
+ indent(out) << "$" << iter_elem << " = $" << set_uses_values << " ? $" << iter_val << " : $" << iter << ";" << '\n';
+ if (elem_type->is_bool()) {
+ indent(out) << "if (!$" << set_uses_values << ") {" << '\n';
+ indent_up();
+ indent(out) << "$" << iter_elem << " = (bool) $" << iter_elem << ";" << '\n';
Review Comment:
The new generator branch for `set<bool>` is not exercised by the PHP tests: the generated JSON regression covers a string set, while the bool cases only exercise the shared TBase/TException helpers. The bool-specific cast and ambiguous legacy-marker path here can therefore regress without detection; add a generated `set<bool>` fixture/test covering both `[true, false]` and the legacy keyed form.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3787:
URL: https://github.com/apache/thrift/pull/3787#discussion_r3999760343
##########
lib/php/lib/Base/TBase.php:
##########
@@ -349,8 +349,24 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = false;
+ if ($set && array_is_list($var)) {
+ // Preserve the legacy `element => true` marker form when every
+ // value is `true`. This keeps ambiguous `set<bool>` inputs such
+ // as `[true]` on the backward-compatible path.
+
+ foreach ($var as $candidate) {
+ if ($candidate !== true) {
+ $setUsesValues = true;
+ break;
+ }
+ }
+ }
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
+ if ($set && !$setUsesValues && $etype === TType::BOOL) {
+ $elem = (bool) $elem;
Review Comment:
This only recasts legacy keys for `bool`. Under `declare(strict_types=1)`, PHP coerces numeric-string array keys to `int`, so a legacy `set<string>` or `set<uuid>` such as `['123' => true]` reaches `writeString()`/`writeUuid()` as an integer and throws `TypeError`. The generated serializer already recasts every scalar key; mirror those type-specific casts here so the shared helper also preserves legacy keyed sets.
##########
lib/php/lib/Exception/TException.php:
##########
@@ -348,8 +348,24 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = false;
+ if ($set && array_is_list($var)) {
+ // Preserve the legacy `element => true` marker form when every
+ // value is `true`. This keeps ambiguous `set<bool>` inputs such
+ // as `[true]` on the backward-compatible path.
+
+ foreach ($var as $candidate) {
+ if ($candidate !== true) {
+ $setUsesValues = true;
+ break;
+ }
+ }
+ }
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
+ if ($set && !$setUsesValues && $etype === TType::BOOL) {
+ $elem = (bool) $elem;
+ }
Review Comment:
This only recasts legacy keys for `bool`. Under `declare(strict_types=1)`, PHP coerces numeric-string array keys to `int`, so a legacy `set<string>` or `set<uuid>` such as `['123' => true]` reaches `writeString()`/`writeUuid()` as an integer and throws `TypeError`. The generated serializer already recasts every scalar key; mirror those type-specific casts here so the shared helper also preserves legacy keyed sets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3787:
URL: https://github.com/apache/thrift/pull/3787#issuecomment-5846728435
### Code review
Found 2 issues:
1. A set in the legacy keyed form is now written as a value list when its keys happen to be `0..n-1` and its marker values are not literally `true`. Before this PR the keys were always the elements and the marker was never read. I built the compiler and lib/php from master and from this head and did a TBinaryProtocol round trip on PHP 8.3. `array_fill_keys([0, 1], 1)` comes back as {0, 1} on master and as {1} with this PR, and `array_fill_keys([0, 1, 2], 1)` as {0, 1, 2} and {1}. That holds for `--gen php`, `php:oop` and `php:inlined` alike, and the extension applies the same rule. Thrift's own `php:rest` output builds exactly this form: it decodes a set argument with `array_fill_keys(json_decode($s, true), 1)`, so a REST call with `[0,1]` hands the handler `[0 => 1, 1 => 1]`. Sets that Thrift itself deserialized carry `true` and are not affected. Emitting `true` in the REST code, and noting in lib/php/README.md that keyed sets now need `true` markers, would cover the in-tree case
and document the change for other callers.
https://github.com/apache/thrift/blob/09140dae68dd5fc67ee83185a80ab3b4b34457cf/compiler/cpp/src/thrift/generate/t_php_generator.cc#L2684-L2708
https://github.com/apache/thrift/blob/09140dae68dd5fc67ee83185a80ab3b4b34457cf/compiler/cpp/src/thrift/generate/t_php_generator.cc#L1940-L1944
2. For the new value-list form, the generated serializer and `TBase`/`TException::writeList()` handle the elements differently. The generator runs `emit_array_key_recast()` on the element in both branches, while `writeList()` casts only in the legacy branch and runs under `strict_types`. At this head, `--gen php` writes a `set<bool>` given `[1, 0]` as {true, false} and a `set<i16>` given `['10', '20']` as {10, 20}. The same values through `--gen php:oop` throw a `TypeError` in `writeBool()` and in `writeI16()`. The two paths should agree on this. The new comment on `generate_serialize_set_element` explains the cast only for array-key sourced scalars.
https://github.com/apache/thrift/blob/09140dae68dd5fc67ee83185a80ab3b4b34457cf/lib/php/lib/Base/TBase.php#L365-L376
https://github.com/apache/thrift/blob/09140dae68dd5fc67ee83185a80ab3b4b34457cf/compiler/cpp/src/thrift/generate/t_php_generator.cc#L2776-L2782
🤖 Generated with [Claude Code](https://claude.ai/code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "sveneld (via GitHub)" <gi...@apache.org>.
sveneld commented on PR #3787:
URL: https://github.com/apache/thrift/pull/3787#issuecomment-5653586469
Addressed the bool typedef review concern with generated-code coverage and simplification. New fixtures cover set<bool>, set<Flag> (typedef bool), and a chained typedef for sequential values, legacy keyed values, ambiguous [true], and legacy true. These tests also passed before this follow-up: generate_serialize_set_element() already calls emit_array_key_recast(), which resolves typedefs and emits the bool cast. The reported TypeError was therefore not reproducible on the current branch. Removed the redundant bool-only branch instead of adding a second typedef-aware cast. Inspected regenerated PHP: all three fields cast before writeBool(). Targeted unit/integration suite: 177 tests, 336 assertions, exit 0 (2 PHPUnit deprecations); PHPCS and git diff --check pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "sveneld (via GitHub)" <gi...@apache.org>.
sveneld commented on code in PR #3787:
URL: https://github.com/apache/thrift/pull/3787#discussion_r3999882065
##########
lib/php/lib/Base/TBase.php:
##########
@@ -349,8 +349,24 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = false;
+ if ($set && array_is_list($var)) {
+ // Preserve the legacy `element => true` marker form when every
+ // value is `true`. This keeps ambiguous `set<bool>` inputs such
+ // as `[true]` on the backward-compatible path.
+
+ foreach ($var as $candidate) {
+ if ($candidate !== true) {
+ $setUsesValues = true;
+ break;
+ }
+ }
+ }
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
+ if ($set && !$setUsesValues && $etype === TType::BOOL) {
+ $elem = (bool) $elem;
Review Comment:
Addressed in both shared helpers: legacy keys are now cast according to the declared scalar type (bool, integer, double, string/UUID), consistently with the generated serializer. Added a regression test for a numeric-string legacy set key in each helper; both failed with TypeError before the fix and pass afterward.
##########
lib/php/lib/Exception/TException.php:
##########
@@ -348,8 +348,24 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set
} else {
$xfer += $output->writeListBegin($etype, count($var));
}
+ $setUsesValues = false;
+ if ($set && array_is_list($var)) {
+ // Preserve the legacy `element => true` marker form when every
+ // value is `true`. This keeps ambiguous `set<bool>` inputs such
+ // as `[true]` on the backward-compatible path.
+
+ foreach ($var as $candidate) {
+ if ($candidate !== true) {
+ $setUsesValues = true;
+ break;
+ }
+ }
+ }
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
+ if ($set && !$setUsesValues && $etype === TType::BOOL) {
+ $elem = (bool) $elem;
+ }
Review Comment:
Addressed in both shared helpers: legacy keys are now cast according to the declared scalar type (bool, integer, double, string/UUID), consistently with the generated serializer. Added a regression test for a numeric-string legacy set key in each helper; both failed with TypeError before the fix and pass afterward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-2950: Fix PHP scalar set serialization [thrift]
Posted by "sveneld (via GitHub)" <gi...@apache.org>.
sveneld commented on PR #3787:
URL: https://github.com/apache/thrift/pull/3787#issuecomment-5653900394
Fixed the accelerated cross-test failures as well: the native PHP extension still serialized only keys for scalar sets. It now uses the same list-versus-all-true-marker rule as the PHP implementation. Added scalar_sets.phpt comparing native wire output with explicitly written expected Binary messages; before the change it failed for integer, string, and bool lists. Verification: 179 targeted PHP tests / 342 assertions, all 4 extension PHPT tests, and accelerated PHP clients against C++ and Python servers over buffered, framed, and HTTP transports all pass (6 cross-test runs). PHPCS and git diff --check pass. Repository-wide make style remains blocked by stale local Autotools files referencing the removed Swift directory. Previous local cross-test validation covered only the non-accelerated path; this follow-up closes that gap. CI needs to run again for the full matrix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org