[12.x] PendingRequest HTTP methods may also return promises

[cosmastech]

I'm doing my first foray into HTTP pooling, and wow, this is dope.

Because of the typehints, I wasn't aware something like this was possible:

class FetchShopifyProductFactory
{
    public function buildPendingRequest(int $productId, ?PendingRequest $pendingRequest = null)
    {
        $pendingRequest = $pendingRequest ?? Http::createPendingRequest();

        return $pendingRequest
            ->async()
            ->get("orders/{$productId}.json")
            ->then(function (Response|RequestException|ConnectionException $r) {
                if ($r instanceof \Throwable) {
                    return $r;
                }

                return ShopifyProduct::from($r->json());
            });
    }
}

$fetchShopifyProductFactory  = new FetchShopifyProductFactory;

/** @var array<string, ShopifyProduct|RequestException|ConnectionException> $results */
$results = Http::pool(static function (Pool $pool) use ($fetchShopifyProductFactory) {
    $fetchShopifyProductFactory->buildPendingRequest(123, $pool->as('product_123'));
    $fetchShopifyProductFactory->buildPendingRequest(867, $pool->as('product_876'));
});

$results['product_123'] instanceof ShopifyProduct; // true

// or
$fetchShopifyProductFactory->buildPendingRequest(123)->wait() instanceof ShopifyProduct; // true

[ManuelLeiner]

@cosmastech speaking of typehints.

The basic example from the documentation fails a larastan/larastan check now.

Http::post(/* ... */)->throw()->json();

=>   :76    Call to an undefined method GuzzleHttp\Promise\PromiseInterface|Illuminate\Http\Client\Response::throw().  
         🪪  method.notFound   

I hope our CI pipeline is the only one out there that crashes 😄

[cosmastech]

@cosmastech speaking of typehints.

The basic example from the documentation fails a larastan/larastan check now.

Http::post(/* ... */)->throw()->json();



=>   :76    Call to an undefined method GuzzleHttp\Promise\PromiseInterface|Illuminate\Http\Client\Response::throw().  

         🪪  method.notFound   

I hope our CI pipeline is the only one out there that crashes 😄

Nah, you're not. We had that issue this morning as well.

I wonder if there's something with Larastan we could add to have it be more intelligent about when it returns a Promise vs when it returns a Response.

[shaedrich]

Is there something missing in @return? All the methods where the union type was added, call send():

framework/src/Illuminate/Http/Client/PendingRequest.php

Lines 936 to 947 in a78e156

	/**
	* Send the request to the given URL.
	*
	* @param string $method
	* @param string $url
	* @param array $options
	* @return \Illuminate\Http\Client\Response
	*
	* @throws \Exception
	* @throws \Illuminate\Http\Client\ConnectionException
	*/
	public function send(string $method, string $url, array $options = [])

-      * @return \Illuminate\Http\Client\Response
+      * @return \Illuminate\Http\Client\Response|\GuzzleHttp\Promise\PromiseInterface

[cosmastech]

Is there something missing in @return? All the methods where the union type was added, call send():

framework/src/Illuminate/Http/Client/PendingRequest.php

Lines 936 to 947 in a78e156

	/**
	* Send the request to the given URL.
	*
	* @param string $method
	* @param string $url
	* @param array $options
	* @return \Illuminate\Http\Client\Response
	*
	* @throws \Exception
	* @throws \Illuminate\Http\Client\ConnectionException
	*/
	public function send(string $method, string $url, array $options = [])

-      * @return \Illuminate\Http\Client\Response
+      * @return \Illuminate\Http\Client\Response|\GuzzleHttp\Promise\PromiseInterface

Yes, and I added that in this PR: https://github.com/laravel/framework/pull/57973/files#diff-1ac9aa0ab55e04c899a86ef074016e5ebc3683e3e90101f03284305301771067R955

That said, the problem is that this PR "fixed" the return type to be technically correct, but lots of code written previously was assuming that these methods returned ONLY a Response.

So the code shared above works fine, but static analysis gripes

[chris-ware]

I believe there is actually a bigger problem than meets the eye here. Yes, PHPStan moans, which is incredibly annoying, especially to actually fix it in app code. However, it is completely correct.

Http::get('https://artisan.page/api/versions')->json()
// the get method in this instance returns \Illuminate\Http\Client\Response and thus fluent methods can be called on it

Http::async()->get('https://artisan.page/api/versions')->json()
// the get method in this instance returnsIlluminate\Http\Client\FluentPromise, which doesn't have any fluent methods on it, as it isn't really designed to be run this way, and yet it can be.

If I had any idea on a path forward for this, I would put a PR together, but I don't have a clue. My only thoughts are that perhaps async should return a modified version of the PendingRequest class, to separate the logic and capabilities?

[cosmastech]

If I had any idea on a path forward for this, I would put a PR together, but I don't have a clue. My only thoughts are that perhaps async should return a modified version of the PendingRequest class, to separate the logic and capabilities?

I agree with this: there should be two classes - one for async and the current one for sync.

I had the same idea in my head, but haven't played around with trying to change this for Laravel 13.

[LauJosefsen]

I just want to add our voices to this discussion as well.

It is more or less mandatory to have static analysis in PHP in enterprise solutions to avoid costly errors. Yet this change forces us to either ignore this error, or significantly reduce the comfort of using the Http client.

For example this code

$response = Http::asForm()->post(...)->throw();

becomes this code

$response = Http::asForm()->post(...)

if ($response instanceof Response) {
    $response->throw();
} else {
    throw new \RuntimeException('Unexpected response type');
}

Not to mention changing a return type probably should have been included in a major?

[canvural]

Not to mention changing a return type probably should have been included in a major?

Oh boy. Do you think Laravel cares about that? 😂