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

[Form] Display option definition from a given form type #24208

Merged
merged 2 commits into from Oct 9, 2017

Conversation

@yceruto
Contributor

yceruto commented Sep 14, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes (deps=high failure expected)
Fixed tickets -
License MIT
Doc PR -

debug-form-option

Show friendly message if typo:
debug-form-not-found

complement of #24185

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 14, 2017

Contributor

I've used CliDumper to dump some values but the output is not colored (see Normalizer value) @nicolas-grekas am I probably missing something?

Contributor

yceruto commented Sep 14, 2017

I've used CliDumper to dump some values but the output is not colored (see Normalizer value) @nicolas-grekas am I probably missing something?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 14, 2017

Member

Color is auto-enabled only when writing to a color-enabled output stream. You can force it, but then it's your responsibility to do it appropriately, see eg
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Dumper/CliDumper.php#L85

Member

nicolas-grekas commented Sep 14, 2017

Color is auto-enabled only when writing to a color-enabled output stream. You can force it, but then it's your responsibility to do it appropriately, see eg
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Dumper/CliDumper.php#L85

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 14, 2017

Contributor

@nicolas-grekas perfect thanks! color enabled.

Contributor

yceruto commented Sep 14, 2017

@nicolas-grekas perfect thanks! color enabled.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Sep 14, 2017

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 15, 2017

Member

Is dumping a closure like the normalizer ones really useful? At least perhaps registering a dedicated caster to only keep file and line no?

Member

ogizanagi commented Sep 15, 2017

Is dumping a closure like the normalizer ones really useful? At least perhaps registering a dedicated caster to only keep file and line no?

@ogizanagi ogizanagi self-requested a review Sep 15, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 15, 2017

Contributor

Status: Needs Work

Contributor

yceruto commented Sep 15, 2017

Status: Needs Work

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 15, 2017

Contributor

Unrelated to this command argument but I added type alternatives if typo on debug:form <type>:
type-alternatives

Contributor

yceruto commented Sep 15, 2017

Unrelated to this command argument but I added type alternatives if typo on debug:form <type>:
type-alternatives

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 15, 2017

Contributor

Status: Needs Review

Contributor

yceruto commented Sep 15, 2017

Status: Needs Review

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 18, 2017

Contributor

I wondering if the use case where default values depend on another option the dumped data is clear enough?

For instance, the FormType define its lazy default for empty_data option and any other child type define its lazy default too for the same option. The definition of the default value for this option will be like this:

Screenshot (before)

debug-empty_data


I'm thinking of splitting this information in two properties "Default" and "Default lazy" to improve its understanding:
Screenshot (after)

debug-empty_data2


But I'm not sure whether it would be more confusing or not, what do you think? Perhaps adding some comment under this "Default lazy" to explain it?
Contributor

yceruto commented Sep 18, 2017

I wondering if the use case where default values depend on another option the dumped data is clear enough?

For instance, the FormType define its lazy default for empty_data option and any other child type define its lazy default too for the same option. The definition of the default value for this option will be like this:

Screenshot (before)

debug-empty_data


I'm thinking of splitting this information in two properties "Default" and "Default lazy" to improve its understanding:
Screenshot (after)

debug-empty_data2


But I'm not sure whether it would be more confusing or not, what do you think? Perhaps adding some comment under this "Default lazy" to explain it?
@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 18, 2017

Contributor

THB in both cases i have no clue what the closures mean. Either way i'd only dump closures with -v and a simple <closure> otherwise. Not sure splitting in 2 fields clarifies; if we have a lazy default then "Default: null" is simply not true.

Also please consider to expose the real default value, i.e. what's the lazy default value if we would have passed no / unrelated options to the form type. Sorry; i keep coming back to this.. hope something is possible :)

Contributor

ro0NL commented Sep 18, 2017

THB in both cases i have no clue what the closures mean. Either way i'd only dump closures with -v and a simple <closure> otherwise. Not sure splitting in 2 fields clarifies; if we have a lazy default then "Default: null" is simply not true.

Also please consider to expose the real default value, i.e. what's the lazy default value if we would have passed no / unrelated options to the form type. Sorry; i keep coming back to this.. hope something is possible :)

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 18, 2017

Contributor

Also please consider to expose the real default value, i.e. what's the lazy default value if we would have passed no / unrelated options to the form type. Sorry; i keep coming back to this.. hope something is possible :)

AFAIK not possible, because all the inputs will be string values and we wouldn't know what is the right cast as its definition could allow multiple scalar types.

if we have a lazy default then "Default: null" is simply not true.

"Default: null" is true, at the beginning of the option resolution and maybe "null" at the end, depend of what returns each configured closures: http://symfony.com/doc/current/components/options_resolver.html#default-values-that-depend-on-another-option

Either way i'd only dump closures with -v and a simple <closure> otherwise.

Same for lazy defaults #24208 (comment)

Contributor

yceruto commented Sep 18, 2017

Also please consider to expose the real default value, i.e. what's the lazy default value if we would have passed no / unrelated options to the form type. Sorry; i keep coming back to this.. hope something is possible :)

AFAIK not possible, because all the inputs will be string values and we wouldn't know what is the right cast as its definition could allow multiple scalar types.

if we have a lazy default then "Default: null" is simply not true.

"Default: null" is true, at the beginning of the option resolution and maybe "null" at the end, depend of what returns each configured closures: http://symfony.com/doc/current/components/options_resolver.html#default-values-that-depend-on-another-option

Either way i'd only dump closures with -v and a simple <closure> otherwise.

Same for lazy defaults #24208 (comment)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 18, 2017

Member

if we have a lazy default then "Default: null" is simply not true.

"Default: null" is true, at the beginning of the option resolution and maybe "null" at the end,

I agree with @ro0NL here: the default null value will never be used. The lazy closure(s) result will be. It may be null at the end, but only because the closure did return null.
Either a default value is set, either a lazy closure (or more) is used.

I'm not sure neither splitting default and default lazy would clarify much, and as said before, the Default: null does not really mean something to me.
So...what about displaying Default: default_value when a default value is set, and Default (lazy): Closure dump when a lazy closure is set?
That would also allow to differentiate a (non-lazy) closure as default value from a lazy closure.
I also think it's either a terminology or a documentation issue that the lazy term is never employed in http://symfony.com/doc/current/components/options_resolver.html#default-values-that-depend-on-another-option. The command should not have to explain this.

About dumping the closure itself: #24208 (comment) is still a good option to me. But I can provide a PR to tweak this later if you prefer.

Member

ogizanagi commented Sep 18, 2017

if we have a lazy default then "Default: null" is simply not true.

"Default: null" is true, at the beginning of the option resolution and maybe "null" at the end,

I agree with @ro0NL here: the default null value will never be used. The lazy closure(s) result will be. It may be null at the end, but only because the closure did return null.
Either a default value is set, either a lazy closure (or more) is used.

I'm not sure neither splitting default and default lazy would clarify much, and as said before, the Default: null does not really mean something to me.
So...what about displaying Default: default_value when a default value is set, and Default (lazy): Closure dump when a lazy closure is set?
That would also allow to differentiate a (non-lazy) closure as default value from a lazy closure.
I also think it's either a terminology or a documentation issue that the lazy term is never employed in http://symfony.com/doc/current/components/options_resolver.html#default-values-that-depend-on-another-option. The command should not have to explain this.

About dumping the closure itself: #24208 (comment) is still a good option to me. But I can provide a PR to tweak this later if you prefer.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 18, 2017

Contributor

Then, we need show all the possible information (that can affect this value) to solve the uncertainty.

👍 from the debugging pov, thus verbose by default.

the default null value will never be used.

Not really true actually :) https://github.com/symfony/symfony/blob/master/src/Symfony/Component/OptionsResolver/OptionsResolver.php#L168

So there can be a previous default other then null, or null set explicitly (which we cant detect).

allow to differentiate a (non-lazy) closure

👍

Contributor

ro0NL commented Sep 18, 2017

Then, we need show all the possible information (that can affect this value) to solve the uncertainty.

👍 from the debugging pov, thus verbose by default.

the default null value will never be used.

Not really true actually :) https://github.com/symfony/symfony/blob/master/src/Symfony/Component/OptionsResolver/OptionsResolver.php#L168

So there can be a previous default other then null, or null set explicitly (which we cant detect).

allow to differentiate a (non-lazy) closure

👍

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 18, 2017

Member

So there can be a previous default other then null,

Hmm, good point. I overread this line and assumed it was always reset to null if a lazy closure was given. So we need both Default and Default (lazy) in this case. WDYT?

Member

ogizanagi commented Sep 18, 2017

So there can be a previous default other then null,

Hmm, good point. I overread this line and assumed it was always reset to null if a lazy closure was given. So we need both Default and Default (lazy) in this case. WDYT?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 18, 2017

Contributor

Dont like "lazy" =/ that's all technical, while docs explain the practical and use PHP terms;

you can implement this feature by passing a closure as the default value

The argument of the callable must be type hinted as Options

So lets talk PHP/practical; Default + Default callbacks maybe?

Contributor

ro0NL commented Sep 18, 2017

Dont like "lazy" =/ that's all technical, while docs explain the practical and use PHP terms;

you can implement this feature by passing a closure as the default value

The argument of the callable must be type hinted as Options

So lets talk PHP/practical; Default + Default callbacks maybe?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 18, 2017

Contributor

allow to differentiate a (non-lazy) closure

Will happen out of the box no? It will always be the Default value.

Contributor

ro0NL commented Sep 18, 2017

allow to differentiate a (non-lazy) closure

Will happen out of the box no? It will always be the Default value.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 18, 2017

Member

Will happen out of the box no? It will always be the Default value.

If we keep a single field to display both, no: when the default value set is a closure, we won't be able to know if it's a lazy default (used by the OptionResolver component) or a real closure value (used by the form type).

Member

ogizanagi commented Sep 18, 2017

Will happen out of the box no? It will always be the Default value.

If we keep a single field to display both, no: when the default value set is a closure, we won't be able to know if it's a lazy default (used by the OptionResolver component) or a real closure value (used by the form type).

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 18, 2017

Contributor

2 fields would be explicit/consistent. Let's do that.

Default:           <mixed>
Default callbacks: [<closure>[, ...]]

naming the 2nd field is hard :) but this has my pref so far.

Contributor

ro0NL commented Sep 18, 2017

2 fields would be explicit/consistent. Let's do that.

Default:           <mixed>
Default callbacks: [<closure>[, ...]]

naming the 2nd field is hard :) but this has my pref so far.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 18, 2017

Member

@yceruto : If you like this suggestion, could you please try something like:

--------- --------------------------------
 Default   Value: <mixed>
           Closure(s): [<closure>[, ...]]
--------- --------------------------------

?

(keeping a single row for defaults, but prefixing dumps by "labels". Or even using row/colspans maybe.)

Member

ogizanagi commented Sep 18, 2017

@yceruto : If you like this suggestion, could you please try something like:

--------- --------------------------------
 Default   Value: <mixed>
           Closure(s): [<closure>[, ...]]
--------- --------------------------------

?

(keeping a single row for defaults, but prefixing dumps by "labels". Or even using row/colspans maybe.)

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 18, 2017

Contributor

@ogizanagi, @ro0NL now this look like this:

null value + lazy default

ogi

closure value + lazy default

2

Contributor

yceruto commented Sep 18, 2017

@ogizanagi, @ro0NL now this look like this:

null value + lazy default

ogi

closure value + lazy default

2

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 26, 2017

Member

I've pushed the OptionResolverIntrospector (relates to #24208 (comment)) and updated the code to use it.
I'll add tests in the OptionResolver component to ensure we don't break anything in the future, but probably in another PR. First let's agree on this to get it merged before feature freeze :)

Member

ogizanagi commented Sep 26, 2017

I've pushed the OptionResolverIntrospector (relates to #24208 (comment)) and updated the code to use it.
I'll add tests in the OptionResolver component to ensure we don't break anything in the future, but probably in another PR. First let's agree on this to get it merged before feature freeze :)

*
* @final
*/
class OptionsResolverIntrospector

This comment has been minimized.

@yceruto

yceruto Sep 26, 2017

Contributor

What do you think about move here the inner implementation of OptionsResolverWrapper? making constructor argument optional and extending from OptionsResolver?

@yceruto

yceruto Sep 26, 2017

Contributor

What do you think about move here the inner implementation of OptionsResolverWrapper? making constructor argument optional and extending from OptionsResolver?

This comment has been minimized.

@yceruto

yceruto Sep 26, 2017

Contributor

Something like this would be useful?

$optionsResolver = $optionsResolver ?: $this;
@yceruto

yceruto Sep 26, 2017

Contributor

Something like this would be useful?

$optionsResolver = $optionsResolver ?: $this;

This comment has been minimized.

@ogizanagi

ogizanagi Sep 26, 2017

Member

OptionsResolverWrapper does not serve the same purpose and aims to replace an OptionResolver instance to not fail on undefined options while configuring it, whereas the introspector just expose an API to get the state of an OptionResolver once configured. Not sure it'll be worth merging the two of them into a DebugOptionResolver.

@ogizanagi

ogizanagi Sep 26, 2017

Member

OptionsResolverWrapper does not serve the same purpose and aims to replace an OptionResolver instance to not fail on undefined options while configuring it, whereas the introspector just expose an API to get the state of an OptionResolver once configured. Not sure it'll be worth merging the two of them into a DebugOptionResolver.

This comment has been minimized.

@yceruto

yceruto Sep 26, 2017

Contributor

fair enough :(

@yceruto

yceruto Sep 26, 2017

Contributor

fair enough :(

Show outdated Hide outdated src/Symfony/Component/OptionsResolver/Debug/OptionsResolverIntrospector.php
Show outdated Hide outdated src/Symfony/Component/OptionsResolver/Debug/OptionsResolverIntrospector.php
/**
* @param string $option
*
* @return string[]

This comment has been minimized.

@yceruto

yceruto Sep 26, 2017

Contributor

Not exactly true, \Closure are allowed too, so mixed[]?

@yceruto

yceruto Sep 26, 2017

Contributor

Not exactly true, \Closure are allowed too, so mixed[]?

This comment has been minimized.

@ogizanagi

ogizanagi Sep 26, 2017

Member

\Closure are only allowed for allowed values, not allowed types, right?

@ogizanagi

ogizanagi Sep 26, 2017

Member

\Closure are only allowed for allowed values, not allowed types, right?

This comment has been minimized.

@yceruto

yceruto Sep 26, 2017

Contributor

Right, I confused it with allowedValues ;)

@yceruto

yceruto Sep 26, 2017

Contributor

Right, I confused it with allowedValues ;)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 27, 2017

Member

@nicolas-grekas: Regarding #24208 (comment), does this look okay to you? Thank you.

Member

ogizanagi commented Sep 27, 2017

@nicolas-grekas: Regarding #24208 (comment), does this look okay to you? Thank you.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 29, 2017

Member

I'm going to merge this (unless there is any objection) before feature freeze, so the whole debug:form feature is complete for 3.4.
Anyway, I'll create a PR to add tests and give more visibility to the introduced OptionResolverIntrospector hidden in this PR. We still have 2 months to tweak this if desired.

Member

ogizanagi commented Sep 29, 2017

I'm going to merge this (unless there is any objection) before feature freeze, so the whole debug:form feature is complete for 3.4.
Anyway, I'll create a PR to add tests and give more visibility to the introduced OptionResolverIntrospector hidden in this PR. We still have 2 months to tweak this if desired.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 29, 2017

Member

I'd prefer to finish everything properly instead of rushing out anything. That would not make sense. We can still merge things next week if needed. there is no hard date.

Member

fabpot commented Sep 29, 2017

I'd prefer to finish everything properly instead of rushing out anything. That would not make sense. We can still merge things next week if needed. there is no hard date.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 29, 2017

Member

Alright :)

Member

ogizanagi commented Sep 29, 2017

Alright :)

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Oct 3, 2017

Contributor

Just rebasing & resolving conflicts :)

Contributor

yceruto commented Oct 3, 2017

Just rebasing & resolving conflicts :)

@fabpot

fabpot approved these changes Oct 8, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 9, 2017

Member

Thank you @yceruto.

Member

fabpot commented Oct 9, 2017

Thank you @yceruto.

@fabpot fabpot merged commit d6d187d into symfony:3.4 Oct 9, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 9, 2017

feature #24208 [Form] Display option definition from a given form typ…
…e (yceruto, ogizanagi)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Display option definition from a given form type

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes (deps=high failure expected)
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

![debug-form-option](https://user-images.githubusercontent.com/2028198/30569305-12a30738-9ca8-11e7-98b7-6eaf78d3d5a7.png)

Show friendly message if typo:
![debug-form-not-found](https://user-images.githubusercontent.com/2028198/30450999-83d58b56-9960-11e7-8705-b60ba33baf48.png)

complement of #24185

Commits
-------

d6d187d Add & use OptionResolverIntrospector
8bbb5e7 Add debug:form type option

@yceruto yceruto deleted the yceruto:debug_form_option branch Oct 9, 2017

This was referenced Oct 18, 2017

@yceruto yceruto referenced this pull request Feb 7, 2018

Closed

Symfony form information #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment