Copilot commented on code in PR #3791:
URL: https://github.com/apache/thrift/pull/3791#discussion_r3921759210
##########
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 serialization chooses between keys vs values using
only array_is_list($set). That misclassifies legacy keyed-set arrays whose keys
happen to be 0..n-1 (e.g. representing {0} as [0 => true]) as “list”, then
serializes the boolean $iter_val instead of the element key, which can throw
under strict_types or produce wrong output. Make the emitted $isList check
robust by treating the legacy "elem => true" shape as keyed, even if it is a
list.
##########
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);
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
Review Comment:
$setUsesValues uses array_is_list($var) to infer a set is provided as a
value-list. This misclassifies legacy keyed-set arrays with sequential numeric
keys (e.g. [0 => true] for a set containing element 0) as “list”, then
serializes $val (true) instead of the element key, which can throw TypeError
under strict_types or serialize incorrect elements. Prefer treating list-form
as values only when it does not match the legacy "elem => true" shape.
This issue also appears on line 352 of the same file.
##########
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);
foreach ($var as $key => $val) {
- $elem = $set ? $key : $val;
+ $elem = $set && !$setUsesValues ? $key : $val;
Review Comment:
$setUsesValues uses array_is_list($var) to decide whether to serialize a set
from array values. This breaks legacy keyed-set arrays when the element keys
are sequential (e.g. [0 => true]), because they are also “lists” in PHP; the
loop then uses $val (true) as the element and can trigger TypeError with typed
protocol methods. Consider treating list-form as values only when the array
isn’t the legacy "elem => true" shape.
--
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]
