From 1c98b5ba4740e97719c877fa77436a1a40c51188 Mon Sep 17 00:00:00 2001
From: Anton Grbin <anton@noom.com>
Date: Mon, 25 Nov 2024 08:21:05 -0800
Subject: [PATCH] [7392] fix conformance test in PHP JSON parser (#19376)

# Motivation

This PR fixes failing JSON conformance tests for php with name `IgnoreUnknownEnumStringValue*`.

The JSON parsing spec was discussed in https://github.com/protocolbuffers/protobuf/issues/7392.

Recent similar changes in other languages:
- Python: https://github.com/protocolbuffers/protobuf/commit/86abf35ef5ee5b1004ec11bebb36d84c2ef6645e
- Swift: https://github.com/apple/swift-protobuf/pull/1345
- C#: https://github.com/protocolbuffers/protobuf/pull/15758
- C++: https://github.com/protocolbuffers/protobuf/pull/16479

Note: this PR is equivalent to https://github.com/protocolbuffers/protobuf/pull/16743. I had to create a new one since I lost access to noom/protobuf in the meantime (switched companies recently).

Closes #19376

COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/19376 from antongrbin:anton--7392--php-newbranch 641a28a2b871f38a7e61ea0f05c3f15d677010ce
PiperOrigin-RevId: 699989555
---
 conformance/failure_list_php.txt             |  4 --
 php/src/Google/Protobuf/Internal/Message.php | 22 +++++++
 php/tests/EncodeDecodeTest.php               | 62 ++++++++++++++++++++
 3 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/conformance/failure_list_php.txt b/conformance/failure_list_php.txt
index 2deb1cccc9..5f450ac2d2 100644
--- a/conformance/failure_list_php.txt
+++ b/conformance/failure_list_php.txt
@@ -4,10 +4,6 @@ Recommended.*.FieldMaskTooManyUnderscore.JsonOutput
 Recommended.*.JsonInput.BytesFieldBase64Url.JsonOutput
 Recommended.*.JsonInput.BytesFieldBase64Url.ProtobufOutput
 Recommended.*.JsonInput.FieldMaskInvalidCharacter
-Recommended.*.JsonInput.IgnoreUnknownEnumStringValueInMapValue.ProtobufOutput
-Recommended.*.JsonInput.IgnoreUnknownEnumStringValueInRepeatedField.ProtobufOutput
-Recommended.*.JsonInput.IgnoreUnknownEnumStringValueInRepeatedPart.ProtobufOutput
-Recommended.*.JsonInput.IgnoreUnknownEnumStringValueInMapPart.ProtobufOutput
 Recommended.*.ProtobufInput.ValidDataOneofBinary.MESSAGE.Merge.ProtobufOutput
 Recommended.*.ValueRejectInfNumberValue.JsonOutput # Should have failed to serialize, but didn't.
 Recommended.*.ValueRejectNanNumberValue.JsonOutput # Should have failed to serialize, but didn't.
diff --git a/php/src/Google/Protobuf/Internal/Message.php b/php/src/Google/Protobuf/Internal/Message.php
index 31e2f29d30..eb91f35f29 100644
--- a/php/src/Google/Protobuf/Internal/Message.php
+++ b/php/src/Google/Protobuf/Internal/Message.php
@@ -1255,6 +1255,18 @@ class Message
                         $tmp_value,
                         $value_field,
                         $ignore_unknown);
+
+                    // Mapped unknown enum string values should be silently
+                    // ignored if ignore_unknown is set.
+                    if ($value_field->getType() == GPBType::ENUM &&
+                        is_string($tmp_value) &&
+                        is_null(
+                          $value_field->getEnumType()->getValueByName($tmp_value)
+                        ) &&
+                        $ignore_unknown) {
+                        continue;
+                    }
+
                     self::kvUpdateHelper($field, $proto_key, $proto_value);
                 }
             } else if ($field->isRepeated()) {
@@ -1270,6 +1282,16 @@ class Message
                         $tmp,
                         $field,
                         $ignore_unknown);
+
+                    // Repeated unknown enum string values should be silently
+                    // ignored if ignore_unknown is set.
+                    if ($field->getType() == GPBType::ENUM &&
+                        is_string($tmp) &&
+                        is_null($field->getEnumType()->getValueByName($tmp)) &&
+                        $ignore_unknown) {
+                        continue;
+                    }
+
                     self::appendHelper($field, $proto_value);
                 }
             } else {
diff --git a/php/tests/EncodeDecodeTest.php b/php/tests/EncodeDecodeTest.php
index abf95cca00..ebe497ff7e 100644
--- a/php/tests/EncodeDecodeTest.php
+++ b/php/tests/EncodeDecodeTest.php
@@ -205,6 +205,68 @@ class EncodeDecodeTest extends TestBase
         $this->assertSame("a", $m->getRepeatedField()[0]->getValue());
     }
 
+    public function testDecodeEnumMapWithUnknownStringValueThrows()
+    {
+        $this->expectException(Exception::class);
+
+        $m = new TestMessage();
+        $m->mergeFromJsonString(
+          "{\"map_int32_enum\":{
+            \"1\": \"ONE\",
+            \"2\": \"UNKNOWN_ENUM\",
+            \"3\": \"ZERO\"
+          }}"
+        );
+    }
+
+    public function testDecodeEnumMapWithUnknownStringValueIgnored()
+    {
+        $m = new TestMessage();
+        $m->mergeFromJsonString(
+          "{\"map_int32_enum\":{
+            \"1\": \"ONE\",
+            \"2\": \"UNKNOWN_ENUM\",
+            \"3\": \"ZERO\"
+          }}",
+          true
+        );
+
+        $this->assertSame(TestEnum::ONE, $m->getMapInt32Enum()["1"]);
+        $this->assertSame(TestEnum::ZERO, $m->getMapInt32Enum()["3"]);
+        $this->assertFalse($m->getMapInt32Enum()->offsetExists(2));
+    }
+
+    public function testDecodeRepeatedEnumWithUnknownStringValueThrows()
+    {
+        $this->expectException(Exception::class);
+
+        $m = new TestMessage();
+        $m->mergeFromJsonString(
+          "{\"repeated_enum\":[
+            \"ONE\",
+            \"UNKNOWN_ENUM\",
+            \"ZERO\"
+          ]}"
+        );
+    }
+
+    public function testDecodeRepeatedEnumWithUnknownStringValueIgnored()
+    {
+        $m = new TestMessage();
+        $m->mergeFromJsonString(
+          "{\"repeated_enum\":[
+            \"ONE\",
+            \"UNKNOWN_ENUM\",
+            \"ZERO\"
+          ]}",
+          true
+        );
+
+        $this->assertSame(2, count($m->getRepeatedEnum()));
+        $this->assertSame(TestEnum::ONE, $m->getRepeatedEnum()[0]);
+        $this->assertSame(TestEnum::ZERO, $m->getRepeatedEnum()[1]);
+    }
+
     public function testDecodeMapStringValue()
     {
         $m = new TestStringValue();