Skip to content

TypeError: Argument 1 passed to ProfileClient::collectExceptionInformations() must be an instance of Exception #353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ The change log describes what is "Added", "Removed", "Changed" or "Fixed" betwee

- Configured clients are now tagged with `'httplug.client'`

### Changed

- Fixed error handling. Now TypeErrors and other \Throwable are correctly handled by ProfileClient

## 1.16.0 - 2019-06-05

### Changed
Expand Down
4 changes: 2 additions & 2 deletions src/Collector/Formatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ public function __construct(MessageFormatter $formatter, CurlCommandFormatter $c
/**
* Formats an exception.
*
* @param Exception $exception
* @param \Throwable $exception
*
* @return string
*/
public function formatException(Exception $exception)
public function formatException(\Throwable $exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a BC break because the class is internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even more, Throwable is wide variation of Exception, so no BC break will happen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But that reason alone is still a BC break because the class is not final.

{
if ($exception instanceof HttpException) {
return $this->formatter->formatResponse($exception->getResponse());
Expand Down
4 changes: 2 additions & 2 deletions src/Collector/ProfileClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,11 @@ private function collectResponseInformations(ResponseInterface $response, Stopwa
}

/**
* @param \Exception $exception
* @param \Throwable $exception
* @param StopwatchEvent $event
* @param Stack $stack
*/
private function collectExceptionInformations(\Exception $exception, StopwatchEvent $event, Stack $stack)
private function collectExceptionInformations(\Throwable $exception, StopwatchEvent $event, Stack $stack)
{
if ($exception instanceof HttpException) {
$this->collectResponseInformations($exception->getResponse(), $event, $stack);
Expand Down
36 changes: 29 additions & 7 deletions tests/Unit/Collector/ProfileClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Http\Promise\FulfilledPromise;
use Http\Promise\Promise;
use Http\Promise\RejectedPromise;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
Expand All @@ -34,7 +35,7 @@ class ProfileClientTest extends TestCase
private $activeStack;

/**
* @var HttpClient
* @var HttpClient|MockObject
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right.

just a comment, no need to change anything now: i have seen the syntax HttpClient&MockObject which is semantically correct and needed from some phpstan level on so phpstan is sure about the type of the variable. however, e.g. phpstorm seems not to know about it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it's right, PhpStorm don't suggest any method of MockObject so I should to add this,

*/
private $client;

Expand All @@ -44,7 +45,7 @@ class ProfileClientTest extends TestCase
private $request;

/**
* @var Formatter
* @var Formatter|MockObject
*/
private $formatter;

Expand Down Expand Up @@ -110,11 +111,6 @@ public function setUp(): void
->with($this->response)
->willReturn('FormattedResponse')
;
$this->formatter
->method('formatException')
->with($this->exception)
->willReturn('FormattedException')
;

$this->stopwatch
->method('start')
Expand Down Expand Up @@ -145,6 +141,26 @@ public function testSendRequest(): void
$this->assertEquals('https', $this->activeStack->getRequestScheme());
}

/**
* @expectedException \Error
* @expectedException "You set string to int prop"
*/
public function testSendRequestTypeError()
{
$this->client
->expects($this->once())
->method('sendRequest')
->willReturnCallback(function () {
throw new \Error('You set string to int prop');
});
$this->formatter
->expects($this->once())
->method('formatException')
->with($this->isInstanceOf(\Error::class));

$this->subject->sendRequest($this->request);
}

public function testSendAsyncRequest(): void
{
$this->client
Expand Down Expand Up @@ -211,6 +227,12 @@ public function testOnRejected(): void
->willReturn($this->rejectedPromise)
;

$this->formatter
->method('formatException')
->with($this->exception)
->willReturn('FormattedException')
;

$this->subject->sendAsyncRequest($this->request);

$this->assertEquals(42, $this->activeStack->getDuration());
Expand Down