Skip to content

Commit

Permalink
feat: more readable phpdoc escaping (#11208)
Browse files Browse the repository at this point in the history
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/*/{table_location=instances/*}/tables/*"
```

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=#11208 from bshaffer:more-readable-phpdoc-escaping a75f974
PiperOrigin-RevId: 603091642
  • Loading branch information
bshaffer authored and Copybara-Service committed Jan 31, 2024
1 parent f8ea418 commit f929439
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 16 deletions.
22 changes: 22 additions & 0 deletions php/tests/GeneratedClassTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
17 changes: 17 additions & 0 deletions php/tests/proto/test_special_characters.proto
Original file line number Diff line number Diff line change
@@ -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;
}
24 changes: 8 additions & 16 deletions src/google/protobuf/compiler/php/php_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down

0 comments on commit f929439

Please sign in to comment.