From bdecc2d3bfd4019b506b77815a75c88771b95009 Mon Sep 17 00:00:00 2001 From: Jaapio Date: Fri, 3 Jan 2020 13:33:06 +0100 Subject: [PATCH 1/3] Fix issue when processing invalid tags When invalid tags are processed a null was returned causing all kind of issues in the normal behavior of this libary. As a solution a generic tag could be created. But that would just drop the error information in. Therefore a new tag was introduced, `invalidTag` the tag is just like the generic tag but does contain the error triggered during the creation of the tag. Which might help applications like phpdocumentor to display validation issues. --- src/DocBlock/DescriptionFactory.php | 5 +- src/DocBlock/StandardTagFactory.php | 7 +- src/DocBlock/TagFactory.php | 2 +- src/DocBlock/Tags/InvalidTag.php | 72 +++++++++++++++++++ src/DocBlockFactory.php | 8 +-- .../unit/DocBlock/DescriptionFactoryTest.php | 27 +++++++ tests/unit/DocBlockFactoryTest.php | 33 --------- 7 files changed, 106 insertions(+), 48 deletions(-) create mode 100644 src/DocBlock/Tags/InvalidTag.php diff --git a/src/DocBlock/DescriptionFactory.php b/src/DocBlock/DescriptionFactory.php index 746a2c2..267e5af 100644 --- a/src/DocBlock/DescriptionFactory.php +++ b/src/DocBlock/DescriptionFactory.php @@ -69,10 +69,7 @@ class DescriptionFactory $tags = []; for ($i = 1; $i < $count; $i += 2) { - $tag = $this->tagFactory->create($tokens[$i], $context); - if ($tag !== null) { - $tags[] = $tag; - } + $tags[] = $this->tagFactory->create($tokens[$i], $context); $tokens[$i] = '%' . ++$tagCount . '$s'; } diff --git a/src/DocBlock/StandardTagFactory.php b/src/DocBlock/StandardTagFactory.php index 59caf8c..c7f5736 100644 --- a/src/DocBlock/StandardTagFactory.php +++ b/src/DocBlock/StandardTagFactory.php @@ -19,6 +19,7 @@ use phpDocumentor\Reflection\DocBlock\Tags\Covers; use phpDocumentor\Reflection\DocBlock\Tags\Deprecated; use phpDocumentor\Reflection\DocBlock\Tags\Factory\StaticMethod; use phpDocumentor\Reflection\DocBlock\Tags\Generic; +use phpDocumentor\Reflection\DocBlock\Tags\InvalidTag; use phpDocumentor\Reflection\DocBlock\Tags\Link as LinkTag; use phpDocumentor\Reflection\DocBlock\Tags\Method; use phpDocumentor\Reflection\DocBlock\Tags\Param; @@ -138,7 +139,7 @@ final class StandardTagFactory implements TagFactory /** * {@inheritDoc} */ - public function create(string $tagLine, ?TypeContext $context = null) : ?Tag + public function create(string $tagLine, ?TypeContext $context = null) : Tag { if (!$context) { $context = new TypeContext(''); @@ -215,7 +216,7 @@ final class StandardTagFactory implements TagFactory * Creates a new tag object with the given name and body or returns null if the tag name was recognized but the * body was invalid. */ - private function createTag(string $body, string $name, TypeContext $context) : ?Tag + private function createTag(string $body, string $name, TypeContext $context) : Tag { $handlerClassName = $this->findHandlerClassName($name, $context); $arguments = $this->getArgumentsForParametersFromWiring( @@ -228,7 +229,7 @@ final class StandardTagFactory implements TagFactory $callable = [$handlerClassName, 'create']; return call_user_func_array($callable, $arguments); } catch (InvalidArgumentException $e) { - return null; + return InvalidTag::create($body, $name, $e); } } diff --git a/src/DocBlock/TagFactory.php b/src/DocBlock/TagFactory.php index e52e88d..1a2dc89 100644 --- a/src/DocBlock/TagFactory.php +++ b/src/DocBlock/TagFactory.php @@ -49,7 +49,7 @@ interface TagFactory * * @throws InvalidArgumentException If an invalid tag line was presented. */ - public function create(string $tagLine, ?TypeContext $context = null) : ?Tag; + public function create(string $tagLine, ?TypeContext $context = null) : Tag; /** * Registers a service with the Service Locator using the FQCN of the class or the alias, if provided. diff --git a/src/DocBlock/Tags/InvalidTag.php b/src/DocBlock/Tags/InvalidTag.php new file mode 100644 index 0000000..f098c55 --- /dev/null +++ b/src/DocBlock/Tags/InvalidTag.php @@ -0,0 +1,72 @@ +name = $name; + $this->body = $body; + $this->throwable = $throwable; + } + + public function getException() : Throwable + { + return $this->throwable; + } + + public function getName() : string + { + return $this->name; + } + + /** + * @inheritDoc + */ + public static function create(string $body, string $name = '', ?Throwable $exception = null) + { + Assert::notNull($exception); + + return new self($name, $body, $exception); + } + + public function render(?Formatter $formatter = null) : string + { + if ($formatter === null) { + $formatter = new Formatter\PassthroughFormatter(); + } + + return $formatter->format($this); + } + + public function __toString() : string + { + return $this->body; + } +} diff --git a/src/DocBlockFactory.php b/src/DocBlockFactory.php index 3055968..1e669b5 100644 --- a/src/DocBlockFactory.php +++ b/src/DocBlockFactory.php @@ -17,7 +17,6 @@ use InvalidArgumentException; use LogicException; use phpDocumentor\Reflection\DocBlock\DescriptionFactory; use phpDocumentor\Reflection\DocBlock\StandardTagFactory; -use phpDocumentor\Reflection\DocBlock\Tag; use phpDocumentor\Reflection\DocBlock\TagFactory; use Webmozart\Assert\Assert; use function array_shift; @@ -236,12 +235,7 @@ final class DocBlockFactory implements DocBlockFactoryInterface $result = []; $lines = $this->splitTagBlockIntoTagLines($tags); foreach ($lines as $key => $tagLine) { - $tag = $this->tagFactory->create(trim($tagLine), $context); - if (!($tag instanceof Tag)) { - continue; - } - - $result[$key] = $tag; + $result[$key] = $this->tagFactory->create(trim($tagLine), $context); } return $result; diff --git a/tests/unit/DocBlock/DescriptionFactoryTest.php b/tests/unit/DocBlock/DescriptionFactoryTest.php index e10978e..3c4647f 100644 --- a/tests/unit/DocBlock/DescriptionFactoryTest.php +++ b/tests/unit/DocBlock/DescriptionFactoryTest.php @@ -13,7 +13,9 @@ declare(strict_types=1); namespace phpDocumentor\Reflection\DocBlock; +use Exception; use Mockery as m; +use phpDocumentor\Reflection\DocBlock\Tags\InvalidTag; use phpDocumentor\Reflection\DocBlock\Tags\Link as LinkTag; use phpDocumentor\Reflection\Types\Context; use PHPUnit\Framework\TestCase; @@ -162,6 +164,31 @@ DESCRIPTION; $this->assertSame($expectedDescription, $description->render()); } + /** + * @uses \phpDocumentor\Reflection\DocBlock\Description + * @uses \phpDocumentor\Reflection\DocBlock\Tags\InvalidTag + * @uses \phpDocumentor\Reflection\DocBlock\Tags\Formatter\PassthroughFormatter + * @uses \phpDocumentor\Reflection\Types\Context + * + * @covers ::__construct + * @covers ::create + */ + public function testDescriptionWithBrokenInlineTags() : void + { + $contents = 'This {@see $name} is a broken use case, but used in real life.'; + $context = new Context(''); + $tagFactory = m::mock(TagFactory::class); + $tagFactory->shouldReceive('create') + ->once() + ->with('@see $name', $context) + ->andReturn(InvalidTag::create('$name', 'see', new Exception())); + + $factory = new DescriptionFactory($tagFactory); + $description = $factory->create($contents, $context); + + $this->assertSame($contents, $description->render()); + } + /** * Provides a series of example strings that the parser should correctly interpret and return. * diff --git a/tests/unit/DocBlockFactoryTest.php b/tests/unit/DocBlockFactoryTest.php index 1c965ad..2298a01 100644 --- a/tests/unit/DocBlockFactoryTest.php +++ b/tests/unit/DocBlockFactoryTest.php @@ -279,37 +279,4 @@ DOCBLOCK $this->assertInstanceOf(DocBlock::class, $docblock); } - - /** - * @uses \phpDocumentor\Reflection\DocBlock\DescriptionFactory - * @uses \phpDocumentor\Reflection\DocBlock\Description - * - * @covers ::__construct - * @covers ::create - */ - public function testTagsAreFilteredForNullValues() : void - { - $tagString = << This is with - multiline description. -TAG; - - $tagFactory = m::mock(TagFactory::class); - $tagFactory->shouldReceive('create')->with($tagString, m::any())->andReturn(null); - - $fixture = new DocBlockFactory(new DescriptionFactory($tagFactory), $tagFactory); - - $given = << This is with - * multiline description. - */ -DOCBLOCK; - - $docblock = $fixture->create($given, new Context('')); - - $this->assertEquals([], $docblock->getTags()); - } } From 1b78639b8bc20d8dc26fa55da62caa36d197e9dc Mon Sep 17 00:00:00 2001 From: Jaapio Date: Fri, 3 Jan 2020 14:06:44 +0100 Subject: [PATCH 2/3] Add testcase for invalid tag creation --- tests/unit/DocBlock/StandardTagFactoryTest.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/DocBlock/StandardTagFactoryTest.php b/tests/unit/DocBlock/StandardTagFactoryTest.php index 5e4ee4c..25542f9 100644 --- a/tests/unit/DocBlock/StandardTagFactoryTest.php +++ b/tests/unit/DocBlock/StandardTagFactoryTest.php @@ -18,6 +18,7 @@ use phpDocumentor\Reflection\DocBlock\Tags\Author; use phpDocumentor\Reflection\DocBlock\Tags\Formatter; use phpDocumentor\Reflection\DocBlock\Tags\Formatter\PassthroughFormatter; use phpDocumentor\Reflection\DocBlock\Tags\Generic; +use phpDocumentor\Reflection\DocBlock\Tags\InvalidTag; use phpDocumentor\Reflection\DocBlock\Tags\Return_; use phpDocumentor\Reflection\DocBlock\Tags\See; use phpDocumentor\Reflection\Fqsen; @@ -330,4 +331,14 @@ class StandardTagFactoryTest extends TestCase $this->assertInstanceOf(Return_::class, $tag); $this->assertSame('return', $tag->getName()); } + + public function testInvalidTagIsReturnedOnFailure() + { + $tagFactory = new StandardTagFactory(m::mock(FqsenResolver::class)); + + /** @var InvalidTag $tag */ + $tag = $tagFactory->create('@see $name some invalid tag'); + + $this->assertInstanceOf(InvalidTag::class, $tag); + } } From bb629f6469b23ebe0470b48578e33beeeea0517e Mon Sep 17 00:00:00 2001 From: Jaapio Date: Sun, 12 Jan 2020 13:17:36 +0100 Subject: [PATCH 3/3] Improvements to code --- src/DocBlock/StandardTagFactory.php | 2 +- src/DocBlock/Tags/InvalidTag.php | 26 +++++++----- .../unit/DocBlock/StandardTagFactoryTest.php | 2 +- tests/unit/DocBlock/Tags/InvalidTagTest.php | 41 +++++++++++++++++++ 4 files changed, 59 insertions(+), 12 deletions(-) create mode 100644 tests/unit/DocBlock/Tags/InvalidTagTest.php diff --git a/src/DocBlock/StandardTagFactory.php b/src/DocBlock/StandardTagFactory.php index c7f5736..3abbc4a 100644 --- a/src/DocBlock/StandardTagFactory.php +++ b/src/DocBlock/StandardTagFactory.php @@ -229,7 +229,7 @@ final class StandardTagFactory implements TagFactory $callable = [$handlerClassName, 'create']; return call_user_func_array($callable, $arguments); } catch (InvalidArgumentException $e) { - return InvalidTag::create($body, $name, $e); + return InvalidTag::create($body, $name)->withError($e); } } diff --git a/src/DocBlock/Tags/InvalidTag.php b/src/DocBlock/Tags/InvalidTag.php index f098c55..373d665 100644 --- a/src/DocBlock/Tags/InvalidTag.php +++ b/src/DocBlock/Tags/InvalidTag.php @@ -6,7 +6,6 @@ namespace phpDocumentor\Reflection\DocBlock\Tags; use phpDocumentor\Reflection\DocBlock\Tag; use Throwable; -use Webmozart\Assert\Assert; /** * This class represents an exception during the tag creation @@ -26,17 +25,16 @@ final class InvalidTag implements Tag /** @var string */ private $body; - /** @var Throwable */ + /** @var Throwable|null */ private $throwable; - private function __construct(string $name, string $body, Throwable $throwable) + private function __construct(string $name, string $body) { - $this->name = $name; - $this->body = $body; - $this->throwable = $throwable; + $this->name = $name; + $this->body = $body; } - public function getException() : Throwable + public function getException() : ?Throwable { return $this->throwable; } @@ -47,13 +45,21 @@ final class InvalidTag implements Tag } /** + * @return self + * * @inheritDoc */ - public static function create(string $body, string $name = '', ?Throwable $exception = null) + public static function create(string $body, string $name = '') { - Assert::notNull($exception); + return new self($name, $body); + } - return new self($name, $body, $exception); + public function withError(Throwable $exception) : self + { + $tag = new self($this->name, $this->body); + $tag->throwable = $exception; + + return $tag; } public function render(?Formatter $formatter = null) : string diff --git a/tests/unit/DocBlock/StandardTagFactoryTest.php b/tests/unit/DocBlock/StandardTagFactoryTest.php index 25542f9..9235344 100644 --- a/tests/unit/DocBlock/StandardTagFactoryTest.php +++ b/tests/unit/DocBlock/StandardTagFactoryTest.php @@ -332,7 +332,7 @@ class StandardTagFactoryTest extends TestCase $this->assertSame('return', $tag->getName()); } - public function testInvalidTagIsReturnedOnFailure() + public function testInvalidTagIsReturnedOnFailure() : void { $tagFactory = new StandardTagFactory(m::mock(FqsenResolver::class)); diff --git a/tests/unit/DocBlock/Tags/InvalidTagTest.php b/tests/unit/DocBlock/Tags/InvalidTagTest.php new file mode 100644 index 0000000..2fc045b --- /dev/null +++ b/tests/unit/DocBlock/Tags/InvalidTagTest.php @@ -0,0 +1,41 @@ + + * @covers ::getName + * @covers ::render + * @covers ::getException + * @covers ::create + */ +final class InvalidTagTest extends TestCase +{ + public function testCreationWithoutError() : void + { + $tag = InvalidTag::create('Body', 'name'); + + self::assertSame('name', $tag->getName()); + self::assertSame('@name Body', $tag->render()); + self::assertNull($tag->getException()); + } + + /** + * @covers ::withError + */ + public function testCreationWithError() : void + { + $exception = new Exception(); + $tag = InvalidTag::create('Body', 'name')->withError($exception); + + self::assertSame('name', $tag->getName()); + self::assertSame('@name Body', $tag->render()); + self::assertSame($exception, $tag->getException()); + } +}