From 518be131c02f6fe5f9be5b8fe71440b97c55b7a7 Mon Sep 17 00:00:00 2001 From: Jaapio Date: Wed, 8 May 2024 20:37:31 +0200 Subject: [PATCH] Bugfix: resolve issue with multiline descriptions The phpstan parser is not consuming the full description when parsing docblocks with a more complex description. For them it's mostlikely not an issue as phpstan doesn't use the descriptions. But it will also parse the descriptions into unexpected tags. This could be an advantage but is not according to the phpdoc spec. Our own tokenizer is already tokenizing the docblocks into the correct parts. So all we needed to do is assume all remaining tokens in the phpstan ast belong to the description. From there our own code is able to handle this as before in v5.3. fixes #365 --- psalm.xml | 1 + .../Tags/Factory/AbstractPHPStanFactory.php | 9 +- src/DocBlock/Tags/Factory/ParamFactory.php | 8 +- src/DocBlock/Tags/Factory/PropertyFactory.php | 8 +- .../Tags/Factory/PropertyReadFactory.php | 8 +- .../Tags/Factory/PropertyWriteFactory.php | 8 +- src/DocBlock/Tags/Factory/ReturnFactory.php | 9 +- src/DocBlock/Tags/Factory/VarFactory.php | 8 +- .../integration/InterpretingDocBlocksTest.php | 100 +++++++++++++++++- .../Tags/Factory/TagFactoryTestCase.php | 9 +- 10 files changed, 157 insertions(+), 11 deletions(-) diff --git a/psalm.xml b/psalm.xml index 7f491f7f..d4b47bfa 100644 --- a/psalm.xml +++ b/psalm.xml @@ -44,6 +44,7 @@ + diff --git a/src/DocBlock/Tags/Factory/AbstractPHPStanFactory.php b/src/DocBlock/Tags/Factory/AbstractPHPStanFactory.php index 7a5f8e2c..2c3b43f7 100644 --- a/src/DocBlock/Tags/Factory/AbstractPHPStanFactory.php +++ b/src/DocBlock/Tags/Factory/AbstractPHPStanFactory.php @@ -23,6 +23,8 @@ use PHPStan\PhpDocParser\Parser\TypeParser; use RuntimeException; +use function property_exists; + /** * Factory class creating tags using phpstan's parser * @@ -48,8 +50,11 @@ public function __construct(PHPStanFactory ...$factories) public function create(string $tagLine, ?TypeContext $context = null): Tag { - $tokens = $this->lexer->tokenize($tagLine); - $ast = $this->parser->parseTag(new TokenIterator($tokens)); + $tokens = new TokenIterator($this->lexer->tokenize($tagLine)); + $ast = $this->parser->parseTag($tokens); + if (property_exists($ast->value, 'description') === true) { + $ast->value->setAttribute('description', $ast->value->description . $tokens->joinUntil(Lexer::TOKEN_END)); + } if ($context === null) { $context = new TypeContext(''); diff --git a/src/DocBlock/Tags/Factory/ParamFactory.php b/src/DocBlock/Tags/Factory/ParamFactory.php index 8e9bcaac..7d680d9c 100644 --- a/src/DocBlock/Tags/Factory/ParamFactory.php +++ b/src/DocBlock/Tags/Factory/ParamFactory.php @@ -19,6 +19,7 @@ use PHPStan\PhpDocParser\Ast\Type\OffsetAccessTypeNode; use Webmozart\Assert\Assert; +use function is_string; use function sprintf; use function trim; @@ -68,11 +69,16 @@ public function create(PhpDocTagNode $node, Context $context): Tag ); } + $description = $tagValue->getAttribute('description'); + if (is_string($description) === false) { + $description = $tagValue->description; + } + return new Param( trim($tagValue->parameterName, '$'), $this->typeResolver->createType($tagValue->type ?? new IdentifierTypeNode('mixed'), $context), $tagValue->isVariadic, - $this->descriptionFactory->create($tagValue->description, $context), + $this->descriptionFactory->create($description, $context), $tagValue->isReference ); } diff --git a/src/DocBlock/Tags/Factory/PropertyFactory.php b/src/DocBlock/Tags/Factory/PropertyFactory.php index 83a18e8a..b744ed08 100644 --- a/src/DocBlock/Tags/Factory/PropertyFactory.php +++ b/src/DocBlock/Tags/Factory/PropertyFactory.php @@ -13,6 +13,7 @@ use PHPStan\PhpDocParser\Ast\PhpDoc\PropertyTagValueNode; use Webmozart\Assert\Assert; +use function is_string; use function trim; /** @@ -34,10 +35,15 @@ public function create(PhpDocTagNode $node, Context $context): Tag $tagValue = $node->value; Assert::isInstanceOf($tagValue, PropertyTagValueNode::class); + $description = $tagValue->getAttribute('description'); + if (is_string($description) === false) { + $description = $tagValue->description; + } + return new Property( trim($tagValue->propertyName, '$'), $this->typeResolver->createType($tagValue->type, $context), - $this->descriptionFactory->create($tagValue->description, $context) + $this->descriptionFactory->create($description, $context) ); } diff --git a/src/DocBlock/Tags/Factory/PropertyReadFactory.php b/src/DocBlock/Tags/Factory/PropertyReadFactory.php index be443ab6..b0898aa7 100644 --- a/src/DocBlock/Tags/Factory/PropertyReadFactory.php +++ b/src/DocBlock/Tags/Factory/PropertyReadFactory.php @@ -13,6 +13,7 @@ use PHPStan\PhpDocParser\Ast\PhpDoc\PropertyTagValueNode; use Webmozart\Assert\Assert; +use function is_string; use function trim; /** @@ -34,10 +35,15 @@ public function create(PhpDocTagNode $node, Context $context): Tag $tagValue = $node->value; Assert::isInstanceOf($tagValue, PropertyTagValueNode::class); + $description = $tagValue->getAttribute('description'); + if (is_string($description) === false) { + $description = $tagValue->description; + } + return new PropertyRead( trim($tagValue->propertyName, '$'), $this->typeResolver->createType($tagValue->type, $context), - $this->descriptionFactory->create($tagValue->description, $context) + $this->descriptionFactory->create($description, $context) ); } diff --git a/src/DocBlock/Tags/Factory/PropertyWriteFactory.php b/src/DocBlock/Tags/Factory/PropertyWriteFactory.php index 08cf7534..749b1eda 100644 --- a/src/DocBlock/Tags/Factory/PropertyWriteFactory.php +++ b/src/DocBlock/Tags/Factory/PropertyWriteFactory.php @@ -13,6 +13,7 @@ use PHPStan\PhpDocParser\Ast\PhpDoc\PropertyTagValueNode; use Webmozart\Assert\Assert; +use function is_string; use function trim; /** @@ -34,10 +35,15 @@ public function create(PhpDocTagNode $node, Context $context): Tag $tagValue = $node->value; Assert::isInstanceOf($tagValue, PropertyTagValueNode::class); + $description = $tagValue->getAttribute('description'); + if (is_string($description) === false) { + $description = $tagValue->description; + } + return new PropertyWrite( trim($tagValue->propertyName, '$'), $this->typeResolver->createType($tagValue->type, $context), - $this->descriptionFactory->create($tagValue->description, $context) + $this->descriptionFactory->create($description, $context) ); } diff --git a/src/DocBlock/Tags/Factory/ReturnFactory.php b/src/DocBlock/Tags/Factory/ReturnFactory.php index 8ebe5ad4..4a17dc24 100644 --- a/src/DocBlock/Tags/Factory/ReturnFactory.php +++ b/src/DocBlock/Tags/Factory/ReturnFactory.php @@ -13,6 +13,8 @@ use PHPStan\PhpDocParser\Ast\PhpDoc\ReturnTagValueNode; use Webmozart\Assert\Assert; +use function is_string; + /** * @internal This class is not part of the BC promise of this library. */ @@ -32,9 +34,14 @@ public function create(PhpDocTagNode $node, Context $context): Tag $tagValue = $node->value; Assert::isInstanceOf($tagValue, ReturnTagValueNode::class); + $description = $tagValue->getAttribute('description'); + if (is_string($description) === false) { + $description = $tagValue->description; + } + return new Return_( $this->typeResolver->createType($tagValue->type, $context), - $this->descriptionFactory->create($tagValue->description, $context) + $this->descriptionFactory->create($description, $context) ); } diff --git a/src/DocBlock/Tags/Factory/VarFactory.php b/src/DocBlock/Tags/Factory/VarFactory.php index 79ffdf12..479ceb27 100644 --- a/src/DocBlock/Tags/Factory/VarFactory.php +++ b/src/DocBlock/Tags/Factory/VarFactory.php @@ -13,6 +13,7 @@ use PHPStan\PhpDocParser\Ast\PhpDoc\VarTagValueNode; use Webmozart\Assert\Assert; +use function is_string; use function trim; /** @@ -34,10 +35,15 @@ public function create(PhpDocTagNode $node, Context $context): Tag $tagValue = $node->value; Assert::isInstanceOf($tagValue, VarTagValueNode::class); + $description = $tagValue->getAttribute('description'); + if (is_string($description) === false) { + $description = $tagValue->description; + } + return new Var_( trim($tagValue->variableName, '$'), $this->typeResolver->createType($tagValue->type, $context), - $this->descriptionFactory->create($tagValue->description, $context) + $this->descriptionFactory->create($description, $context) ); } diff --git a/tests/integration/InterpretingDocBlocksTest.php b/tests/integration/InterpretingDocBlocksTest.php index 51f3e8f0..e9685e97 100644 --- a/tests/integration/InterpretingDocBlocksTest.php +++ b/tests/integration/InterpretingDocBlocksTest.php @@ -17,16 +17,21 @@ use phpDocumentor\Reflection\DocBlock\Description; use phpDocumentor\Reflection\DocBlock\StandardTagFactory; use phpDocumentor\Reflection\DocBlock\Tag; +use phpDocumentor\Reflection\DocBlock\Tags\Generic; use phpDocumentor\Reflection\DocBlock\Tags\InvalidTag; use phpDocumentor\Reflection\DocBlock\Tags\Method; use phpDocumentor\Reflection\DocBlock\Tags\MethodParameter; use phpDocumentor\Reflection\DocBlock\Tags\Param; use phpDocumentor\Reflection\DocBlock\Tags\Return_; use phpDocumentor\Reflection\DocBlock\Tags\See; +use phpDocumentor\Reflection\DocBlock\Tags\Since; use phpDocumentor\Reflection\PseudoTypes\ConstExpression; use phpDocumentor\Reflection\Types\Array_; +use phpDocumentor\Reflection\Types\Compound; +use phpDocumentor\Reflection\Types\Context; use phpDocumentor\Reflection\Types\Integer; use phpDocumentor\Reflection\Types\Mixed_; +use phpDocumentor\Reflection\Types\Object_; use phpDocumentor\Reflection\Types\Self_; use phpDocumentor\Reflection\Types\String_; use phpDocumentor\Reflection\Types\Void_; @@ -123,8 +128,8 @@ public function testInterpretingTags(): void $this->assertInstanceOf(See::class, $seeTags[0]); $seeTag = $seeTags[0]; - $this->assertSame('\\' . StandardTagFactory::class, (string) $seeTag->getReference()); - $this->assertSame('', (string) $seeTag->getDescription()); + $this->assertSame('\\' . StandardTagFactory::class, (string)$seeTag->getReference()); + $this->assertSame('', (string)$seeTag->getDescription()); } public function testDescriptionsCanEscapeAtSignsAndClosingBraces(): void @@ -268,4 +273,95 @@ public function testConstantReferenceTypes(): void $docblock->getTags() ); } + + public function testRegressionWordpressDocblocks(): void + { + $docCommment = <<create($docCommment); + + self::assertEquals( + new DocBlock( + 'Install a package.', + new Description( + 'Copies the contents of a package from a source directory, and installs them in' . PHP_EOL . + 'a destination directory. Optionally removes the source. It can also optionally' . PHP_EOL . + 'clear out the destination folder if it already exists.' + ), + [ + new Since('2.8.0', new Description('')), + new Since('6.2.0', new Description('Use move_dir() instead of copy_dir() when possible.')), + new Generic( + 'global', + new Description('WP_Filesystem_Base $wp_filesystem WordPress filesystem subclass.') + ), + new Generic( + 'global', + new Description('array $wp_theme_directories') + ), + new Param( + 'args', + new Compound([new Array_(new Mixed_()), new String_()]), + false, + new Description( + '{' . "\n" . + 'Optional. Array or string of arguments for installing a package. Default empty array.' . "\n" . + "\n" . + ' @type string $source Required path to the package source. Default empty.' . "\n" . + ' @type string $destination Required path to a folder to install the package in.' . "\n" . + ' Default empty.' . "\n" . + ' @type bool $clear_destination Whether to delete any files already in the destination' . "\n" . + ' folder. Default false.' . "\n" . + ' @type bool $clear_working Whether to delete the files from the working directory' . "\n" . + ' after copying them to the destination. Default false.' . "\n" . + ' @type bool $abort_if_destination_exists Whether to abort the installation if' . "\n" . + ' the destination folder already exists. Default true.' . "\n" . + ' @type array $hook_extra Extra arguments to pass to the filter hooks called by' . "\n" . + ' WP_Upgrader::install_package(). Default empty array.' . "\n" . + '}' + ) + ), + new Return_( + new Compound([new Array_(new Mixed_()), new Object_(new Fqsen('\WP_Error'))]), + new Description('The result (also stored in `WP_Upgrader::$result`), or a WP_Error on failure.') + ), + ], + new Context('\\') + ), + $docblock + ); + } } diff --git a/tests/unit/DocBlock/Tags/Factory/TagFactoryTestCase.php b/tests/unit/DocBlock/Tags/Factory/TagFactoryTestCase.php index ec905654..d97c7f1d 100644 --- a/tests/unit/DocBlock/Tags/Factory/TagFactoryTestCase.php +++ b/tests/unit/DocBlock/Tags/Factory/TagFactoryTestCase.php @@ -26,6 +26,8 @@ use PHPStan\PhpDocParser\Parser\TypeParser; use PHPUnit\Framework\TestCase; +use function property_exists; + abstract class TagFactoryTestCase extends TestCase { public function parseTag(string $tag): PhpDocTagNode @@ -34,7 +36,12 @@ public function parseTag(string $tag): PhpDocTagNode $tokens = $lexer->tokenize($tag); $constParser = new ConstExprParser(); - return (new PhpDocParser(new TypeParser($constParser), $constParser))->parseTag(new TokenIterator($tokens)); + $tagNode = (new PhpDocParser(new TypeParser($constParser), $constParser))->parseTag(new TokenIterator($tokens)); + if (property_exists($tagNode->value, 'description') === true) { + $tagNode->value->setAttribute('description', $tagNode->value->description); + } + + return $tagNode; } public function giveTypeResolver(): TypeResolver