diff --git a/compiler/cpp/src/thrift/generate/t_php_generator.cc b/compiler/cpp/src/thrift/generate/t_php_generator.cc index 073bb9eb445..12c0795169c 100644 --- a/compiler/cpp/src/thrift/generate/t_php_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_php_generator.cc @@ -2681,13 +2681,33 @@ 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` 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'; + generate_serialize_set_element(out, (t_set*)ttype, iter_elem); } else { + indent(out) << "foreach ($" << prefix << " as $" << iter << " => $" << iter_val << ") {" << '\n'; + indent_up(); generate_serialize_set_element(out, (t_set*)ttype, iter_val); } scope_down(out); @@ -2754,9 +2774,10 @@ void t_php_generator::generate_serialize_map_element(ostream& out, * Serializes the members of a set. */ void t_php_generator::generate_serialize_set_element(ostream& out, t_set* tset, string iter) { - // Set element used as PHP array key — same coercion concern as map keys; - // see comment on emit_array_key_recast. Helper no-ops for non-castable - // element types. + // Scalar PHP sets may be represented either as a legacy keyed array of + // element => true markers or as a plain list of element values. Cast when + // needed so typed writeXxx() calls accept array-key sourced scalars after + // PHP key coercion. emit_array_key_recast(out, tset->get_elem_type(), iter); t_field efield(tset->get_elem_type(), iter); diff --git a/lib/php/lib/Base/TBase.php b/lib/php/lib/Base/TBase.php index f0a7447e848..9bd2d366287 100644 --- a/lib/php/lib/Base/TBase.php +++ b/lib/php/lib/Base/TBase.php @@ -349,8 +349,30 @@ 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` 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) { + $elem = match ($etype) { + TType::BOOL => (bool) $elem, + TType::BYTE, TType::I16, TType::I32, TType::I64 => (int) $elem, + TType::DOUBLE => (float) $elem, + TType::STRING, TType::UUID => (string) $elem, + default => $elem, + }; + } if (isset($ewrite)) { $xfer += $output->$ewrite($elem); } else { diff --git a/lib/php/lib/Exception/TException.php b/lib/php/lib/Exception/TException.php index ffc43627861..fe394353f9a 100644 --- a/lib/php/lib/Exception/TException.php +++ b/lib/php/lib/Exception/TException.php @@ -348,8 +348,30 @@ 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` 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) { + $elem = match ($etype) { + TType::BOOL => (bool) $elem, + TType::BYTE, TType::I16, TType::I32, TType::I64 => (int) $elem, + TType::DOUBLE => (float) $elem, + TType::STRING, TType::UUID => (string) $elem, + default => $elem, + }; + } if (isset($ewrite)) { $xfer += $output->$ewrite($elem); } else { diff --git a/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp b/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp index fc8975ec872..4d20333058d 100644 --- a/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp +++ b/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp @@ -950,7 +950,18 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* transport.writeI32(zend_hash_num_elements(ht)); HashPosition key_ptr; - if(ttype_is_scalar(keytype)){ + bool set_uses_values = false; + if (ttype_is_scalar(keytype) && zend_array_is_list(ht)) { + // All-true lists retain the legacy element => true interpretation. + ZEND_HASH_FOREACH_VAL(ht, val_ptr) { + ZVAL_DEREF(val_ptr); + if (Z_TYPE_P(val_ptr) != IS_TRUE) { + set_uses_values = true; + break; + } + } ZEND_HASH_FOREACH_END(); + } + if(ttype_is_scalar(keytype) && !set_uses_values){ for (zend_hash_internal_pointer_reset_ex(ht, &key_ptr); (val_ptr = zend_hash_get_current_data_ex(ht, &key_ptr)) != nullptr; zend_hash_move_forward_ex(ht, &key_ptr)) { diff --git a/lib/php/src/ext/thrift_protocol/tests/scalar_sets.phpt b/lib/php/src/ext/thrift_protocol/tests/scalar_sets.phpt new file mode 100644 index 00000000000..82c917aac77 --- /dev/null +++ b/lib/php/src/ext/thrift_protocol/tests/scalar_sets.phpt @@ -0,0 +1,94 @@ +--TEST-- +Scalar sets accept value lists and preserve legacy keyed sets +--SKIPIF-- + +--FILE-- + [TType::I32, [], []], + 'integer list' => [TType::I32, [42, -7, 19], [42, -7, 19]], + 'legacy sequential keys' => [TType::I32, [0 => true, 1 => true], [0, 1]], + 'string list' => [TType::STRING, ['a', '123'], ['a', '123']], + 'legacy numeric string' => [TType::STRING, ['123' => true], ['123']], + 'bool list' => [TType::BOOL, [true, false], [true, false]], + 'ambiguous bool' => [TType::BOOL, [true], [false]], + 'legacy bool' => [TType::BOOL, [1 => true], [true]], +]; +foreach ($cases as $name => [$type, $input, $elements]) { + ScalarSetPayload::$tspec = [1 => [ + 'var' => 'items', 'type' => TType::SET, + 'etype' => $type, 'elem' => ['type' => $type], + ]]; + $payload = new ScalarSetPayload(); + $payload->items = $input; + $actual = new TMemoryBuffer(); + thrift_protocol_write_binary(new TBinaryProtocol($actual), 'test', 1, $payload, 0, true); + + $expected = new TMemoryBuffer(); + $protocol = new TBinaryProtocol($expected); + $protocol->writeMessageBegin('test', 1, 0); + $protocol->writeStructBegin('ScalarSetPayload'); + $protocol->writeFieldBegin('items', TType::SET, 1); + $protocol->writeSetBegin($type, count($elements)); + $writer = [TType::I32 => 'writeI32', TType::STRING => 'writeString', TType::BOOL => 'writeBool'][$type]; + foreach ($elements as $element) { + $protocol->$writer($element); + } + $protocol->writeSetEnd(); + $protocol->writeFieldEnd(); + $protocol->writeFieldStop(); + $protocol->writeStructEnd(); + $protocol->writeMessageEnd(); + echo $name, ': ', $actual->getBuffer() === $expected->getBuffer() ? 'OK' : 'FAIL', "\n"; +} +?> +--EXPECT-- +empty: OK +integer list: OK +legacy sequential keys: OK +string list: OK +legacy numeric string: OK +bool list: OK +ambiguous bool: OK +legacy bool: OK diff --git a/lib/php/test/Integration/Lib/Protocol/TJSONProtocolTest.php b/lib/php/test/Integration/Lib/Protocol/TJSONProtocolTest.php index 22989089d35..26fbdc36ec1 100644 --- a/lib/php/test/Integration/Lib/Protocol/TJSONProtocolTest.php +++ b/lib/php/test/Integration/Lib/Protocol/TJSONProtocolTest.php @@ -29,8 +29,10 @@ use Thrift\Transport\TMemoryBuffer; use Basic\ThriftTest\Insanity; use Basic\ThriftTest\Numberz; +use Basic\ThriftTest\OptionalSetDefaultTest; use Basic\ThriftTest\Xtruct; use Basic\ThriftTest\Xtruct2; +use Basic\TestValidators\BoolSetTest; /*** * This test suite depends on running the compiler against the ./Resources/ThriftTest.thrift file: @@ -67,6 +69,44 @@ public function testMessageReadWrite() $this->assertSame('successResponse', $result); } + public function testWriteScalarSetAcceptsSequentialValues(): void + { + $struct = new OptionalSetDefaultTest([ + 'with_default' => ['element1', 'element2'], + ]); + + $struct->write($this->protocol); + + $actual = $this->transport->read(self::BUFFER_SIZE); + + $this->assertSame('{"1":{"set":["str",2,"element1","element2"]}}', $actual); + } + + #[DataProvider('boolSetDataProvider')] + public function testWriteBoolSetsResolvesTypedefs(array $values, string $expectedSet): void + { + $struct = new BoolSetTest([ + 'direct' => $values, + 'aliased' => $values, + 'chained' => $values, + ]); + $struct->write($this->protocol); + + $this->assertSame( + '{"1":{"set":' . $expectedSet . '},"2":{"set":' . $expectedSet + . '},"3":{"set":' . $expectedSet . '}}', + $this->transport->getBuffer() + ); + } + + public static function boolSetDataProvider(): iterable + { + yield 'sequential values' => [[true, false], '["tf",2,1,0]']; + yield 'legacy keyed values' => [[0 => true, 1 => true], '["tf",2,0,1]']; + yield 'ambiguous legacy false' => [[true], '["tf",1,0]']; + yield 'legacy true' => [[1 => true], '["tf",1,1]']; + } + #[DataProvider('writeDataProvider')] public function testWrite( $argsClassName, @@ -299,6 +339,13 @@ public static function writeDataProvider() ], 'expected' => '{"1":{"set":["i32",3,1,5,6]}}', ]; + yield 'set sequential legacy keys' => [ + 'argsClassName' => \Basic\ThriftTest\ThriftTest_testSet_args::class, + 'argsValues' => [ + 'thing' => [0 => true, 1 => true], + ], + 'expected' => '{"1":{"set":["i32",2,0,1]}}', + ]; yield 'list' => [ 'argsClassName' => \Basic\ThriftTest\ThriftTest_testList_args::class, 'argsValues' => [ diff --git a/lib/php/test/Resources/ThriftTest.thrift b/lib/php/test/Resources/ThriftTest.thrift index 97b4834ac28..9784dff327d 100644 --- a/lib/php/test/Resources/ThriftTest.thrift +++ b/lib/php/test/Resources/ThriftTest.thrift @@ -26,6 +26,15 @@ union UnionOfStrings { 2: string bb; } +typedef bool Flag +typedef Flag FlagAlias + +struct BoolSetTest { + 1: set direct; + 2: set aliased; + 3: set chained; +} + service TestService { void test() throws(1: ThriftTest.Xception xception); } diff --git a/lib/php/test/Unit/Lib/Base/Fixture/ComplexStruct.php b/lib/php/test/Unit/Lib/Base/Fixture/ComplexStruct.php index 23bc7e40bdf..7d978571b22 100644 --- a/lib/php/test/Unit/Lib/Base/Fixture/ComplexStruct.php +++ b/lib/php/test/Unit/Lib/Base/Fixture/ComplexStruct.php @@ -92,6 +92,14 @@ class ComplexStruct extends TBase 'var' => 'optionalField', 'type' => TType::STRING, ], + 9 => [ + 'var' => 'boolSetField', + 'type' => TType::SET, + 'etype' => TType::BOOL, + 'elem' => [ + 'type' => TType::BOOL, + ], + ], ]; public $flag = null; @@ -102,6 +110,7 @@ class ComplexStruct extends TBase public $setField = null; public $mapOfLists = null; public $optionalField = null; + public $boolSetField = null; public function read(TProtocol $input): int { diff --git a/lib/php/test/Unit/Lib/Base/TBaseTest.php b/lib/php/test/Unit/Lib/Base/TBaseTest.php index 686ef250634..4effa8afb4e 100644 --- a/lib/php/test/Unit/Lib/Base/TBaseTest.php +++ b/lib/php/test/Unit/Lib/Base/TBaseTest.php @@ -32,6 +32,27 @@ class TBaseTest extends TestCase { + public function testWriteLegacyNumericStringSet(): void + { + $protocol = new TBinaryProtocol(new TMemoryBuffer()); + $method = new \ReflectionMethod(\Thrift\Base\TBase::class, 'writeList'); + $method->invoke( + new ComplexStruct(), + ['123' => true], + ['etype' => TType::STRING, 'elem' => ['type' => TType::STRING]], + $protocol, + true + ); + + $type = $size = 0; + $protocol->readSetBegin($type, $size); + $protocol->readString($value); + $protocol->readSetEnd(); + $this->assertSame(TType::STRING, $type); + $this->assertSame(1, $size); + $this->assertSame('123', $value); + } + public function testConstructorHydratesKnownFieldsFromSpec(): void { $struct = new ComplexStruct( @@ -74,6 +95,76 @@ 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); + } + + public function testWriteSetPreservesSequentialLegacyKeys(): void + { + $struct = new ComplexStruct( + ComplexStruct::$tspec, + [ + 'setField' => [0 => true, 1 => true], + ] + ); + + $restored = $this->roundTrip($struct); + + $this->assertSame([0 => true, 1 => true], $restored->setField); + } + + public function testWriteBoolSetAcceptsSequentialValuesWhenUnambiguous(): void + { + $struct = new ComplexStruct( + ComplexStruct::$tspec, + [ + 'boolSetField' => [true, false], + ] + ); + + $restored = $this->roundTrip($struct); + + $this->assertSame([1 => true, 0 => true], $restored->boolSetField); + } + + public function testWriteBoolSetPreservesLegacyKeys(): void + { + $struct = new ComplexStruct( + ComplexStruct::$tspec, + [ + 'boolSetField' => [1 => true], + ] + ); + + $restored = $this->roundTrip($struct); + + $this->assertSame([1 => true], $restored->boolSetField); + } + + public function testWriteBoolSetPreservesLegacyMarkersForAmbiguousSequentialValues(): void + { + $struct = new ComplexStruct( + ComplexStruct::$tspec, + [ + 'boolSetField' => [true], + ] + ); + + $restored = $this->roundTrip($struct); + + $this->assertSame([0 => true], $restored->boolSetField); + } + public function testReadSkipsUnknownAndUnexpectedFields(): void { $transport = new TMemoryBuffer(); diff --git a/lib/php/test/Unit/Lib/Exception/TExceptionTest.php b/lib/php/test/Unit/Lib/Exception/TExceptionTest.php index a860c20e3ae..192c12bb213 100644 --- a/lib/php/test/Unit/Lib/Exception/TExceptionTest.php +++ b/lib/php/test/Unit/Lib/Exception/TExceptionTest.php @@ -23,8 +23,8 @@ namespace Test\Thrift\Unit\Lib\Exception; -use PHPUnit\Framework\TestCase; use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\TestCase; use Test\Thrift\Unit\Lib\Fixture\TestRichException; use Thrift\Base\TBase; use Thrift\Exception\TException; @@ -34,6 +34,27 @@ class TExceptionTest extends TestCase { + public function testWriteLegacyNumericStringSet(): void + { + $protocol = new TBinaryProtocol(new TMemoryBuffer()); + $method = new \ReflectionMethod(TException::class, 'writeList'); + $method->invoke( + new TException(), + ['123' => true], + ['etype' => TType::STRING, 'elem' => ['type' => TType::STRING]], + $protocol, + true + ); + + $type = $size = 0; + $protocol->readSetBegin($type, $size); + $protocol->readString($value); + $protocol->readSetEnd(); + $this->assertSame(TType::STRING, $type); + $this->assertSame(1, $size); + $this->assertSame('123', $value); + } + public function testTmethodMirrorsTBase(): void { // Guard against future drift between the HackTown-duplicated @@ -127,6 +148,56 @@ public static function writeAndReadFieldDataProvider() yield 'empty set' => ['field' => 'setField', 'value' => []]; } + public function testWriteAndReadScalarSetProvidedAsSequentialValues(): void + { + $exception = new TestRichException(); + $exception->setField = [5, 9]; + + $result = $this->roundtrip($exception); + + $this->assertSame([5 => true, 9 => true], $result->setField); + } + + public function testWriteAndReadScalarSetPreservesSequentialLegacyKeys(): void + { + $exception = new TestRichException(); + $exception->setField = [0 => true, 1 => true]; + + $result = $this->roundtrip($exception); + + $this->assertSame([0 => true, 1 => true], $result->setField); + } + + public function testWriteAndReadBoolSetAcceptsSequentialValuesWhenUnambiguous(): void + { + $exception = new TestRichException(); + $exception->boolSetField = [true, false]; + + $result = $this->roundtrip($exception); + + $this->assertSame([1 => true, 0 => true], $result->boolSetField); + } + + public function testWriteAndReadBoolSetPreservesLegacyKeys(): void + { + $exception = new TestRichException(); + $exception->boolSetField = [1 => true]; + + $result = $this->roundtrip($exception); + + $this->assertSame([1 => true], $result->boolSetField); + } + + public function testWriteAndReadBoolSetPreservesLegacyMarkersForAmbiguousSequentialValues(): void + { + $exception = new TestRichException(); + $exception->boolSetField = [true]; + + $result = $this->roundtrip($exception); + + $this->assertSame([0 => true], $result->boolSetField); + } + public function testWriteAndReadAllFields() { $exception = new TestRichException(); diff --git a/lib/php/test/Unit/Lib/Fixture/TestRichException.php b/lib/php/test/Unit/Lib/Fixture/TestRichException.php index bd8dbb00681..a1b81820023 100644 --- a/lib/php/test/Unit/Lib/Fixture/TestRichException.php +++ b/lib/php/test/Unit/Lib/Fixture/TestRichException.php @@ -40,6 +40,7 @@ class TestRichException extends TException public $listField = null; public $setField = null; public $uuidField = null; + public $boolSetField = null; public function __construct($vals = null) { @@ -84,6 +85,12 @@ public function __construct($vals = null) 'var' => 'uuidField', 'type' => TType::UUID, ], + 9 => [ + 'var' => 'boolSetField', + 'type' => TType::SET, + 'etype' => TType::BOOL, + 'elem' => ['type' => TType::BOOL], + ], ]; if (is_array($vals)) { diff --git a/test/php/TestClient.php b/test/php/TestClient.php index 20e5b1a5e4b..003d77f994b 100755 --- a/test/php/TestClient.php +++ b/test/php/TestClient.php @@ -319,6 +319,22 @@ function roundtrip($testClient, $method, $value) $exitcode |= ERR_CONTAINERS; } +// THRIFT-2950: list-form input must send values, not PHP array indexes. +foreach ([[], [-2, -1, 0, 1, 2], [42, -7, 19]] as $setValues) { + print_r('testSet(list values {' . implode(', ', $setValues) . '})'); + $setin = $testClient->testSet($setValues); + print_r(' = {' . implode(', ', array_keys($setin)) . "}\n"); + + // Reading a set still returns the legacy element => true representation. + $expectedSet = array_fill_keys($setValues, true); + ksort($expectedSet); + ksort($setin); + if ($setin !== $expectedSet) { + echo "**FAILED**\n"; + $exitcode |= ERR_CONTAINERS; + } +} + /** * LIST TEST */