Skip to content

[Form] Add debug:form command #23694

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

Merged
merged 1 commit into from
Aug 30, 2017
Merged

Conversation

yceruto
Copy link
Member

@yceruto 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
Copy link
Member Author

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 [FrameworkBundle] Add debug:form command [Form] Add debug:form command Jul 28, 2017
@yceruto yceruto force-pushed the debug_form_command branch from d68b634 to a4938b6 Compare July 28, 2017 15:46
@yceruto
Copy link
Member Author

yceruto commented Jul 28, 2017

Failure on deps=high is normal.

@ogizanagi
Copy link
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?

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

yceruto commented Jul 28, 2017

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

@yceruto
Copy link
Member Author

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.

@yceruto yceruto force-pushed the debug_form_command branch 2 times, most recently from a02cd92 to 1437e85 Compare July 28, 2017 21:11
@ogizanagi
Copy link
Contributor

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

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 yceruto force-pushed the debug_form_command branch from 1871684 to 68d49b7 Compare July 30, 2017 21:52
@yceruto
Copy link
Member Author

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

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

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 force-pushed the debug_form_command branch 4 times, most recently from d738b92 to d67dc00 Compare August 15, 2017 18:07
@yceruto yceruto force-pushed the debug_form_command branch from d67dc00 to 69d5260 Compare August 27, 2017 16:51
@yceruto
Copy link
Member Author

yceruto commented Aug 27, 2017

Ready for me (just squashing commits).

@yceruto yceruto force-pushed the debug_form_command branch from 69d5260 to 4f040d7 Compare August 29, 2017 17:41
@nicolas-grekas
Copy link
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 :)

@ogizanagi
Copy link
Contributor

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

@yceruto
Copy link
Member Author

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

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

Same opinion as @chalasr

@ogizanagi
Copy link
Contributor

Thanks @yceruto.

@ogizanagi ogizanagi merged commit 4f040d7 into symfony:3.4 Aug 30, 2017
ogizanagi added a commit that referenced this pull request Aug 30, 2017
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 debug_form_command branch August 30, 2017 16:26
@chalasr
Copy link
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
Copy link
Member Author

yceruto commented Aug 31, 2017

Sure, I'll take a look soon.

@ogizanagi
Copy link
Contributor

Done in #24059

@yceruto
Copy link
Member Author

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
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants