-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Define FormType class instead of a classic type
#385
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
Conversation
type
@Pierstoval do you still want this feature? Will it work for any case? |
Yep, I'd like to, but I must rebase and make many checks before this.
I think it could cover all possible uses cases. |
I updated my PR but it's still a WIP, I don't have time to work on it right now so I'm gonna take time next week, just don't merge this PR unless you know what you're doing :) |
$formType = new $metadata['fieldType']; | ||
} elseif (strpos($metadata['fieldType'], '@') === 0) { | ||
// If the type starts with "@", we try to retrive the associated service. | ||
$formType = $this->get(substr($metadata['fieldType'], 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pierstoval : I don't understand: why not using the form type alias instead of @form_type_service
? You won't even need this right ? It should already be possible to use our own form types here, no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows you to use a form type as a service without having it to be registered as a global type in Symfony (= no tag in your service definition).
I thought it could be useful to have services that are not bound to the Form component, because it can be useful later if we want to support "EasyAdmin plugins" ( = bundles that add features to EasyAdmin, mostly entities configuration)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😕 Ok. I don't know, it feels odd to me. Other bundles can provide real form types, it makes no difference to me.
On another note: Remember that in sf 2.8, form type aliases would be deprecated in favor of always using the form type FQCN. So, the first option about creating a form instance from a classname would eventually be problematic.
Also, I don't see much benefits to this possibility, as it cannot be instantiated with arguments, whereas the service is much more powerful.
Offering too many ways to achieve the same thing, but with constraints is rarely a good idea :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, specifying the class name is a constraint.
In this deprecation, you would then register form types with services only or by class names (i.e. with or without DIC).
But actually I'm totally lost in the Form
component, it's a part of Symfony that is far too complex for me (brainplosion each time I have to struggle with complex forms...).
So, what to do then? As of Symfony 2.8, class names will be "natively" supported, so do you think I have to cancel this PR, or at least add a version condition for Symfony < 2.8 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR makes sense if trying to use our own form types by specifying its alias in the easyadmin configuration isn't fully supported (I don't remember if it is actually). For symfony 2.8 and up, it would not make any difference, as we could use both form alias (until it is removed from symfony) and class FQCN (if sf 2.8+). That would be handled be the form component itself.
All we have to ensure here, is that there isn't any restriction made by EasyAdmin on the usable form types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I tried, and this is working like a charm in 2.8 with both aliases and FQCN without this PR (You have to declare the form type as service of course).
Therefore, I think there is nothing more to do here =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's close the PR then ;)
AF08
Closing it after reading your comments. Honestly, I can't understand the Symfony Form component, so I can't review this or add anything else to your discussion. Thanks for helping me with this! |
We'll review something similar once Symfony 2.3 will be no longer supported, because the Form component has endured many updates since 2.3, and I don't want to break too many things :) |
Before:
After:
If you set up correct
cascade
options on your entities, it works.BONUS: I added a new
InvalidConfigurationException
class to futurely manage all configuration errors and show them in the back-end directly. It'll allow us to replace all different exceptions in the controller with this one when they depend on configuration.