Make Voice gateway client an external dependency #1436
Conversation
It's public now
|
This PR will need to |
valzargaming
added
important
features
dependencies
danielhe4rt
left a comment
danielhe4rt
left a comment
There was a problem hiding this comment.
Thank god for you adding docblocks everywhere around.
That is something that Rust has and after I saw it I really wanted to see more in PHP.
Left some comments, see if it make any sense.
| if (class_exists(Manager::class, false)) { | ||
| $this->voice = new Manager($this); | ||
| $this->logger->info('voice class initialized'); | ||
| } |
There was a problem hiding this comment.
This isn't a better approach? Since it's a 3rd party dependency, using the namespace directly on the "class_exists" method IMHO is a better standard.
| if (class_exists(Manager::class, false)) { | |
| $this->voice = new Manager($this); | |
| $this->logger->info('voice class initialized'); | |
| } | |
| if (class_exists(\Discord\Voice\Manager::class, false)) { | |
| $this->voice = new Manager($this); | |
| $this->logger->info('voice class initialized'); | |
| } |
There was a problem hiding this comment.
The manager class is added as an import at the top of the file, so this is effectively the same thing. To reduce redundancy there is no need to type out the entire namespace, and if it ever changed in the future it would then need to be updated in multiple places rather than just in the import.
The only reason this is now an external repo is because the voice API version can and will differ from the regular API version, and we want to have proper version control and guarantee things won't break for users who need to upgrade in the future.
There was a problem hiding this comment.
The class is not 3rd party, it's also managed/maintained by us.
| static $allowed = ['loop', 'options', 'logger', 'http', 'application_commands', 'voice_sessions']; | ||
| static $allowed = ['loop', 'options', 'logger', 'http', 'application_commands']; |
There was a problem hiding this comment.
What effectively changes here? What is the other way to get the voice_sessions?
There was a problem hiding this comment.
This was originally made because I believed that simply getting it through the magic method was sufficient, however because it was a protected variable and was also needing to be modified attempting to do so would fail. This property is now public to resolve that, and also so it can be used in areas outside of the voice client where a user may just want to know what voice sessions are active without needing the client to be processing voice data. For example, an interaction on a message that should only be usable by users who are in a voice call with the bot.
There was a problem hiding this comment.
it's going to be within the voice package i believe.
There was a problem hiding this comment.
Correct, it is used for voice session management. If one already exists we need to use the existing session, otherwise we need to manage a new one.
If we overwrite it we won't get useful data logged in the first warning
This pull request introduces support for the
discord-php-helpers/voicepackage and updates the instantiation of theVoiceClientclass to align with the new helper package's interface. The main focus is on integrating the new voice helper and simplifying theVoiceClientconstructor usage.Dependency update:
discord-php-helpers/voiceas a dependency incomposer.jsonto enable new voice-related features.Voice client integration:
VoiceClientinstantiation inDiscord.phpto use the new constructor signature, removing now-unnecessary parameters and aligning with the helper package's API.