From f929439ccd19872d71f56050e6ba7af3cd7c753c Mon Sep 17 00:00:00 2001
From: Brent Shaffer <betterbrent@google.com>
Date: Wed, 31 Jan 2024 10:29:51 -0800
Subject: [PATCH] feat: more readable phpdoc escaping (#11208)

The PHPDoc escaping in PHP is aggressive in that it escapes some character sequences that don't need to be escaped (`/*`), and it uses HTML entities to escape others (`*/` and `@`) instead of the recommended PHPDoc escape sequences.

For Example, in [`Google\Api\RoutingParameter`](https://github.com/googleapis/common-protos-php/blob/main/src/Api/RoutingParameter.php#L42):

```
 * path_template: "projects/&#42;&#47;{table_location=instances/&#42;}/tables/&#42;"
```

Should be escaped as:

```
 * path_template: "projects/{@*}{table_location=instances/*}/tables/*"
```

according to [the PHPDoc guide](https://manual.phpdoc.org/HTMLframesConverter/default/phpDocumentor/tutorial_phpDocumentor.howto.pkg.html#basics.desc):

 - For `@`: "if you need an actual "@" in your DocBlock's description parts, you should be careful to either ensure it is not the first character on a line, or else escape it ("\\@") to avoid it being interpreted as a PhpDocumentor tag marker."

 - For `*/`: " If you need to use the closing comment "\*/" in a DocBlock, use the special escape sequence "{@*}."

Closes #11208

COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/11208 from bshaffer:more-readable-phpdoc-escaping a75f9745ea5cd51e87342a052c8a07127ed8d192
PiperOrigin-RevId: 603091642
---
 php/tests/GeneratedClassTest.php              | 22 +++++++++++++++++
 php/tests/proto/test_special_characters.proto | 17 +++++++++++++
 .../protobuf/compiler/php/php_generator.cc    | 24 +++++++------------
 3 files changed, 47 insertions(+), 16 deletions(-)
 create mode 100644 php/tests/proto/test_special_characters.proto

diff --git a/php/tests/GeneratedClassTest.php b/php/tests/GeneratedClassTest.php
index 0af663ac38306..6f3bff7f734fb 100644
--- a/php/tests/GeneratedClassTest.php
+++ b/php/tests/GeneratedClassTest.php
@@ -13,6 +13,7 @@
 use Foo\TestEnum;
 use Foo\TestIncludeNamespaceMessage;
 use Foo\TestIncludePrefixMessage;
+use Foo\TestSpecialCharacters;
 use Foo\TestMessage;
 use Foo\TestMessage\Sub;
 use Foo\TestMessage_Sub;
@@ -1906,4 +1907,25 @@ public function testIssue9440()
         $m->setVersion('1');
         $this->assertEquals(8, $m->getId());
     }
+
+    public function testSpecialCharacters()
+    {
+        $reflectionMethod = new \ReflectionMethod(TestSpecialCharacters::class, 'getA');
+        $docComment = $reflectionMethod->getDocComment();
+        $commentLines = explode("\n", $docComment);
+        $this->assertEquals('/**', array_shift($commentLines));
+        $this->assertEquals('     */', array_pop($commentLines));
+        $docComment = implode("\n", $commentLines);
+        // test special characters
+        $this->assertContains(";,/?:&=+$-_.!~*'()", $docComment);
+        // test open doc comment
+        $this->assertContains('/*', $docComment);
+        // test escaped closed doc comment
+        $this->assertNotContains('*/', $docComment);
+        $this->assertContains('{@*}', $docComment);
+        // test escaped at-sign
+        $this->assertContains('\@foo', $docComment);
+        // test forwardslash on new line
+        $this->assertContains("* /\n", $docComment);
+    }
 }
diff --git a/php/tests/proto/test_special_characters.proto b/php/tests/proto/test_special_characters.proto
new file mode 100644
index 0000000000000..a4dd2e42fc1d8
--- /dev/null
+++ b/php/tests/proto/test_special_characters.proto
@@ -0,0 +1,17 @@
+syntax = "proto3";
+
+package foo;
+
+message TestSpecialCharacters {
+  // test special characters (shouldn't escape): ;,/?:&=+$-_.!~*'()
+  // test open comment (shouldn't escape): /*
+  // test close comment (should escape): */
+  // test at-sign (should escape): @foo
+  // test forward slash as first character on a newline:
+  ///
+  string a = 1;
+
+  ///
+  // test forward slash as first character on first line
+  string b = 2;
+}
diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc
index 5dcb737308851..9eef41b52de87 100644
--- a/src/google/protobuf/compiler/php/php_generator.cc
+++ b/src/google/protobuf/compiler/php/php_generator.cc
@@ -1556,38 +1556,30 @@ static std::string EscapePhpdoc(absl::string_view input) {
   std::string result;
   result.reserve(input.size() * 2);
 
-  char prev = '*';
+  char prev = '\0';
 
   for (std::string::size_type i = 0; i < input.size(); i++) {
     char c = input[i];
     switch (c) {
-      case '*':
-        // Avoid "/*".
-        if (prev == '/') {
-          result.append("&#42;");
-        } else {
-          result.push_back(c);
-        }
-        break;
+      // NOTE: "/*" is allowed, do not escape it
       case '/':
-        // Avoid "*/".
+        // Escape "*/" with "{@*}".
         if (prev == '*') {
-          result.append("&#47;");
+          result.pop_back();
+          result.append("{@*}");
         } else {
           result.push_back(c);
         }
         break;
       case '@':
-        // '@' starts phpdoc tags including the @deprecated tag, which will
-        // cause a compile-time error if inserted before a declaration that
-        // does not have a corresponding @Deprecated annotation.
-        result.append("&#64;");
+        // '@' starts phpdoc tags. Play it safe and escape it.
+        result.append("\\");
+        result.push_back(c);
         break;
       default:
         result.push_back(c);
         break;
     }
-
     prev = c;
   }