Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Refactored OptionsResolver docs to have a better example#2547
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
webmozart commentedApr 25, 2013
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": useSymfony\Component\OptionsResolver\Options;// ...protectedfunctionsetDefaultOptions(OptionsResolverInterface$resolver){// ...$resolver->setDefaults(array('encryption' =>null,'port' =>function (Options$options) {if ('ssl' ===$options['encryption']) {return465; }return25; }, ));} 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). // ...protectedfunctionsetDefaultOptions(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. |
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
wouterj commentedJun 30, 2013
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. |
weaverryan commentedJun 30, 2013
weaverryan commentedJul 1, 2013
@wouterj sounds fine! I did miss Bernhard's comment. |
This PR was merged into the 2.3 branch.Discussion----------Fixed OptionsResolver component docsI 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
Mailerclass. There are only 2 things I don't like:portsetting depending on thehostsetting. I think it's a really weird example, but I can't think of a better one.replaceDefaultsmethod.