-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored OptionsResolver docs to have a better example #2547
Conversation
Thanks again for your work on this! Some feedback below:
This block is actually misplaced. When you just use Next, while adding getters for options is a possibility, I wouldn't recommend it as a default showcase. As we are describing a service, it would make more sense to show a Also I would rename The blocks about optional options and default values are a bit misleading. Options with default values are optional too. The documentation suggests that I need to call both which is not true. I would restructure the docs to describe (in this order):
When it comes to dependent default values, I'd use the examples "encryption" and "port": use Symfony\Component\OptionsResolver\Options;
// ...
protected function setDefaultOptions(OptionsResolverInterface $resolver)
{
// ...
$resolver->setDefaults(array(
'encryption' => null,
'port' => function (Options $options) {
if ('ssl' === $options['encryption']) {
return 465;
}
return 25;
},
));
} Here may also be the appropriate place to describe why the options in the closure are an Options instance and not an array. For the allowed default values, I'd use the example "encryption" again (to avoid introducing another example). // ...
protected function setDefaultOptions(OptionsResolverInterface $resolver)
{
// ...
$resolver->setAllowedValues(array(
'encryption' => array(null, 'ssl', 'tls'),
));
} The last example about normalizing the host is probably wrong. Host names for mailing usually don't start with 'http://', since the protocol is SMTP, not HTTP. I've scanned the Swiftmailer configuration for a bit but I'm still lacking a better example for the mailer case. |
@@ -276,7 +269,7 @@ Normalize the Options | |||
--------------------- | |||
|
|||
Some values need to be normalized before you can use them. For instance, the | |||
``firstName`` should always start with an uppercase letter. To do that, you can | |||
``host`` should always start ``http://``. To do that, you can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition looks really weird to me given that you are configuring a mailer. You may want to find a better normalization example
Refactored OptionsResolver docs to have a better example
This actually wasn't ready to merge yet, I haven't had the time to implement @bschussek's suggestions. I'll make a new PR soon to fix those things. |
@wouterj sounds fine! I did miss Bernhard's comment. |
This PR was merged into the 2.3 branch. Discussion ---------- Fixed OptionsResolver component docs I finally applied the comments of #2547 (comment) | Q | A | --- | --- | Doc fix? | yes | New docs? | no | Applies to | 2.1+ | Fixed tickets | #2547 (comment) Commits ------- 0e9a474 Applied fixes by @bschussek
As requested by @bschussek, I changed the example to a
Mailer
class. There are only 2 things I don't like:port
setting depending on thehost
setting. I think it's a really weird example, but I can't think of a better one.replaceDefaults
method.