[3.0] [Form] Added the new getBlockPrefix() method to the ResolvedFormTypeInterface #16200
Conversation
wouterj
commented
Oct 11, 2015
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | - |
|
The implementations should change the phpdoc to |
|
2.8 should trigger a deprecation when your form type does not implement the method though. |
|
@stof there is a default implementation of this method in |
| /** | ||
| * Returns the prefix of the template block name for this type. | ||
| * | ||
| * The block prefixes default to the underscored short class name with |
There was a problem hiding this comment.
Using singular "The block prefix defaults to" would fit better since this method is about exactly one prefix.
|
LGTM 👍 |
Status: Needs Work |
|
Changed PHPdoc to |
|
Status: Needs review |
| * Returns the prefix of the template block name for this type. | ||
| * | ||
| * @return string The prefix of the template block name | ||
| * {@inheritdoc} |
There was a problem hiding this comment.
this is wrong as ResolvedFormTypeInterface does not define a getBlockPrefix. So either the method must be added to that interface as well. Or the public method should be removed from ResolvedFormType. I don't see why we need that public method on the resolved type as one can access it easily through getInnerType
There was a problem hiding this comment.
getName was part of the ResolvedFormTypeInterface: https://github.com/symfony/form/blob/2.8/ResolvedFormTypeInterface.php#L23-L28 I think it makes much sense to make getBlockPrefix part of the interface in 3.0
There was a problem hiding this comment.
I think it should be removed. I don't see the point of having this method in the resolved type as it must equal the inner one. So overwriting it makes no sense either.
There was a problem hiding this comment.
But if we remove it, we should do so in 2.8. @webmozart what is the point of having a getBlockPrefix on the ResolvedFormType?
There was a problem hiding this comment.
let's just merge this as is?
There was a problem hiding this comment.
I think we should add the method to ResolvedFormTypeInterface also to respect the LoD. The method is used in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/BaseType.php#L80.
There was a problem hiding this comment.
@webmozart why not simply call $type->getInnerType()->getBlockPrefix() there? It seems strange to me to offer two ways to do the same thing.
There was a problem hiding this comment.
I agree with @Tobion if that's the only reason to have this method in the interface.
There was a problem hiding this comment.
My argument can be found in #16749. ResolvedFormType was always supposed to have all methods of FormType. getInnerType() exists for edge cases where you want to know by which actual class a form type is backed.
|
This one should be finished and merged today or tomorrow. |
|
@fabpot the PR itself is finished. Should I split the PR in 2 PRs: One for the FormType and one for the ResolvedFormType? (as the FormType is a no-brainer and has to be merged) |
|
Splitting would be indeed a good way to merge it faster. |