PHPUnit\Util\Annotation\DocBlock instantiation problem

[X-Coder264]

Q	A
PHPUnit version	8.5.2
PHP version	7.2.28
Installation Method	Composer

Current behavior

\PHPUnit\Util\Annotation\DocBlock::ofMethod tries to instantiate the DocBlock class, the problem is that it doesn't check if the \ReflectionMethod $method that it receives contains all the needed data that is passed to the constructor. For example I ran into a case where $method->getStartLine(), $method->getEndLine() etc returns false, but the constructor expects int and everything breaks then.

Argument 4 passed to PHPUnit\Util\Annotation\DocBlock::__construct() must be of the type integer, boolean given, called in /webroot_data/webroot/vendor/phpunit/phpunit/src/Util/Annotation/DocBlock.php on line 118

How to reproduce

Call \PHPUnit\Util\Annotation\DocBlock::ofMethod with an \ReflectionMethod instance which only has the class and name properties set.

Expected behavior

The \PHPUnit\Util\Annotation\DocBlock class should be instantiated without errors.

[sebastianbergmann]

Thank you for your report.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

[sebastianbergmann]

CC @Ocramius

[Ocramius]

$method->getStartLine() and $method->getEndLine() could return false if the sources aren't available (eval() or internal types)

[sebastianbergmann]

Ah, of course. Question is: are we ever interested in those? I doubt it. So we should just throw an exception if the class/interface/trait is built-in or eval()ed.

[X-Coder264]

This should definitely be fixed and I don't consider just throwing an exception if the class/interface/trait is built-in or eval()ed as a fix.

My test assertions all pass and instead of getting a green build, a TypeError is thrown because the Docblock class couldn't be instantiated and the build fails. Throwing an exception doesn't fix anything, I'd just get an exception back (instead of a TypeError).

[Ocramius]

A TypeError is a Throwable since PHP 7.0, so there's probably some wording confusion there.

Do you have a class leading to this behavior? Or a trace with arguments, maybe?

[X-Coder264]

@Ocramius

I run my test via vendor/bin/phpunit --filter myTest. A deprecation gets triggered by Symfony:

https://github.com/symfony/symfony/blob/v4.4.4/src/Symfony/Component/ErrorHandler/DebugClassLoader.php#L378

with the message The "Leezy\PheanstalkBundle\Event\CommandEvent" class extends "Symfony\Component\EventDispatcher\Event" that is deprecated since Symfony 4.3, use "Symfony\Contracts\EventDispatcher\Event" instead.

Symfony creates a Deprecation object here:

https://github.com/symfony/symfony/blob/v4.4.4/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php#L126 (the message argument is the same as above, the file argument is /webroot_data/webroot/vendor/symfony/symfony/src/Symfony/Component/ErrorHandler/DebugClassLoader.php

In the Deprecation constructor the line gets fetched:

https://github.com/symfony/symfony/blob/v4.4.4/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Deprecation.php#L59

and the line looks like this:

$line['file'] = '/webroot_data/webroot/vendor/phpunit/phpunit/src/Framework/TestSuite.php';
$line['function'] = 'next';
$line['class'] = 'FilterIterator';

After that $deprecation->isLegacy() gets called here:

https://github.com/symfony/symfony/blob/v4.4.4/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php#L139

which calls Test::getGroups($class, $method), true) here:

https://github.com/symfony/symfony/blob/v4.4.4/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Deprecation.php#L147

The arguments are:

$class = 'PHPUnit\Runner\Filter\NameFilterIterator';
$method = 'next';

This then ends up calling DocBlock::ofMethod here with the following arguments:

https://github.com/sebastianbergmann/phpunit/blob/8.5.2/src/Util/Annotation/Registry.php#L87

$classInHierarchy = 'PHPUnit\Runner\Filter\NameFilterIterator';
$method = 'next';

[Ocramius]

If you hook in Registry#L87, is (new \ReflectionClass(\PHPUnit\Runner\Filter\NameFilterIterator::class))->getFileName() giving you an actual path?

[X-Coder264]

It is giving me an actual path there for (new \ReflectionClass(\PHPUnit\Runner\Filter\NameFilterIterator::class))->getFileName() :

/webroot_data/webroot/vendor/phpunit/phpunit/src/Runner/Filter/NameFilterIterator.php.

[Ocramius]

Yep, seems like next is indeed declared in RecursiveFilterIterator, which is an internal class. Wondering why the DeprecationErrorHandler is querying it, but indeed, if the return value is false, we should return an empty string.

@X-Coder264 a test case would probably take an internal class (RecursiveFilterIterator is already a good candidate) and verify the behavior of DocBlock::ofMethod() when a ReflectionMethod of an internal class is given

[sebastianbergmann]

A ReflectionMethod of an internal class should never reach DocBlock::ofMethod().

[X-Coder264]

I've just finished my PR for defaulting from false to integer zero so that the DocBlock class can be instantiated (I've also added a test for this). But if it isn't gonna be merged (as @sebastianbergmann said if DocBlock::ofMethod() shouldn't even be reached for internal classes) then I won't send it. That'd mean that Symfony PHPUnit bridge would need to be the one responsible for checking that it doesn't call any code path which would lead to calling DocBlock::ofMethod() with an internal class. I'm not sure that Symfony PHPUnit bridge can even make a check to detect all such internal classes.

[Ocramius]

A ReflectionMethod of an internal class should never reach DocBlock::ofMethod().

I would say that the API, whilst indeed internal, is a non-complete function (problem), and if setting a default of 0 would suffice, we could roll with that.

The declaration of DocBlock::ofMethod() does not state any restrictions on the types passed to it, so IMO it's OK to fix it, and it's not a big burden if done correctly (and tested well)

[X-Coder264]

I would say that the API, whilst indeed internal, is a non-complete function (problem), and if setting a default of 0 would suffice, we could roll with that.

I'll have to check tomorrow at work if that is sufficient or if it is gonna break something else in the class as we have to set both start and ending line to zero and file name to an empty string (for example in the getInlineAnnotations method there is $code = \file($this->fileName); which is gonna return false when an empty string is passed and then array_slice will break a couple of lines below that as it expects an array, not a boolean).