Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
weaverryan merged 1 commit intosymfony:2.1fromwouterj:refactor_optionsresolver
Jun 30, 2013

Conversation

@wouterj
Copy link
Member

QA
Doc fix?yes
New docs?no
Applies to2.1+
Fixed tickets#2353 (comment)

As requested by @bschussek, I changed the example to aMailer class. There are only 2 things I don't like:

  1. The example under"Default Values that depend on another Option" shows aport setting depending on thehost setting. I think it's a really weird example, but I can't think of a better one.
  2. I can't find a good example for thereplaceDefaults method.

@webmozart
Copy link
Contributor

Thanks again for your work on this! Some feedback below:

The $options property is an instance of :class:Symfony\\Component\\OptionsResolver\\Options

This block is actually misplaced. When you just useresolve(), you never have an Options instance. You pass in an array, you get out an array. You only deal with Options instances in closures.

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 amail() method that accesses these internal options.

Also I would renamesetDefaultOptions toconfigureOptions, since we do much more in here than just default value setting. The fact that it's calledsetDefaultOptions in the Form component is unfortunate and cannot be changed for now.

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):

  • setDefaults: This is what people will and should use most of the time, because most options are usually optional (hence the name). This method guarantees that option keys will be present in the array returned byresolve so that you don't have to worry about usingisset and things like that anymore.
  • setRequired: Used in the (rarer) occasion that an option actually has to be set by the user.
  • setOptional: Used in the (vary rare) occasion that an option should not be present in the final option array if the user did not pass it. This makes it possible to decide whether a user actually passed an option or not.

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.

Copy link
Member

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

weaverryan added a commit that referenced this pull requestJun 30, 2013
Refactored OptionsResolver docs to have a better example
@weaverryanweaverryan merged commitd29eceb intosymfony:2.1Jun 30, 2013
@wouterj
Copy link
MemberAuthor

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
Copy link
Member

Merged! Woot! Minor tweaks at sha:cbb304f - very nice work@wouterj!

@weaverryan
Copy link
Member

@wouterj sounds fine! I did miss Bernhard's comment.

@wouterjwouterj deleted the refactor_optionsresolver branchJanuary 26, 2014 14:59
weaverryan added a commit that referenced this pull requestFeb 4, 2014
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wouterj@webmozart@weaverryan@stof

[8]ページ先頭

©2009-2025 Movatter.jp