phpgt / promise Goto Github PK

HTTPlug provides a Promise Interface so HTTP clients can be interchanged with the use of the sendRequest and sendAsyncRequest. On top of the existing functionality of this repo, only functions getState():string and wait():void need to be implemented.

Compatibility with HTTPlug means that the PHP.Gt/Fetch library will be able to use PHP.Gt/Promise without any facade to the underlying HTTPlug Promise.

Here's some example code that an end-developer would write (rather than a library developer):

use Gt\Fetch\Http;

$http = new Http();
$http->fetch("https://api.github.com/orgs/phpgt/repos")
->then(function(BodyResponse $response) {
	if(!$response->ok) {
		echo "Error fetching Github's API.";
		exit(1);
	}

	return $response->json();
})
->then(function(JsonArrayPrimitive $json) {
// $json is a pre-decoded object. Expected response is an array of Repositories,
// as per https://developer.github.com/v3/repos/#list-organization-repositories
	echo "PHP.Gt repository list:" . PHP_EOL;

	/** @var JsonKvpObject $repo */
	foreach($json->getPrimitiveValue() as $repo) {
		echo $repo->getString("name") . PHP_EOL;
	}
}, function(Throwable $exception) {
	echo "There was an error: " . $exception->getMessage() . PHP_EOL;
});

The above program will query the Github API for a list of all repositories under the phpgt organisation. When the first promise fulfills, it will resolve with a BodyResponse. The json() function will return a new promise that will fulfill with the parsed JsonObject.

The problem I've identified is when there's a chain of promises like this, when there's a rejection (for example, in the parsing of the json), the rejection callback will not fire.

Assume that $response->json() throws a JsonDecodeException in the above example. The promise will resolve, but it will not fulfill OR reject, so we will never see the "There was an error" message.

The simplified model that's in progress (from branch simple-model) introduces the need to complete promise chains. This is currently done with the complete() function, but it probably makes more sense to do this automatically from within the handler (Deferrer in reality).

Maybe a third parameter of the executor can be the completor?

This isn't exactly clear from the tests or implementation yet, so some more unit tests are required to articulate this.

At what point in a promise's resolution/rejection should it become FULFILLED or REJECTED?

Dependabot needs to be tamed, as per PhpGt/WebEngine#568

Typical fetch:

$http->fetch("http://example.com")

->then(function(Response $response) {
  if(!$response->ok) {
    return; // HERE LIES AN ISSUE
  }

  return $response->text();
})

->then(function(string $html) {
  // ...
});

Because the next promise in the chain expects a string of html, it's impossible to return nothing.

What's the proper way of returning early? Should we be able to return void at this point?

If there isn't a catch, throw any raised exceptions!

More consistent terminology, and will make more sense when combined with the upcoming await() function from PHP.Gt/Async.

In the process of simplifying the promise chain handling, I've purposefully removed the functionality that performs a type check on the catch() function parameters. This is because, having removed the unnecessary integration with HTTPlug, parity with the Web API is the most crucial priority - and the Web API doesn't handle types.

However, PHP does handle types, so I'd like to add this back in at some point. Here are the old tests that can be used:

	/*
	 * The functionality tested here is an important distinction in the PHP
	 * implemnetation, because of the type safety PHP can enforce compared
	 * to the JavaScript implementation. If there's a catch function, but
	 * the type of exception does not match the actual rejection, the
	 * rejection should be thrown to the main thread instead.
	 */
	public function testCatchRejectionHandlerIsNotCalledByTypeHintedOnRejectedCallback() {
		$exception = new RangeException();
		$promiseContainer = $this->getTestPromiseContainer();
		$sut = $promiseContainer->getPromise();

		$shouldNeverBeCalled = self::mockCallable(0);
		self::expectException(RangeException::class);

		$sut->catch(function(PromiseException $reason) use($shouldNeverBeCalled) {
			call_user_func($shouldNeverBeCalled, $reason);
		});

		$promiseContainer->reject($exception);
	}

	public function testMatchingTypedCatchRejectionHandlerCanHandleInternalTypeErrors() {
		$exception = new RangeException("No michael no!");
		$promiseContainer = $this->getTestPromiseContainer();
		$sut = $promiseContainer->getPromise();

		$onRejected1 = self::mockCallable(0);
		$onRejected2 = self::mockCallable(0);

		// There is a type error in the matching catch callback. This
		// should bubble out of the chain rather than being seen as
		// missing the RangeException type hint.
		self::expectException(TypeError::class);
		self::expectExceptionMessage("DateTime::__construct(): Argument #1 (\$datetime) must be of type string, Closure given");

		$sut->catch(function(PromiseException $reason1) use($onRejected1) {
			call_user_func($onRejected1, $reason1);
		})
		->catch(function(RangeException $reason2) use($onRejected2) {
			new DateTime(fn() => "That was so not right!");
			call_user_func($onRejected2, $reason2);
		});
		$promiseContainer->reject($exception);
	}

	public function testCatchRejectionHandlerIsCalledByAnotherMatchingTypeHintedOnRejectedCallback() {
		$exception = new RangeException();
		$promiseContainer = $this->getTestPromiseContainer();
		$sut = $promiseContainer->getPromise();

		$onRejected1 = self::mockCallable(0);
		$onRejected2 = self::mockCallable(1);

		$sut->catch(function(PromiseException $reason) use($onRejected1) {
			call_user_func($onRejected1, $reason);
		})->catch(function(RangeException $reason) use($onRejected2) {
			call_user_func($onRejected2, $reason);
		});

		$promiseContainer->reject($exception);
	}

The latest changes mean errors are being suppressed.

When debugging promises, it's really difficult to understand where the original resolutions were made, because every step of the promise's life is recorded as a separate entry.

To resolve this, when promises are resolved, they should be done on references to new objects. A bit of investigation needs to be made in order to figure out the most effective place for this to happen.

This is a stupid thing to do, but if you resolve a promise before there are any ->then() functions, it will never resolve if a then function is eventually added.

When processing the contents of a ThenChain, if the callback returns a Promise, the next Chainable should receive the resolved value, rather than the Promise itself.

In procedural code, exceptions are handled like so:

try {
    doSomething();
}
catch(Exception $e) {
   handleException($e);
}

In the world of promises, the same code can be expressed as:

$promise->then(function() {
    doSomething();
})->catch(function(Exception $e) {
    handleException($e);
});

The above example is how this library works at the moment, but there is an issue with the implementation: if calling doSomething() throws an exception and there isn't a catch() callback registered in the chain, the exception is silently gobbled up forever.

I would expect that an exception being thrown without a catch callback function should throw the exception to interrupt the main program.

There are two unit tests isolating the issue (PromiseTest::testCatchMethodNotBubblesThrowables passes, PromiseTest::testNoCatchMethodBubblesThrowables fails), but in short, the following code should work:

try {
    $promise->then(function() {
        doSomething();
    });
}
catch(Exception $e) {
    // This line never gets reached!
}