Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors option normalization in the Discord class by migrating logic from manual post-processing to Symfony OptionsResolver's normalizer feature, improving code organization and maintainability.
Changes:
- Moved normalization logic for
logger,loop,intents,dnsConfig, andsocket_optionsfrom manual code after resolution to dedicated normalizers within the OptionsResolver configuration - Added comprehensive property documentation for the
$optionsarray structure including all supported option keys and their types - Updated the
dnsConfigallowed type to use the importedDnsConfigclass instead of the fully qualified class name
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Discord/Discord.php
Outdated
| ->setAllowedTypes('loop', LoopInterface::class) | ||
| ->setNormalizer('loop', fn ($options, $value) => $value ?? Loop::get()) |
There was a problem hiding this comment.
The 'loop' option needs a default value in the setDefaults() configuration. Currently, 'loop' is defined but has no default, which means if a user doesn't provide it, the option will be undefined and fail the LoopInterface type check before reaching the normalizer. Add 'loop' => null, to the setDefaults array around line 1717, and update setAllowedTypes on line 1749 to include 'null' as an allowed type: ->setAllowedTypes('loop', ['null', LoopInterface::class]).
| ->setAllowedTypes('dnsConfig', ['string', DnsConfig::class]) | ||
| ->setNormalizer('dnsConfig', function ($options, $value) { | ||
| if (null === $value) { | ||
| $value = DnsConfig::loadSystemConfigBlocking(); | ||
| if (! $value->nameservers) { | ||
| $value->nameservers[] = '8.8.8.8'; | ||
| } | ||
| } | ||
|
|
||
| return $value; | ||
| }) |
There was a problem hiding this comment.
The 'dnsConfig' option needs a default value in the setDefaults() configuration. Currently, 'dnsConfig' is defined but has no default, which means if a user doesn't provide it, the option will be undefined and the normalizer's null check won't be reached. Add 'dnsConfig' => null, to the setDefaults array around line 1717, and update setAllowedTypes on line 1790 to include 'null' as an allowed type: ->setAllowedTypes('dnsConfig', ['null', 'string', DnsConfig::class]).
src/Discord/Discord.php
Outdated
| ->setAllowedTypes('token', 'string') | ||
| ->setAllowedTypes('logger', ['null', LoggerInterface::class]) | ||
| ->setNormalizer('logger', function ($options, $value) { | ||
| if (null === $options['logger']) { |
There was a problem hiding this comment.
The normalizer is checking $options['logger'] instead of $value. In OptionsResolver normalizers, the first parameter is the full options array and the second parameter is the current value of the option being normalized. This should be if (null === $value) to correctly check if a logger was provided.
| if (null === $options['logger']) { | |
| if (null === $value) { |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request refactors how options are resolved in the
Discordclass, moving option normalization logic from manual post-processing to the SymfonyOptionsResolver's normalizer feature. This change improves code clarity, centralizes validation and normalization, and makes option handling more robust and maintainable.Refactoring and Centralization of Option Normalization:
logger,loop,intents,dnsConfig, andsocket_optionsfrom manual code at the end ofresolveOptionsto dedicated normalizers in theOptionsResolverconfiguration, ensuring all option logic is handled consistently and in one place. [1] [2] [3]Validation and Defaults Improvements:
intentsoption to ensure only valid intents are accepted and automatically converted arrays to bitwise integer values using the normalizer.dnsConfigby loading the system configuration and falling back to Google's DNS if none are found, now handled within the normalizer.socket_options['happy_eyeballs']is always set tofalseto prevent issues with IPv6, now set via the normalizer instead of manual assignment.Type and Property Documentation Updates:
optionskeys, with their types, for better IDE and developer clarity.DnsConfigclass. [1] [2]