Skip to content

THRIFT-5090: Add PHP JSON slash regression coverage - #3791

Merged
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5090
Sep 11, 2026
Merged

THRIFT-5090: Add PHP JSON slash regression coverage#3791
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5090

Conversation

@sveneld

@sveneld sveneld commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Add regression coverage for unescaped forward slashes in PHP JSON protocol output and exercise the same slash-heavy string in the PHP cross-test client.

Status

Rebased onto upstream master 81103bf on 2026-09-11. This branch now contains only THRIFT-5090 and no longer depends on THRIFT-2950 / #3787.

Testing

  • PHP TJSONProtocol unit tests in Docker: 55 tests, 112 assertions passed on the rebased branch.
  • PHP cross-test client syntax check passed.
  • Cross-language execution awaits CI.

Generated-by: OpenAI Codex GPT-5 and GPT-6

@sveneld
sveneld marked this pull request as ready for review September 3, 2026 06:31
Copilot AI lite review requested due to automatic review settings September 3, 2026 06:31
@mergeable mergeable Bot added the php label Sep 3, 2026
@mergeable mergeable Bot added the compiler label Sep 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new array_is_list()-based set handling can break legacy keyed-set arrays with sequential numeric keys (e.g. element 0), causing incorrect serialization or strict-types TypeErrors.

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

Pull request overview

This PR adds PHP test coverage to lock in JSON protocol string behavior around forward slashes, and also introduces/extends PHP set-serialization behavior to accept sequential-value arrays (with accompanying unit/integration coverage and generator updates).

Changes:

  • Add a unit regression test asserting TJSONProtocol::writeString() does not escape /.
  • Extend the PHP cross-test client to roundtrip a slash-heavy string (path + URL).
  • Update PHP runtime + PHP code generator to treat scalar sets as either legacy keyed arrays or sequential-value lists, with new tests validating sequential-value set inputs.
File summaries
File Description
test/php/TestClient.php Adds a slash-heavy testString roundtrip to keep cross-tests exercising JSON slash handling.
lib/php/test/Unit/Lib/Protocol/TJSONProtocolTest.php Adds a unit test asserting forward slashes are not escaped in JSON string output.
lib/php/test/Unit/Lib/Exception/TExceptionTest.php Adds a regression test for serializing a set provided as sequential values on exceptions.
lib/php/test/Unit/Lib/Base/TBaseTest.php Adds a regression test for serializing a set provided as sequential values on structs.
lib/php/test/Integration/Lib/Protocol/TJSONProtocolTest.php Adds an integration assertion for JSON output when a set is provided as sequential values.
lib/php/lib/Exception/TException.php Changes set serialization to sometimes use values (for list arrays) instead of keys.
lib/php/lib/Base/TBase.php Changes set serialization to sometimes use values (for list arrays) instead of keys.
compiler/cpp/src/thrift/generate/t_php_generator.cc Changes generated scalar-set serialization to choose between keys vs values using array_is_list().
Review details

Suppressed comments (1)

lib/php/lib/Base/TBase.php:352

  • The PR description/title focus on adding JSON forward-slash regression coverage without changing runtime semantics, but this change alters PHP runtime behavior for set serialization (accepting sequential-value arrays) and is paired with generator changes and new set-focused tests. Consider splitting the set-serialization behavior change into its own PR (or updating the PR title/description to reflect the additional behavioral change).
        $setUsesValues = $set && array_is_list($var);
  • Files reviewed: 8/8 changed files
  • Comments generated: 3
  • Review effort level: Lite

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

Comment thread compiler/cpp/src/thrift/generate/t_php_generator.cc Outdated
Comment thread lib/php/lib/Base/TBase.php Outdated
Comment thread lib/php/lib/Exception/TException.php Outdated
@sveneld
sveneld marked this pull request as draft September 3, 2026 06:58
Client: php

Generated-by: OpenAI Codex GPT-5 <noreply@openai.com>
@sveneld
sveneld marked this pull request as ready for review September 11, 2026 10:42
Copilot AI review requested due to automatic review settings September 11, 2026 10:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No blocking issues were identified.

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

@sveneld
sveneld merged commit b0b31cf into apache:master Sep 11, 2026
76 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants