THRIFT-2950: Fix PHP scalar set serialization

[sveneld]

Summary

PHP scalar sets passed as sequential value lists were serialized using their array indexes. Accept value lists in generated serializers, the OOP runtime, and the native extension while preserving keyed sets with boolean true markers.

• Generate REST set arguments with true markers so sequential keys such as {0, 1} survive serialization.
• Cast scalar set elements to their declared type in both keyed and value-list forms, consistently across generated code, TBase, and TException.
• Cover REST round trips, numeric-string and integer-boolean inputs, typedefs, legacy keyed sets, and value lists. Retain the shared PHP cross-tests for empty and nonempty sets.

Compatibility

Keyed sets should use array_fill_keys($elements, true). Arrays with sequential keys and markers other than strictly true are interpreted as value lists; their original intent cannot be inferred from the array. The PHP README documents this change and the all-true ambiguity: [true] retains the legacy key 0 interpretation; [1 => true] represents a boolean set containing only true.

Validation

• Reproduced the review failures before the fixes: REST dropped element 0, and both OOP runtime helpers raised TypeError for convertible scalar values.
• Rebuilt the compiler and regenerated all PHP test fixtures.
• PHP 8.4.21 / PHPUnit 13.3.3: 204 related unit and integration tests, 381 assertions, no failures, errors, or warnings after rebasing onto current master.
• PHPCS passes for changed PHP files; git diff --check passes. make style is unavailable in this CMake checkout (no generated Makefile).
• Earlier validation on this branch: six PHP cross-test runs against C++ and Python servers with Binary, Compact, and JSON protocols; these cross-tests were not rerun for this review update.

Generated-by: Codex (GPT-6)

[Copilot]

🟡 Changes recommended

The new array_is_list()-based detection can misclassify legacy keyed-set inputs with sequential element keys (e.g. {0,1}), causing incorrect serialization in both runtime and generated code.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the PHP serializer (both generated PHP code and the shared runtime helpers) to accept scalar Thrift set<> values provided as sequential PHP lists, while adding regression tests for TBase/TException round-trips and JSON protocol output.

Changes:

• Accept sequential PHP arrays (e.g. [10, 20]) as scalar-set inputs during serialization.
• Apply the same compatibility logic in both TBase and TException runtime writers and the PHP code generator.
• Add unit/integration coverage for sequential set inputs (TBase, TException, TJSONProtocol).

File summaries

File	Description
lib/php/test/Unit/Lib/Exception/TExceptionTest.php	Adds a unit test asserting round-trip behavior when a set is provided as a sequential list.
lib/php/test/Unit/Lib/Base/TBaseTest.php	Adds a unit test asserting round-trip behavior when a struct set field is provided as a sequential list.
lib/php/test/Integration/Lib/Protocol/TJSONProtocolTest.php	Adds an integration test asserting JSON serialization output for a sequential-list set input.
lib/php/lib/Exception/TException.php	Updates set/list writing logic to optionally serialize set values for sequential arrays.
lib/php/lib/Base/TBase.php	Updates set/list writing logic to optionally serialize set values for sequential arrays.
compiler/cpp/src/thrift/generate/t_php_generator.cc	Updates generated scalar-set serialization to choose between iterating keys vs values based on list-ness.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new list-vs-legacy heuristic introduces an unresolved ambiguity for set<bool> (e.g., [true]) that needs an explicit behavior decision plus tests/docs to match the PR’s stated compatibility goal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The runtime and generator changes are consistent with each other and are backed by targeted unit/integration regression tests covering the key compatibility and ambiguity cases.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Legacy keyed string-set serialization still passes uncast keys in TBase and TException.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

lib/php/lib/Base/TBase.php:368

• The legacy-marker branch still passes the raw PHP array key to the typed protocol writer. A valid legacy set<string> such as ['0' => true] is stored by PHP with integer key 0, so writeString() throws under strict_types=1; the generated serializer already recasts set keys to their declared scalar type. Cast the selected legacy key according to $etype before dispatch, not only for BOOL.

            $elem = $set && !$setUsesValues ? $key : $val;
            if ($set && !$setUsesValues && $etype === TType::BOOL) {
                $elem = (bool) $elem;

lib/php/lib/Exception/TException.php:368

• The legacy-marker branch still passes the raw PHP array key to the typed protocol writer. A valid legacy set<string> such as ['0' => true] is stored by PHP with integer key 0, so writeString() throws under strict_types=1; the generated serializer already recasts set keys to their declared scalar type. Cast the selected legacy key according to $etype before dispatch, not only for BOOL.

            if ($set && !$setUsesValues && $etype === TType::BOOL) {
                $elem = (bool) $elem;
            }

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Resolve typedefs before checking for boolean element types in generated set serialization.

Review details

Suppressed comments (1)

compiler/cpp/src/thrift/generate/t_php_generator.cc:2707

• elem_type can be a typedef, but t_typedef does not override is_bool(). For typedef bool Flag; set<Flag>, the ambiguous legacy form such as [true] therefore skips the cast and the generated strict-types code passes the integer PHP key (0) to writeBool(bool), causing a TypeError. Resolve the type before checking is_bool() so typedef-based bool sets follow the same compatibility path.

      if (elem_type->is_bool()) {

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[sveneld]

Addressed the bool typedef review concern with generated-code coverage and simplification. New fixtures cover set, set (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.

[Copilot]

🟡 Changes recommended

Moderate issues remain in TBase and TException for legacy string/UUID keyed sets under strict typing.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Lite

[sveneld]

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.

[Copilot]

🔵 Needs a closer look

Add inline-mode coverage that verifies the serializer emits actual non-consecutive set elements.

Review details

Suppressed comments (1)

compiler/cpp/src/thrift/generate/t_php_generator.cc:2685

• The updated set loop also changes the binary_inline_ serializer, but the existing inlined test only writes newset and then reads it with the older V1 schema, so that field is skipped and an implementation that still serialized indexes would pass. Add an inline-mode assertion or round-trip that inspects the emitted set elements, including a non-consecutive list value.

    if (php_is_scalar(elem_type)) {

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Jens-G]

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.

thrift/compiler/cpp/src/thrift/generate/t_php_generator.cc

Lines 2684 to 2708 in 09140da

	t_type* elem_type = ((t_set*)ttype)->get_elem_type();
	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';
	generate_serialize_set_element(out, (t_set*)ttype, iter_elem);
	} else {

thrift/compiler/cpp/src/thrift/generate/t_php_generator.cc

Lines 1940 to 1944 in 09140da

	<< (*a_iter)->get_name() << ", true);" << '\n';
	} else if (atype->is_set()) {
	f_service_rest << indent() << "$" << (*a_iter)->get_name() << " = array_fill_keys(json_decode($"
	<< (*a_iter)->get_name() << ", true), 1);" << '\n';
	} else if (atype->is_struct() || atype->is_xception()) {

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.

thrift/lib/php/lib/Base/TBase.php

Lines 365 to 376 in 09140da

	foreach ($var as $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)) {

thrift/compiler/cpp/src/thrift/generate/t_php_generator.cc

Lines 2776 to 2782 in 09140da

	void t_php_generator::generate_serialize_set_element(ostream& out, t_set* tset, string iter) {
	// 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);

🤖 Generated with Claude Code