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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
