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

Add maker command for form type extensions #85

Closed
wants to merge 5 commits into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Dec 4, 2017

I've two versions of this command:

  1. This asks only for the class name of the form type extension and the extended type is always the FormType::class. Then, the user is responsible for changing it if necessary (with IDE autocompletion this task is very fast).
  2. (The current one) Ask also for the FQCN of the extended type + class exists validation, but it could be a boring task considering that most of the time this form type comes from a vendor with a long namespace. To improve it, I thought about injecting all available form types and allowing the input of the class name only, something similar to what was done with the command debug:form <FormType> argument.

form-type-extension

closes #78

@yceruto
Copy link
Member Author

yceruto commented Dec 11, 2017

Anyone there!? :) very low review activity on this amazing new bundle by the way 🚤 I'd like to put 2th gear

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks OK to me. Thanks Yonel.

@yceruto
Copy link
Member Author

yceruto commented Dec 14, 2017

@javiereguiluz thanks for the minor reword ;) it looks better now.

@weaverryan
Copy link
Member

weaverryan commented Dec 31, 2017

Hey @yceruto!

Sorry for the delay - I wanted to get the new functional test system in place before working on any additional makers.

Imo, I would prefer option (2): where we allow the user to type in the extended type, but help them a bit (because yea, nobody wants to type the long class name, especially since \ needs to be doubled to \\ - I'm not sure if that's true anymore...). We do something similar in MakeSubscriber: we list all the events, then allow them to free-type, but with auto-completion. I agree that we should only make the user type the short version of the class name (e.g. FormType) (if there is a conflict, I guess we'll need to somehow use a less-shortened version... but that's an edge case).

Could you update the maker for these changes? Also, the FunctionalTest class has changed and there has been a slight change in MakerInterface after #97.

very low review activity on this amazing new bundle by the way 🚤 I'd like to put 2th gear

I agree :D. I think it's because I've been busy with some "boring" low-level tasks, but those are done now!

@yceruto
Copy link
Member Author

yceruto commented Jan 2, 2018

Hi! happy new year to everyone 🎉

@weaverryan yeah, i'll try to do it this weekend, cheers.

@yceruto yceruto changed the title Add maker command for form type extensions (WIP) Add maker command for form type extensions Jan 7, 2018
@yceruto yceruto changed the title (WIP) Add maker command for form type extensions Add maker command for form type extensions Jan 8, 2018
@yceruto
Copy link
Member Author

yceruto commented Jan 8, 2018

Latest changes:

  • Added extended type argument.
  • Show autocomplete values, populated with all "available" form types (core types + registered as services).
  • Show ambiguous validation & choice question if there is more than one FQCN with the same CN.
  • Added class exists validation to Validator.

Ready for review!

@yceruto
Copy link
Member Author

yceruto commented Jan 12, 2018

Updated description with preview!

@yceruto
Copy link
Member Author

yceruto commented Jan 18, 2018

this is ready from my side... ;)

@javiereguiluz javiereguiluz added the Feature New Feature label Jan 24, 2018
@yceruto
Copy link
Member Author

yceruto commented Feb 19, 2018

I'm closing here because I can't fix this after rebase on master.

@yceruto yceruto closed this Feb 19, 2018
@yceruto yceruto deleted the form_type_extension branch February 19, 2018 19:42
@weaverryan
Copy link
Member

@yceruto sorry about that - I should have communicated better. I wanted to get a few internal updates done (which are now done) before merging this. If you can’t rebate, I understand. But both of your PR’s are good imo :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Add a make:form:type-extension command
3 participants