Skip to content
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

[10.x] Fix Http client pool return type #47530

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Conversation

miguilimzero
Copy link
Contributor

@miguilimzero miguilimzero commented Jun 22, 2023

The current docblock code presumes that any method by PendingRequest return itself, however, this is not the case, even for simple scenarios:

Http::pool(fn (Pool $pool) => [
    $pool->get('http://localhost/first'), // Returns Promise and not PendingRequest
    $pool->get('http://localhost/second'), // Returns Promise and not PendingRequest
    $pool->get('http://localhost/third'), // Returns Promise and not PendingRequest
]);

Any method that represents creating an async request will return a Promise class: get, head, post, patch, put, and delete. Only methods to configure the request will return the class itself, ex: withOptions, retry, timeout.

When using a static analysis tool and creating a function to mock a pool request is where the problem comes. Here is an example to demonstrate it:

public function prepareMyPoolRequest(Pool $pool, string $endpoint): Promise // This is the correct return type, however when running Psalm, it will throw an error saying that the ->get() method returns a PendingRequest.
{
     $baseUrl = '...';
     
     return $pool->get($baseUrl.$endpoint);
}

This pull request adds \GuzzleHttp\Promise\Promise to the __call magic method from Pool class for async requests. This will create a union, and any method called (according to the new docblock) may return any of the two classes. This may generate some uncertainty in some static analysis cases.

A better way to patch this, in the static analysis view, would be creating the methods get, head, post, patch, put, and delete on the Pool class, enforcing that they will return a Promise instance and all other methods will fallback to the __call magic method that says it returns a PendingRequest instance, which is now correct. But this will lead to some new lines of code instead of just adjusting a docblock value.

@taylorotwell taylorotwell merged commit b4ab629 into laravel:10.x Jun 22, 2023
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.

2 participants