-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Added explicit implementation of AbstractType::getName()
#11185
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
-1 for this. Using the short class is far too likely to create clashes, and these are really hard to debug. So this would not contribute to the DX. I would even say it would make it worse. |
An alternative implementation would be to just hash the FQCN: return substr(sha1(get_class($this)), 0, 10); Pro:
Con:
|
@stof ping |
The type name is the way you reference form types to use them (passing instances of your type to create it works, but it can be a performance killer when the type is meant to build several forms, for instance when using it inside a collection). So using a hash as name is not good DX either |
Don't you have to register your type as a service to be able to only use the name in And this name is set in the tag of the service, isn't it? |
when registering it as service, you indeed have to copy the name in the tag attributes (this allows to lazy-load the form types). But it must be the same name (you will get an exception in case of mismatch to avoid hard-to-debug errors). Note that types can also be registered using form extensions (useful mostly when using Silex or other consumers of the component, as relying on the DI form extension gives the benefit of lazy-loading when using the fullstack framework). |
@stof thanks for the explanation, I did not know that! I will leave this implementation off to RAD bundles then, where you are more likely to expect such behavior (and probably not in the main framework). |
@stof why FQCN is not usable? With new PHP versions we can do like this:
|
@inso this looks really nice, actually. |
I'm actually a big +1 for this. For me:
This is a When I teach forms, I very often say "create a getName() method, return it a value - it's not important at all, but you just need to do this". I hope we could avoid making this a requirement. Thoughts? |
@weaverryan I mainly do 2) and therefore this proposal - even with the hashed name. If you just use the form to use it in your controller the name doesn't (really) matter. And for 1).. Well, you can still override it - this is just the default implementation. And I would argue, that if you know how to create a custom reusable type, you know that you need to return a custom name (especially since the exception tells you so). |
I'll reopen it for now: still under discussion. |
if it is the FQCN itself, it is indeed easy to use for PHP 5.5+ users (still a pain for others) thanks to the constant. But this will break the usage of the @weaverryan I saw many people building a collection form in their controller and passing a form type instance for the inner type because they thought it was not a reusable type. This is a very bad idea, as it forces the Form component to bypass the optimizations done in Symfony 2.1 or 2.2 (I don't remember), making the form building much slower (the impact of the refactoring was far from being negligeable). And there is still something important in the value: if it matches the name of another type used in your form, it will create weird issues, especially if the other type was defining a custom rendering in the form theme. So for non-reused themes, you don't care about the actual value, but you care about its uniqueness |
the exception only tells you that you made a mistake when defining your service because the name does not match. And it only tells you when trying to use this type in your form. |
This is not a real issue. First it is not extremely complex to create the FQCN string in < PHP 5.5, secondly the usage of PHP 5.5+ can only rise (with PHP 5.6 RC just out the door) and finally you can still use the component as it is today: just return a custom name. Remember: if you use it as you use it today (= custom implementation of the method) nothing will change.
This is the only real issue I see here. Is there a concrete issue why a backslash is not allowed in the form name or is it just an implementation detail?
... which is accomplished by using a FQCN. |
@apfelbox |
and |
I'm going to be a pain :) but I'm not convinced that the uniqueness is important for normal use-cases. Where is uniqueness needed?
Stof, can you give a bit more information about this?
Also, about this:
I actually do this :). And unfortunately, I probably will continue to do this. Because if I understand correctly, I would need to register my form type as a reusable type (e.g. service + tag) to be able to do this correctly, right? That's a lot of extra work. I'm saying this not at all to disagree with you - I think this is a really interesting point. It makes me wonder if there is some different solution to all of this. Because additionally, form types are kind of a pain to create because you have to get the method signatures just right (e.g. |
The FormRegistry stores the known types. It only ever keep a single instance of each type, and the type name is the identifier. So if you register another type with the same name, it will replace it, thus affecting the next steps of the form building and the form rendering (this can create really weird issues if you name your serach form
The type name determines the Twig block used for the rendering of the form. I let you read http://symfony.com/doc/current/book/forms.html#form-fragment-naming for more details.
then you will continue to make your form creation 40% slower (IIRC, this was the difference found in the benchmark when refactoring things in #4882, the actual number might be different now because of later changes) |
@stof I still don't understand why this proposal made by @apfelbox and @weaverryan cannot be included in Symfony:
|
Also discussed in #5321 |
Thanks for linking that :). #5321 is really interesting, because it talks about how people do things like "new FooType", which could degrade performance. So, I think that is a separate problem that we already have, and shouldn't be considered in this PR (because we have that problem now and will have it equally after this). And I think I agree with @javiereguiluz's last comment - I don't understand the problem with implementing this. @stof: if you have 2 form types with the same |
The main problem in this PR seems to be: The form name, with which it is referenced internally is bound to the HTML name-Attribute (and vice versa). If these two would be separate, the issues would resolve. PS: #5321 seems to go in that direction |
@weaverryan yes, they do interfer. The second usage will replace the first type in the FormRegistry. And next time the type will be needed, the second instance will be used. I don't remember exactly how it works internally now, so I don't know the exact impact. But it was even possible that the form building starts with one type and ends with the other one for the same field (causing very weird issues). @apfelbox We are talking about type names here, not about form names. Using the type name as form name is only the case when you use @apfelbox #5321 does not change anything about the implementation of |
I'm proposing the following changes:
Before: class TaskType extends AbstractType
{
public function getName()
{
return 'task';
}
} services:
acme.demo.form.task:
class: Acme\DemoBundle\Form\TaskType
tags:
- { name: form.type, alias: task } $form = $this->createForm(new TaskType());
// or
$form = $this->createForm('task');
echo $form->getName();
// task After: class TaskType extends AbstractType
{
} services:
acme.demo.form.task:
class: Acme\DemoBundle\Form\TaskType
tags:
- { name: form.type } $form = $this->createForm(new TaskType());
// or
$form = $this->createForm(TaskType::class);
// or
$form = $this->createForm('Acme\DemoBundle\Form\TaskType');
echo $form->getName();
// task To me this seems to be the best solution with both older and newer PHP versions in mind. Any objections? |
Wow, that solution seems absolutely perfect to me from a usability perspective. Of course, we would still certainly document the fact that you can shorten this internal name by overriding |
I'm against returning the FQCN for
I'm not worried about name clashes for the following reasons
|
What should also be prevented is that core types get overwritten unintentionally see twigphp/Twig#1623 |
@Tobion 👍 good idea. for what I know is that types are loaded using Form extensions, as component (none lazy) they are provided by the core-extension (which is always checked first). So if the FormFactory looks for a type the core-extension will be the first one that is checked. But when using the FrameworkBundle, types are registered as services and have a di-tag to create a type-look-up mapping and registering a duplicate will overwrite the previous one. Normally you wouldn't overwrite types (as its not really possible) but this is a current behavior so I'm not sure if this is a BC break? |
I opened an issue with a description of how this problem should be solved: #15008. Please open a new PR if you implement a solution for this issue. Thanks! :) |
Proposal
Provide a default implementation of
AbstractType::getName()
which just uses the class name as form name.Caveats
This patch does not prevent name clashes - but you can override the method in your own
AbstractType
implementation.Another implementation could be to just use the full class name (and replace
\
with_
for example - but this would produce really long form names).Problems
Not unit-tested. The current implementation is not directly testable in a unit test - I could move the logic of the class name retrieval in a separate utils class, but I wasn't sure in which class to move it to.
FormUtil
sounds like a generic utils helper, but this logic is not exactly form-related..PS: meant to be a contribution to the DX initiative.