-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Deprecated FormTypeInterface::getName() and passing of type instances #15079
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
I think you can simply add the service id to the |
|
||
if (false === ($pos = strrpos($typeName, '\\'))) { | ||
// No FQCN - leave unchanged for BC | ||
$name = $typeName; |
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.
are we going to remove that in 3.0? Then we could also remove the FormType::getName
completely, or?
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.
Good idea!
de9a855
to
f516801
Compare
Updated. Ping @symfony/deciders |
{ | ||
public function getParent() | ||
{ | ||
return new ParentType(); |
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.
this was more likely to return an alias though
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.
?
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.
ah sorry, I misread the deprecation message. I confused with the deprecation of type names (which is the main goal of this PR even though this other change went in)
Returning an instance in getParent
was indeed a bad idea by forcing to resolve types again and again.
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 stof missed that this is describing the deprecated way.
@webmozart I have a question. You say that when using PHP 5.5, the needed changes are minimal: Before: $form = $this->createFormBuilder()
->add('name', 'text')
->add('age', 'integer')
->getForm();
$form = $this->createForm(new AuthorType()); After: use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\Extension\Core\Type\IntegerType;
use AppBundle\Form\Type\AuthorType;
$form = $this->createFormBuilder()
->add('name', TextType::class)
->add('age', IntegerType::class)
->getForm();
$form = $this->createForm(AuthorType::class); My question is: if you use PHP 5.3 or PHP 5.4, you should use the following? $form = $this->createFormBuilder()
->add('name', 'Symfony\Component\Form\Extension\Core\Type\TextType')
->add('age', 'Symfony\Component\Form\Extension\Core\Type\IntegerType')
->getForm();
$form = $this->createForm('AppBundle\Form\Type\AuthorType'); |
@javiereguiluz yes |
@javiereguiluz Exactly. However, passing "text" or similar type names is still supported until Symfony 3.0 (although you'll get deprecation notes). Symfony 3.0 has a higher minimum PHP version anyway so there this is not a problem anymore. |
@webmozart the original idea of this change was to improve DX by getting rid of the |
@webmozart can you fix the SecurityBundle form login test failures ? These tests are passing on the current 2.8 branch |
Does it possible use dynamic form names? Currently we are using inline editing for cities in grid: $form = $this->container->get('form.factory')->createNamed(
CityInlineEditType::getNameForCity($city),
new CityInlineEditType(),
$city
); Is this code still walid after this PR? |
@Koc Sure, you can use any form name you like. However, the type needs to be passed as FQCN, i.e. $form = $this->container->get('form.factory')->createNamed(
CityInlineEditType::getNameForCity($city),
CityInlineEditType::class,
$city
); |
@Koc this PR is about type names, not about forms names. @webmozart how does this impact the block names in form themes ? |
Is it still possible to use alias? What if you have a type and you want then use this with various dependencies? I have to extend my class to solve this problem? |
@javiereguiluz That's FUD. As I've shown above, you are writing less code than before. As of using types in forms, instead of typing |
@DavidBadura you should add the type as a service <service id="my.type" class="Vendor\Type\MyType">
<tag name="form.type" />
<argument type="service" id="some.service.id" />
</service> and do something like this $form = $this->createFormBuilder()
->add('name', MyType::class)¿
->getForm(); |
* The block prefixes defaults to the underscored short class name with | ||
* the "Type" suffix removed (e.g. "UserProfileType" => "user_profile"). | ||
* | ||
* @return string|null The prefix of the template block name or `null` to |
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.
the null part is not relevant anymore
I think the old way of forcing people to implement
This is why I proposed to remove/deprecate this concept completely instead of just making it optional. The only disadvantage I see is that using the class name does not correspond to type extensions. Using a class name as reference (in contrast to an abstract name) makes people assume it's really just this class. But type extensions can do anything on top of it which somehow clashes with the object-oriented way of using a class name. But I think type extensions are not so common anyway (esp. in user-defined ones). All in all, I'm 👍 |
array_unshift($blockPrefixes, $type->getBlockPrefix()); | ||
} else { | ||
@trigger_error(get_class($type).': The ResolvedFormTypeInterface::getBlockPrefix() method will be added in version 3.0. You should add it to your implementation.', E_USER_DEPRECATED); | ||
array_unshift($blockPrefixes, $type->getName()); |
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.
This code needs to deal with FQCNs.
not only type extension but also type inheritance. but this is just a documentation thing IMO. |
This looks really good. Thanks @webmozart. |
Updated. |
/** | ||
* Returns the prefix of the template block name for this type. | ||
* | ||
* The block prefixes defaults to the underscored short class name with |
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.
typo: default without s
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.
fixed in 8d049c5
Thank you @webmozart. |
…sing of type instances (webmozart) This PR was merged into the 2.8 branch. Discussion ---------- [Form] Deprecated FormTypeInterface::getName() and passing of type instances | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #5321, #15008 | License | MIT | Doc PR | TODO #### Type Names This PR deprecates the definition of the `getName()` method of form types. See #15008 for a more detailed description. Before: ```php class MyType extends AbstractType { public function getName() { return 'mytype'; } // ... } ``` After: ```php class MyType extends AbstractType { // ... } ``` You should always reference other types by their fully-qualified class names. Thanks to PHP 5.5, that's easy: Before: ```php $form = $this->createFormBuilder() ->add('name', 'text') ->add('age', 'integer') ->getForm(); ``` After: ```php $form = $this->createFormBuilder() ->add('name', TextType::class) ->add('age', IntegerType::class) ->getForm(); ``` #### Type Instances Furthermore, passing of type instances is deprecated. Before: ```php $form = $this->createForm(new AuthorType()); ``` After: ```php $form = $this->createForm(AuthorType::class); ``` #### DIC Aliases When registering a type in the DIC, you should omit the "alias" attribute now. Before: ```xml <service id="my.type" class="Vendor\Type\MyType"> <tag name="form.type" alias="mytype" /> <argument type="service" id="some.service.id" /> </service> ``` After: ```xml <service id="my.type" class="Vendor\Type\MyType"> <tag name="form.type" /> <argument type="service" id="some.service.id" /> </service> ``` Types without dependencies don't need to be registered in the DIC as they can be instantiated right away. #### Template Block Prefixes By default, the class name of the type in underscore notation minus "Type" suffix is used as Twig template block prefix (e.g. `UserProfileType` => `user_profile_*`). If you want to customize that, overwrite the new `getBlockPrefix()` method in your type: ```php class UserProfileType extends AbstractType { public function getBlockPrefix() { return 'profile'; } // ... } ``` Commits ------- 3d9e5de [Form] Deprecated FormTypeInterface::getName() and passing of type instances
with this change how will i do now that my Type has some dependency injection stuff as constructor or otherwise? before i did this in service.yml: mybundle.form.type.address:
class: %mybundle.form.type.address.class%
arguments: [ @mybundle.service.transformator_factory ]
tags:
- { name: form.type, alias: address } the transformator_factory did return a DataTransformerInterface i did use for addModelTransformer now does the FormBuilder respect my service definition, and if not how can i get that working again in future symfony 2.8+ or 3.0? |
@Hanmac please open an issue, comments in closed PRs are hard to track |
@Hanmac this works exactly as before (except that you don't need the alias anymore in the tag if you use only the 2.8+ API) |
…eahDude) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle] Deprecated form types as services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ These services are needless in regard of #15079. If they have no dependencies, whatever are the registered extensions, `FormTypeInterface` instances will be autoloaded by their FQCN. Commits ------- dfe4f53 [FrameworkBundle] Deprecated form types as services
from your form types. | ||
|
||
If you want to customize the block prefix of a type in Twig, you should now | ||
implement `FormTypeInterface::getBlockPrefix()` instead: |
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.
What exactly does "block prefix of a type in Twig" mean?
I'm upgrading old Symfony projects and this line is a mystery for me. Can I remove the getName()
or not?
I'd appreciated some Twig snippet example what exactly is meant 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.
This refers to how form fragment names are generated. By default, the value is derived from the class name (see e.g. https://symfony.com/doc/current/form/form_themes.html#form-fragment-naming where textarea
is derived from TextareaType
class). If you have your own TextareaType
class, you will maybe want to override the getBlockPrefix()
method to not reuse the blocks used by the core TextareaType
.
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 never used such fragments and docs is very confusing for me. I don't see any twig sample for textarea_widget
keyword. I also checked PRs referecing this PR when upgrading, but it seems nobody ever used it on custom types.
I look for 3 lines of twig sample, so I can just scan twig templates with regualar expression. There are no Twig tests that could tell us what broke.
{{ form_widget(form.age) }}
{{ textarea_widget(form.age) }}
So for custom:
class SomeType extends FormType
{
public function getName()
{
return 'yolo';
}
}
↓
{{ yolo_widget(form.age) }}
Like this?
So it's string from getName()
+ somehow _widget
magically added?
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.
If you just looking for a way to automatically update the code of the form type class, I would compare the value returned in getName()
with what the automatically computed name would be (see
return StringUtil::fqcnToBlockPrefix(static::class) ?: ''; |
getName()
method to getBlockPrefix()
. Otherwise it can be removed.
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.
That's what I already do with rectorphp/rector#3705 I made it just before reading your comment :D
I want to take it further and remove all the getBlockPrefix()
that:
- don't have standard name
- are never used in the twig templates
That's why I ask how can I find it in the twig templates.
Any example for that?
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.
The special block names are *_row
, *_label
, *_widget
and *_errors
(nowadays there is also *_help
, but I guess you are addressing 2.8 applications here). But you need to be careful as the block prefix is also used to build block names for CollectionType
entries (see https://symfony.com/doc/2.8/form/form_customization.html#form-custom-prototype).
Type Names
This PR deprecates the definition of the
getName()
method of form types. See #15008 for a more detailed description.Before:
After:
You should always reference other types by their fully-qualified class names. Thanks to PHP 5.5, that's easy:
Before:
After:
Type Instances
Furthermore, passing of type instances is deprecated.
Before:
After:
DIC Aliases
When registering a type in the DIC, you should omit the "alias" attribute now.
Before:
After:
Types without dependencies don't need to be registered in the DIC as they can be instantiated right away.
Template Block Prefixes
By default, the class name of the type in underscore notation minus "Type" suffix is used as Twig template block prefix (e.g.
UserProfileType
=>user_profile_*
). If you want to customize that, overwrite the newgetBlockPrefix()
method in your type: