discord-php / DiscordPHP

An API to interact with the popular messaging app Discord
MIT License
985 stars 236 forks source link

Event Loop Being Overwritten #970

Closed ellisonpatterson closed 1 year ago

ellisonpatterson commented 1 year ago

Environment

Describe the bug In the initialization for the event loop, you are calling \React\EventLoop\Factory::create() which overwrites the custom loop one can set in the configuration. Using this method is also deprecated as the notes list. https://github.com/reactphp/event-loop/blob/3992edd97694303260ac87b85f86ff45cc92feea/src/Factory.php#L13-L41

It needs to be changed from using \React\EventLoop\Factory::create() to \React\EventLoop\Loop::get() so a second loop isn't created by DiscordPHP.

To Reproduce Steps to reproduce the behavior:

use React\EventLoop\Loop;

var_dump('New loop instance: ' . spl_object_id(Loop::get()));

new \Discord\Discord([
    'token' => $_ENV['DISCORD_TOKEN'],
    'loop' => Loop::get()
]);

var_dump('Same loop instance: ' . spl_object_id(Loop::get()));

This outputs that the loop I passed in was overwritten:

discord@roc-hosting:~/cliapps$ php development.php 
string(20) "New loop instance: 9"
[2022-11-08T19:54:14.414131+00:00] DiscordPHP.DEBUG: Initializing DiscordPHP v7.3.2 (DiscordPHP-Http: v9.1.6 & Gateway: v9) on PHP 8.1.11 [] []
[2022-11-08T19:54:14.427053+00:00] DiscordPHP.DEBUG: BUCKET getoauth2/applications/@me queued REQ GET oauth2/applications/@me [] []
[2022-11-08T19:54:14.431185+00:00] DiscordPHP.DEBUG: BUCKET getgateway/bot queued REQ GET gateway/bot [] []
string(22) "Same loop instance: 55"
SQKo commented 1 year ago

It is already fixed in dev-master / v10 but not in v7.x because the react loop was never a direct dependency and react making this breaking change from a 1.1 to a 1.2 which is not a major version, suppose i fix this by locking to react loop 1.1 but if the dependency was never there it means doing so is a breaking change and requires bump to major version. So this was not mistake on our side.

Solution: dont use Loop::get(), avoid the static call, store and pass your loop object instead or use $discord->getLoop().

I will probably set loop to null in 7.4.x (not as a fix) but requires user to explicitly set Loop in their options, which is not really breaking change if they have react/loop 1.2+ installed already

Edit: I might detect for the new Loop class exists in the react loop to detect if user has <1.1 or >1.2

SQKo commented 1 year ago

Fixed in 7.3.3 6d76ad8fbf320e97304f6d04cc9f5894e2fed023

ellisonpatterson commented 1 year ago

I already was passing in a loop I created without the static helper, but it was still overriding it.

Thank you for the quick fix!