Added explicit implementation of `AbstractType::getName()` #11185

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
Contributor

apfelbox commented Jun 20, 2014

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

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.

Member

stof commented Jun 20, 2014

-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.
And if we were using the FQCN, the type names would be unusable most of the time.

Contributor

apfelbox commented Jun 20, 2014

An alternative implementation would be to just hash the FQCN:

return substr(sha1(get_class($this)), 0, 10);

Pro:

  • easy to implement
  • clashes highly unlikely

Con:

  • "unreadable" form name
Contributor

apfelbox commented Jun 20, 2014

@stof ping

Member

stof commented Jun 20, 2014

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

Contributor

apfelbox commented Jun 20, 2014

Don't you have to register your type as a service to be able to only use the name in $builder->add("field", "myForm") (instead of $builder->add("field", new MyForm()))?

And this name is set in the tag of the service, isn't it?
(Not very familiar with the internals of the form component)

Member

stof commented Jun 20, 2014

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

Contributor

apfelbox commented Jun 20, 2014

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

@apfelbox apfelbox closed this Jun 20, 2014

Contributor

inso commented Jun 20, 2014

@stof why FQCN is not usable? With new PHP versions we can do like this:

$form = $this->createForm(TaskType::class, $task);
Contributor

apfelbox commented Jun 20, 2014

@inso this looks really nice, actually.

@weaverryan weaverryan added the DX label Jun 20, 2014

Member

weaverryan commented Jun 20, 2014

I'm actually a big +1 for this. For me:

  1. If you're creating a reusable form type, then yes, you should specify this specifically (e.g. my_fancy_choice).

  2. But, if you're just creating a normal form that you'll use in a controller, then you don't care at all about this.

This is a DX issue, and it was on my list too. Case 2 is the major use-case. Additionally, since if you are doing (1) and don't realize you need to override getName, you will get an exception as Stof mentioned.

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?

Contributor

apfelbox commented Jun 20, 2014

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

Contributor

apfelbox commented Jun 20, 2014

I'll reopen it for now: still under discussion.

@apfelbox apfelbox reopened this Jun 20, 2014

Member

stof commented Jun 20, 2014

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 createForm shortcut in controller and the form factory, forcing you to build named forms everywhere, because \ is not allowed in form names, and the shortcut uses the type name as form name.

@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

Member

stof commented Jun 20, 2014

especially since the exception tells you so

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.

Contributor

apfelbox commented Jun 20, 2014

if it is the FQCN itself, it is indeed easy to use for PHP 5.5+ users (still a pain for others)

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.

because \ is not allowed in form names

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?

So for non-reused themes, you don't care about the actual value, but you care about its uniqueness

... which is accomplished by using a FQCN.

Member

stof commented Jun 20, 2014

@apfelbox \ in HTML input names and ids are not allowed in HTML4 (I'm not sure about HTML5 as they expanded the list of allowed chars)

Member

stof commented Jun 20, 2014

and \ is not allowed either in Twig block names, so I'm not sure we allow it either in form type names (it would make it impossible to customize the rendering of the form type in a form theme).
Using \ in form names has the same issue for Twig for the instance customization rather than the type customization in form themes

Member

weaverryan commented Jun 20, 2014

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?

  1. If you're actually defining this as a reusable form type. If we have 2 registered form types with the same name, do we currently get an exception? It seems like we could (if we don't already), so we could protected against this

  2. Stof mentioned something related to form theming:

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

Stof, can you give a bit more information about this?

  1. Anywhere else uniqueness is needed?

Also, about this:

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

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. buildForm) with all the use statements. Easy with an IDE, but looks daunting to a new user without an IDE. I know this last part isn't constructive - I'm thinking out loud.

Member

stof commented Jun 20, 2014

Where is uniqueness needed?

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 search, being a compound type, as Symfony also defines search which gets rendered as <input type="search"> for instance)

Stof, can you give a bit more information about this?

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.
If you reuse the same name, you will get the rendering intended for another form type.

I actually do this :). And unfortunately, I probably will continue to do this

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)

Member

javiereguiluz commented Jul 10, 2014

@stof I still don't understand why this proposal made by @apfelbox and @weaverryan cannot be included in Symfony:

  • AbstractType implements the getName() method logic ensuring that the generated form name is unique.
  • If you don't need or care about the form name, you don't override the getName() method in your form (this is what would improve the DX).
  • If you need to use a custom form name, just override the gteName() as we used to do until now.
Contributor

webmozart commented Aug 5, 2014

Also discussed in #5321

Member

weaverryan commented Aug 5, 2014

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 getName return value, and you add both of these to a form (by instantiating them directly, not registering them as custom form types), then do they interfere with each other? Would I end up with 2 versions of one of the form types because of the name collision? If so, this would be the only issue I can see. I know we could fix this by making the default getName some version of the namespace, but, at least for top-level forms, I'd love to not expose some giant, line namespacey' name attribute in the HTML form :).

Contributor

apfelbox commented Aug 11, 2014

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

Member

stof commented Aug 11, 2014

@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 $formFactory->createBuilder() (which is used by Controller::createForm). Using $formFactory->createNamedBuilder() lets you use what you want as name for the form (with some restrictions on the allowed chars) as it is an argument of the method.

@apfelbox #5321 does not change anything about the implementation of getName in form types. We would still need a name for the types (and we cannot use the class names, as backslashes are not valid in Twig block names, so we would still need a separate unique identifier of the form type when doing the rendering)

Contributor

webmozart commented Sep 15, 2014

I'm proposing the following changes:

  • let getName() return the type's FQCN by default
  • set the alias to the class name by default in the DI config (if that's possible - need to check)
  • set the form name to the lower-cased short name of the type by default, minus the "type" suffix

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?

Member

weaverryan commented Sep 15, 2014

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 getName() and probably should for a re-used form/field. But that could now be a more-advanced detail that most users don't need to worry about.

@Tobion Tobion referenced this pull request Oct 27, 2014

Closed

[DX] Automatic form name #12339

Contributor

flip111 commented Oct 28, 2014

I'm against returning the FQCN for getName() as i think it would show up in the html. Proposed naming lcfirst(substr(get_class($this), 0, -4));
example

classname: TestForm   / TestType
form name: test

I'm not worried about name clashes for the following reasons

  1. It's not a frequent use case to use forms of the same name at the same time
  2. There is no warning system in place right now if you manually set the same name using getName()
  3. It's easy to see in the html source that there are two forms with exactly the same name
Member

Tobion commented Feb 4, 2015

What should also be prevented is that core types get overwritten unintentionally see twigphp/Twig#1623

@fabpot fabpot added the Form label Feb 12, 2015

Contributor

sstok commented Feb 14, 2015

@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?

Contributor

webmozart commented Jun 16, 2015

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! :)

@webmozart webmozart closed this Jun 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment