Promise v3

[key2peace]

No description provided.

[2colours]

@key2peace what is the state of this PR? Being stuck with v2 Promises (thanks, Composer, for not handling multiple versions of one package btw) means even something as banal as set_rejection_handler cannot be used.

[SQKo]

@key2peace what is the state of this PR? Being stuck with v2 Promises (thanks, Composer, for not handling multiple versions of one package btw) means even something as banal as set_rejection_handler cannot be used.

It wont be in v10, many dependencies still stuck with v2 promises
As well compatibility issue with future php 8.4 version, which they'll promise to support by reactphp v3

about set_rejection_handle, Just be careful with your promise codes for now
always use then() followed by argumentless and empty done() so you can refactor your code easier in future

[2colours]

@SQKo DiscordPHP should at the very least handle the rejection path with a logger, with a log level >= warning. There is no reason to not do that since afterwards nobody could even catch the rejection currently. I think that's way better than leaving it up to the consumers to develop some quasi framework that minimizes the boilerplate for handling this for each and every registered command.

I'm already looking into the code; it's just not easy to navigate around with something you have only been actively using for like 2 months...

[2colours]

see #1208

This is anything but exhaustive, it just covers my most obvious use case.

[SQKo]

@SQKo DiscordPHP should at the very least handle the rejection path with a logger, with a log level >= warning. There is no reason to not do that since afterwards nobody could even catch the rejection currently. I think that's way better than leaving it up to the consumers to develop some quasi framework that minimizes the boilerplate for handling this for each and every registered command.

I'm already looking into the code; it's just not easy to navigate around with something you have only been actively using for like 2 months...

We know about it already, unfortunately the issue we are facing might be also caused by dependencies, our code is fine when web socket is not connected yet..

Regarding to your suggestion, we already considered about it, but it is wrong to log everything even those promise rejections already handled/caught by users and not respecting users configured Logger.
So we went to a better idea, It is somewhat already implemented but if i recall, it requires your code to be wrapped inside a coroutine yields so your own code throws will be also forwarded:

DiscordPHP/src/Discord/Discord.php

Lines 793 to 799 in ac16fdf

	if ($e instanceof \Error) {
	throw $e;
	} elseif ($e instanceof \Exception) {
	$this->logger->error('exception while trying to handle dispatch packet', ['packet' => $data->t, 'exception' => $e]);
	} else {
	$this->logger->warning('rejection while trying to handle dispatch packet', ['packet' => $data->t, 'rejection' => $e]);
	}

notice why it is logged only in websocket events because it only gets silence after websocket connected...

Due to the nature of promise chain, where this library being in middle position, the ideal solution has to be implemented on user's end.
It's not true that nobody can even catch the rejection, thats why I previously remind the proper use of then() and done(). Same for this library perspective, if the dependency does not handle promise rejection properly, it can never be caught by us.

[valzargaming]

wyrihaximus/react-cache-redis is currently locked to ^3.0 || >=4.0 <4.4 and we are required to use at least ^4.5.0 to include react/promise ^3.1. This is considered to be a core feature of the v10 library to support external caching and must be tested before this can be included as part of a new release.

[valzargaming]

Tested with Civilizationbot and wyrihaximus/react-cache-redis ^4.5 and all is well. This introduces BC for React\Promise\resolve and React\Promise\reject and values must be passed for both: value for resolve (even null is fine) and a \Throwable for reject. Similarly v3 has deprecated functions like React\Promise\map and userland implementations will need to use React\Promise\all or React\Promise\any instead.

[valzargaming]

All Instances of $promise->then need to take parameters (callable $onFulfilled, callable $onRejected); Let's get these functions either defined as $variable = function()s or make some generic handlers we can throw in that will make relevant calls to Monolog

[valzargaming]

Per the release notes; It is no longer possible to resolve a promise without a value (use null instead) or reject a promise without a reason (use Throwable instead).

[valzargaming]

Instead of resolve(true) let's change these to resolve(null) as recommended per the release notes: https://github.com/reactphp/promise/releases/tag/v3.0.0