Skip to content
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

[OptionsResolver] Add new OptionConfigurator class to define options with fluent interface #33848

Open
wants to merge 5 commits into
base: 4.4
from

Conversation

@lmillucci
Copy link

commented Oct 4, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #33735
License MIT
Doc PR symfony/symfony-docs#12426
  • submit changes to the documentation

This PR adds OptionConfigurator to the OptionsResolver

Copy link
Member

left a comment

Thanks @lmillucci for working on this feature :) I like it ! and congratulation for your first pull request.

I've left some comments to move as fast as we can, thanks again.

@yceruto yceruto added this to the next milestone Oct 4, 2019
@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

something else we may want to consider, and what we did for DI configurators as well, is to make it truly fluid;

$optionsResolver
    ->define('foo')
        ->required()
    ->define('bar')
        ->required()
;
@yceruto
yceruto approved these changes Oct 7, 2019
Copy link
Member

left a comment

With minor comments.

@yceruto

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@lmillucci thanks for your work and for solving all comments.

After the discussion we had about this #33848 (comment) where I suggest you to remove the annotation @return $this, I think we should add it again now, because it should help make it clear that it is a fluent interface by saying that each method will return the same instance of the class $this, except for the define() method that will return a new instance.

I'm sorry about that, and I hope you're okay.

@lmillucci

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

@yceruto no problem, I've easily restored @return $this annotation for all methods except define()

@yceruto
yceruto approved these changes Oct 7, 2019
@yceruto yceruto changed the title [OptionsResolver] add OptionConfigurator Issue #33735 [OptionsResolver] Add new OptionConfigurator class to define options with fluent interface Oct 9, 2019
@yceruto

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

@stof could you please review this PR again and have your vote on this ? thanks!

@yceruto

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

@yceruto yceruto force-pushed the lmillucci:33735-option-resolver branch from 3e44405 to 8326c70 Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.