Create OptionsResolver normalizers#1455
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
This should be good to merge, but the actual benefits it would have are unclear. I'd rather not touch something that isn't broken and doesn't actually change the functionality in any meaningful way. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @property array $options | ||
| * @property string $options['token'] | ||
| * @property LoggerInterface $options['logger'] | ||
| * @property LoopInterface $options['loop'] | ||
| * @property array|bool $options['loadAllMembers'] | ||
| * @property array $options['disabledEvents'] | ||
| * @property bool $options['storeMessages'] | ||
| * @property array|bool $options['retrieveBans'] | ||
| * @property int|null $options['large_threshold'] | ||
| * @property array|null $options['shard'] | ||
| * @property int|null $options['shard_id'] | ||
| * @property int|null $options['num_shards'] | ||
| * @property int|null $options['shardId'] | ||
| * @property int|null $options['shardCount'] | ||
| * @property array|null $options['presence'] | ||
| * @property int $options['intents'] | ||
| * @property array $options['socket_options'] | ||
| * @property DnsConfig|string $options['dnsConfig'] | ||
| * @property array $options['cache'] | ||
| * @property string $options['collection'] | ||
| * @property bool $options['useTransportCompression'] |
There was a problem hiding this comment.
The PHPDoc uses @property tags with array offsets (e.g. $options['token']). This syntax isn’t understood by most PHPDoc parsers (including phpDocumentor), so it’s likely to be ignored or mis-parsed. If the goal is to document the options array shape for tooling, consider using an @var array{...} shape (or a @phpstan-type/@psalm-type) instead of @property entries for offsets.
| * @property array $options | |
| * @property string $options['token'] | |
| * @property LoggerInterface $options['logger'] | |
| * @property LoopInterface $options['loop'] | |
| * @property array|bool $options['loadAllMembers'] | |
| * @property array $options['disabledEvents'] | |
| * @property bool $options['storeMessages'] | |
| * @property array|bool $options['retrieveBans'] | |
| * @property int|null $options['large_threshold'] | |
| * @property array|null $options['shard'] | |
| * @property int|null $options['shard_id'] | |
| * @property int|null $options['num_shards'] | |
| * @property int|null $options['shardId'] | |
| * @property int|null $options['shardCount'] | |
| * @property array|null $options['presence'] | |
| * @property int $options['intents'] | |
| * @property array $options['socket_options'] | |
| * @property DnsConfig|string $options['dnsConfig'] | |
| * @property array $options['cache'] | |
| * @property string $options['collection'] | |
| * @property bool $options['useTransportCompression'] | |
| * @property array{ | |
| * token: string, | |
| * logger: LoggerInterface, | |
| * loop: LoopInterface, | |
| * loadAllMembers: array|bool, | |
| * disabledEvents: array, | |
| * storeMessages: bool, | |
| * retrieveBans: array|bool, | |
| * large_threshold: int|null, | |
| * shard: array|null, | |
| * shard_id: int|null, | |
| * num_shards: int|null, | |
| * shardId: int|null, | |
| * shardCount: int|null, | |
| * presence: array|null, | |
| * intents: int, | |
| * socket_options: array, | |
| * dnsConfig: DnsConfig|string, | |
| * cache: array, | |
| * collection: string, | |
| * useTransportCompression: bool | |
| * } $options |
| 'loop' => Loop::get(), | ||
| 'logger' => new Monolog('DiscordPHP', [(new StreamHandler('php://stdout', Level::Debug))->setFormatter(new LineFormatter(null, null, true, true))]), |
There was a problem hiding this comment.
loop and logger now have concrete defaults set in the resolver, but the function still contains post-resolve() fallback logic for these options later on. Keeping both paths makes the resolution behavior harder to reason about and risks future divergence; consider removing the redundant post-processing and letting the resolver defaults/normalizers be the single source of truth.
| 'loop' => Loop::get(), | |
| 'logger' => new Monolog('DiscordPHP', [(new StreamHandler('php://stdout', Level::Debug))->setFormatter(new LineFormatter(null, null, true, true))]), | |
| 'loop' => null, | |
| 'logger' => null, |
| ->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'; | ||
| } |
There was a problem hiding this comment.
The dnsConfig normalizer only runs when the caller explicitly sets the option; since dnsConfig has no default in the resolver, omitting it will bypass this normalizer and rely on separate manual initialization later in resolveOptions(). To fully centralize normalization, consider setting a default (e.g. null) so the normalizer always executes, and then remove the later manual dnsConfig fallback block.
| ->setAllowedTypes('collection', 'string') | ||
| ->setNormalizer('collection', function ($options, $value) { | ||
| if (is_string($value) && class_exists($value) && is_subclass_of($value, ExCollectionInterface::class)) { | ||
| return $value; | ||
| } | ||
|
|
||
| return Collection::class; | ||
| }) |
There was a problem hiding this comment.
The newly added collection allowed-types + normalizer block duplicates an existing collection configuration later in this method (there’s already another setAllowedTypes('collection', ...) + normalizer below). This results in the same option being configured twice and makes future changes error-prone; keep only a single collection configuration block.
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]