`GuzzlePromiseAdapter::then()` breaks promise chaining by returning `$this`

[joshtrichards]

I believe this is a bug; ran across while poking around at other matters.

OC\Http\Client\GuzzlePromiseAdapter::then() calls the underlying Guzzle promise’s then(...), but discards the returned promise and returns $this instead.

This breaks normal promise chaining semantics.

server/lib/private/Http/Client/GuzzlePromiseAdapter.php

Lines 34 to 66 in 2a5e57d

	/**
	* Appends fulfillment and rejection handlers to the promise, and returns
	* a new promise resolving to the return value of the called handler.
	*
	* @param ?callable(IResponse): void $onFulfilled Invoked when the promise fulfills. Gets an \OCP\Http\Client\IResponse passed in as argument
	* @param ?callable(Exception): void $onRejected Invoked when the promise is rejected. Gets an \Exception passed in as argument
	*
	* @return IPromise
	* @since 28.0.0
	*/
	public function then(
	?callable $onFulfilled = null,
	?callable $onRejected = null,
	): IPromise {
	if ($onFulfilled !== null) {
	$wrappedOnFulfilled = static function (ResponseInterface $response) use ($onFulfilled): void {
	$onFulfilled(new Response($response));
	};
	} else {
	$wrappedOnFulfilled = null;
	}
	if ($onRejected !== null) {
	$wrappedOnRejected = static function (RequestException $e) use ($onRejected): void {
	$onRejected($e);
	};
	} else {
	$wrappedOnRejected = null;
	}
	$this->promise->then($wrappedOnFulfilled, $wrappedOnRejected);
	return $this;
	}

The current implementation also contradicts the IPromise::then() docs, which say it returns a new promise resolving to the handler’s return value.

For what it's worth, app_api is currently a consumer...

https://github.com/nextcloud/app_api/blob/c9364a51c2491c0aadc2d200b50d40e4f5c3a037/lib/Service/AppAPIService.php#L131-L149

Also could use some unit tests for async contract behavior...

[skjnldsv]

Something for the backend experts 🤔

[susnux]

@artonge something for your team maybe it seems otherwise @DaphneMuller team will run into problem with this sooner or later (app api)

[DaphneMuller]

@oleksandr-nc @julien-nc @marcelklehr @kyteinsky FYI

[kyteinsky]

OC\Http\Client\GuzzlePromiseAdapter::then() calls the underlying Guzzle promise’s then(...), but discards the returned promise and returns $this instead.

This breaks normal promise chaining semantics.

Without much knowledge of the guzzle stuff, I suppose it's done that way to use the wrapper callbacks, and returns its own $this so this wrapped method can be chained.
The promise from guzzle isn't discarded, the object protected PromiseInterface $promise, is used again on subsequent calls of this method, which is essentially chaining in PromiseInterface's context.

The current implementation also contradicts the IPromise::then() docs, which say it returns a new promise resolving to the handler’s return value.

yeah the doc is a bit misleading.