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] Add debug:form command #23694

Merged
merged 1 commit into from Aug 30, 2017

Conversation

@yceruto
Contributor

yceruto commented Jul 27, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23688
License MIT
Doc PR -

debug-form

A short class name (e.g. DateType) can be passed as class argument too (the command will try to resolve its FQCN if it's in known form type namespaces).

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jul 28, 2017

Contributor

I'm thinking in more options to avoid type the FQCN, so what about to search the form type inside registered types or installed bundles?

$ bin/console debug:form EasyAdminFormType

if there is more than one registered with the same suffix, we could display the first and print at the bottom a comment with another matches?

Contributor

yceruto commented Jul 28, 2017

I'm thinking in more options to avoid type the FQCN, so what about to search the form type inside registered types or installed bundles?

$ bin/console debug:form EasyAdminFormType

if there is more than one registered with the same suffix, we could display the first and print at the bottom a comment with another matches?

@yceruto yceruto changed the title from [FrameworkBundle] Add debug:form command to [Form] Add debug:form command Jul 28, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jul 28, 2017

Contributor

Failure on deps=high is normal.

Contributor

yceruto commented Jul 28, 2017

Failure on deps=high is normal.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Jul 28, 2017

Member

I'm thinking in more options to avoid type the FQCN, so what about to search the form type inside registered types or installed bundles?

At first, I'd suggest to add the list of "preferred" namespaces to use as an optional constructor argument of the command.
Then, we might either:

  1. Populate it with only core namepaces (form component types + doctrine bridge as you did here) and let bundles or users add their own.
  2. Or hook in the FormPass to gather namespaces for all known types.

if there is more than one registered with the same suffix, we could display the first and print at the bottom a comment with another matches?

If there is more than one match let's show a message saying the argument is ambiguous.
In interactive mode, ask the user to choose the it from a list using a choice question.
In non interactive mode, I think we should only display the list of matches without their information.

Member

ogizanagi commented Jul 28, 2017

I'm thinking in more options to avoid type the FQCN, so what about to search the form type inside registered types or installed bundles?

At first, I'd suggest to add the list of "preferred" namespaces to use as an optional constructor argument of the command.
Then, we might either:

  1. Populate it with only core namepaces (form component types + doctrine bridge as you did here) and let bundles or users add their own.
  2. Or hook in the FormPass to gather namespaces for all known types.

if there is more than one registered with the same suffix, we could display the first and print at the bottom a comment with another matches?

If there is more than one match let's show a message saying the argument is ambiguous.
In interactive mode, ask the user to choose the it from a list using a choice question.
In non interactive mode, I think we should only display the list of matches without their information.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jul 28, 2017

Contributor

@ogizanagi Sorry, I didn't see your last comment before submit these changes.

Contributor

yceruto commented Jul 28, 2017

@ogizanagi Sorry, I didn't see your last comment before submit these changes.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jul 28, 2017

Contributor

Last update: We use the namespace from all known form types as base to search the form type. We show a comment when there is more than one type found.

This mean, if there is at least one form type registered, its namespace will be used to find other ones not registered.

Contributor

yceruto commented Jul 28, 2017

Last update: We use the namespace from all known form types as base to search the form type. We show a comment when there is more than one type found.

This mean, if there is at least one form type registered, its namespace will be used to find other ones not registered.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Jul 29, 2017

Member

Not a blocker to me (and don't work on it right now, we need more opinions), but other debug commands are using *Descriptors to get the output in different formats (md, txt, json, xml). I'm wondering if it can be useful. It also lighten the command itself by delegating the output construction to a proper class.

Member

ogizanagi commented Jul 29, 2017

Not a blocker to me (and don't work on it right now, we need more opinions), but other debug commands are using *Descriptors to get the output in different formats (md, txt, json, xml). I'm wondering if it can be useful. It also lighten the command itself by delegating the output construction to a proper class.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Jul 29, 2017

Member

The debug:container command lists all services when run without any arg.
I think it'll be great to replicate this behavior and get the full list of registered types when the class argument is not provided (and extensions?).

Member

ogizanagi commented Jul 29, 2017

The debug:container command lists all services when run without any arg.
I think it'll be great to replicate this behavior and get the full list of registered types when the class argument is not provided (and extensions?).

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jul 30, 2017

Contributor

The debug:container command lists all services when run without any arg.
I think it'll be great to replicate this behavior and get the full list of registered types when the class argument is not provided (and extensions?).

@ogizanagi I thought on this also at the beginning, but then I gave up, because this information can be obtained by using the command debug:container precisely. So I'm not convinced the command should show these list too, even if it's related to the Form Component.

Even though, if any else votes for this, I'll be glad to implement it.

Contributor

yceruto commented Jul 30, 2017

The debug:container command lists all services when run without any arg.
I think it'll be great to replicate this behavior and get the full list of registered types when the class argument is not provided (and extensions?).

@ogizanagi I thought on this also at the beginning, but then I gave up, because this information can be obtained by using the command debug:container precisely. So I'm not convinced the command should show these list too, even if it's related to the Form Component.

Even though, if any else votes for this, I'll be glad to implement it.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Jul 30, 2017

Contributor

other debug commands are using *Descriptors to get the output in different formats (md, txt, json, xml). I'm wondering if it can be useful. It also lighten the command itself by delegating the output construction to a proper class.

@ogizanagi I had left this to the end, but now that you mention it, this suggestion #23694 (comment) could be the proper class to use descriptors.

Contributor

yceruto commented Jul 30, 2017

other debug commands are using *Descriptors to get the output in different formats (md, txt, json, xml). I'm wondering if it can be useful. It also lighten the command itself by delegating the output construction to a proper class.

@ogizanagi I had left this to the end, but now that you mention it, this suggestion #23694 (comment) could be the proper class to use descriptors.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 31, 2017

Member

Even though, if any else votes for this, I'll be glad to implement it.-

I also think that listing them all when no arg is passed would be nice, getting the same with debug:container requires some filters.

About descriptors, I think that not using them is fine. The ones which describe component specific data are in the framework (container, router, ..), using them would prevent having the command in the form component.

Member

chalasr commented Jul 31, 2017

Even though, if any else votes for this, I'll be glad to implement it.-

I also think that listing them all when no arg is passed would be nice, getting the same with debug:container requires some filters.

About descriptors, I think that not using them is fine. The ones which describe component specific data are in the framework (container, router, ..), using them would prevent having the command in the form component.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Jul 31, 2017

Member

The ones which describe component specific data are in the framework (container, router, ..), using them would prevent having the command in the form component.

@chalasr : We could add our own in the component itself (or even just the TextDescriptor one for now and eventually add other ones later). It does not have to be handled in the FrameworkBundle ones.

Member

ogizanagi commented Jul 31, 2017

The ones which describe component specific data are in the framework (container, router, ..), using them would prevent having the command in the form component.

@chalasr : We could add our own in the component itself (or even just the TextDescriptor one for now and eventually add other ones later). It does not have to be handled in the FrameworkBundle ones.

@yceruto yceruto changed the title from [Form] Add debug:form command to [Form] Add debug:form command (WIP) Jul 31, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Aug 6, 2017

Contributor

To avoid confusion I've renamed the header title of the column "Inherited options" to "Parent options" as all of them (except the "Options" column) are inherited options.

Contributor

yceruto commented Aug 6, 2017

To avoid confusion I've renamed the header title of the column "Inherited options" to "Parent options" as all of them (except the "Options" column) are inherited options.

@ogizanagi

Only minor comments.

Well done @yceruto!

@@ -209,6 +209,8 @@ public function load(array $configs, ContainerBuilder $container)
if (!class_exists('Symfony\Component\Validator\Validation')) {
throw new LogicException('The Validator component is required to use the Form component.');
}
} else {
$container->removeDefinition('Symfony\Component\Form\Command\DebugCommand');

This comment has been minimized.

@ogizanagi

ogizanagi Aug 6, 2017

Member

DebugCommand::class ?

@ogizanagi

ogizanagi Aug 6, 2017

Member

DebugCommand::class ?

This comment has been minimized.

@yceruto

yceruto Aug 7, 2017

Contributor

Thinking about future DebugCommand from other components ... mm I'm not sure if we would like to avoid collisions...

@yceruto

yceruto Aug 7, 2017

Contributor

Thinking about future DebugCommand from other components ... mm I'm not sure if we would like to avoid collisions...

This comment has been minimized.

@ro0NL

ro0NL Aug 15, 2017

Contributor

use as FormDebugCommand?

@ro0NL

ro0NL Aug 15, 2017

Contributor

use as FormDebugCommand?

This comment has been minimized.

@stof

stof Aug 30, 2017

Member

instead of removing it here, the definition should be part of the file loaded only when the config is enabled, inside registerFormConfiguration

@stof

stof Aug 30, 2017

Member

instead of removing it here, the definition should be part of the file loaded only when the config is enabled, inside registerFormConfiguration

Show outdated Hide outdated src/Symfony/Component/Form/Command/DebugCommand.php
@@ -32,7 +32,8 @@
"symfony/http-kernel": "^3.3.5|~4.0",
"symfony/security-csrf": "~2.8|~3.0|~4.0",
"symfony/translation": "~2.8|~3.0|~4.0",
"symfony/var-dumper": "~3.3|~4.0"
"symfony/var-dumper": "~3.3|~4.0",

This comment has been minimized.

@chalasr

chalasr Aug 8, 2017

Member

why is it required?

@chalasr

chalasr Aug 8, 2017

Member

why is it required?

This comment has been minimized.

@ogizanagi

ogizanagi Aug 8, 2017

Member

For the data collector.

@ogizanagi

ogizanagi Aug 8, 2017

Member

For the data collector.

@chalasr

chalasr approved these changes Aug 8, 2017

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Aug 27, 2017

Contributor

Ready for me (just squashing commits).

Contributor

yceruto commented Aug 27, 2017

Ready for me (just squashing commits).

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 30, 2017

Member

why are the class "internal"? isn't their design clean already?
since we build a framework, we have to balance open-closed . "closing" all the time means increasing the risk for users to ignore the annotations - and reduces the extension possibilities of the framework (which is paradoxical for a framework)
dunno if this applies here - just wanted to write it down :)

Member

nicolas-grekas commented Aug 30, 2017

why are the class "internal"? isn't their design clean already?
since we build a framework, we have to balance open-closed . "closing" all the time means increasing the risk for users to ignore the annotations - and reduces the extension possibilities of the framework (which is paradoxical for a framework)
dunno if this applies here - just wanted to write it down :)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 30, 2017

Member

All other descriptor classes were always made internal and it simplifies maintenance. I don't think we ever had requests to open these since they exist. Regarding the OptionsResolverWrapper, I don't see use-cases in userland (and would probably not belong to the form component if the use case was legit).

Member

ogizanagi commented Aug 30, 2017

All other descriptor classes were always made internal and it simplifies maintenance. I don't think we ever had requests to open these since they exist. Regarding the OptionsResolverWrapper, I don't see use-cases in userland (and would probably not belong to the form component if the use case was legit).

@@ -209,6 +209,8 @@ public function load(array $configs, ContainerBuilder $container)
if (!class_exists('Symfony\Component\Validator\Validation')) {
throw new LogicException('The Validator component is required to use the Form component.');
}
} else {
$container->removeDefinition('Symfony\Component\Form\Command\DebugCommand');

This comment has been minimized.

@stof

stof Aug 30, 2017

Member

instead of removing it here, the definition should be part of the file loaded only when the config is enabled, inside registerFormConfiguration

@stof

stof Aug 30, 2017

Member

instead of removing it here, the definition should be part of the file loaded only when the config is enabled, inside registerFormConfiguration

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Aug 30, 2017

Contributor

instead of removing it here, the definition should be part of the file loaded only when the config is enabled, inside registerFormConfiguration.

@stof I'm no sure :/ as It was the modus operandi and the consensus for other commands #23624, #23801, #23694 (comment)

Contributor

yceruto commented Aug 30, 2017

instead of removing it here, the definition should be part of the file loaded only when the config is enabled, inside registerFormConfiguration.

@stof I'm no sure :/ as It was the modus operandi and the consensus for other commands #23624, #23801, #23694 (comment)

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Aug 30, 2017

Contributor

Regarding the OptionsResolverWrapper, I don't see use-cases in userland (and would probably not belong to the form component if the use case was legit).

@ogizanagi should I move the OptionsResolverWrapper class to OptionsResolver Component?

Contributor

yceruto commented Aug 30, 2017

Regarding the OptionsResolverWrapper, I don't see use-cases in userland (and would probably not belong to the form component if the use case was legit).

@ogizanagi should I move the OptionsResolverWrapper class to OptionsResolver Component?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 30, 2017

Member

@yceruto I would let it as is. If one asks for extensibility with a legit use case, we can move it easily, removing the internal flag.

Member

chalasr commented Aug 30, 2017

@yceruto I would let it as is. If one asks for extensibility with a legit use case, we can move it easily, removing the internal flag.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 30, 2017

Member

Same opinion as @chalasr

Member

ogizanagi commented Aug 30, 2017

Same opinion as @chalasr

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi
Member

ogizanagi commented Aug 30, 2017

Thanks @yceruto.

@ogizanagi ogizanagi merged commit 4f040d7 into symfony:3.4 Aug 30, 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

ogizanagi added a commit that referenced this pull request Aug 30, 2017

feature #23694 [Form] Add debug:form command (yceruto)
This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Add debug:form command

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23688
| License       | MIT
| Doc PR        | -

![debug-form](https://user-images.githubusercontent.com/2028198/29007125-c3508cd6-7aca-11e7-91e2-c2b509847db5.png)

A short class name (e.g. `DateType`) can be passed as `class` argument too (the command will try to resolve its FQCN if it's in known form type namespaces).

Commits
-------

4f040d7 Add debug:form command

@yceruto yceruto deleted the yceruto:debug_form_command branch Aug 30, 2017

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 31, 2017

Member

@yceruto Tests are failing on master, could you have a look? https://travis-ci.org/symfony/symfony/jobs/270338064

Member

chalasr commented Aug 31, 2017

@yceruto Tests are failing on master, could you have a look? https://travis-ci.org/symfony/symfony/jobs/270338064

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Aug 31, 2017

Contributor

Sure, I'll take a look soon.

Contributor

yceruto commented Aug 31, 2017

Sure, I'll take a look soon.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 31, 2017

Member

Done in #24059

Member

ogizanagi commented Aug 31, 2017

Done in #24059

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Aug 31, 2017

Contributor

Thanks @ogizanagi! I'm really busy right now.

Contributor

yceruto commented Aug 31, 2017

Thanks @ogizanagi! I'm really busy right now.

ogizanagi added a commit that referenced this pull request Sep 22, 2017

minor #24294 [Form] Add ambiguous & exception debug:form tests (ogiza…
…nagi)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Add ambiguous & exception debug:form tests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | complete #23694  <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Just completing tests a bit for this new feature. cc @yceruto

Commits
-------

35f9c0b [Form] Add ambiguous & exception debug:form tests

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