Skip to content

[11.x] Http client: record request when faking connection exception#53530

Merged
taylorotwell merged 3 commits intolaravel:11.xfrom
gdebrauwer:record-requests-when-using-fake-connection-exception
Nov 15, 2024
Merged

[11.x] Http client: record request when faking connection exception#53530
taylorotwell merged 3 commits intolaravel:11.xfrom
gdebrauwer:record-requests-when-using-fake-connection-exception

Conversation

@gdebrauwer
Copy link
Copy Markdown
Contributor

I added support for faking a connection exception in a test (see #53485), but I noticed that the request was not being recorded. That means asserting how many requests were sent is currently not correct. This PR fixes that.

Http::fake(['https://laravel.com' => Http::failedConnection()]);

try {
    Http::get('https://laravel.com')
} catch (ConnectionException) {
}

Http::assertSentCount(1); // This would fail before this PR

@StSarc
Copy link
Copy Markdown
Contributor

StSarc commented Jan 2, 2025

@gdebrauwer We also need to handle $this->factory = null

                $this->factory?->recordRequestResponsePair($request, null);

$factory has been initialized as:

/**
     * The factory instance.
     *
     * @var \Illuminate\Http\Client\Factory|null
     */
    protected $factory;

$factory can be null.

In the buildRecorderHandler function, $this->factory = null is already handled.

return $promise->then(function ($response) use ($request, $options) {
                    $this->factory?->recordRequestResponsePair(
                        (new Request($request))->withData($options['laravel_data']),
                        $this->newResponse($response)
                    );

                    return $response;
                });

@taylorotwell tagging you just to ask if it's okay if I raise a PR for this. It's a minor change.

@gdebrauwer
Copy link
Copy Markdown
Contributor Author

@StSarc I'm not sure when the $factory property can be null 🤔 I would just create the PR with the minor change and see what Taylor thinks about it (I don't think he gets a notification on a comment in merged pr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants