further improvements based on comments to #22 #23
Conversation
| ? $this->createForm(new LegacyModifySettingsForm(), $formData) | ||
| : $this->get('form.factory')->createNamed('craue_config_modifySettings', 'Craue\ConfigBundle\Form\ModifySettingsForm', $formData); | ||
| $form = method_exists('Symfony\Component\Form\AbstractType', 'getBlockPrefix') | ||
| ? $this->get('form.factory')->createNamed('craue_config_modifySettings', 'Craue\ConfigBundle\Form\ModifySettingsForm', $formData) |
There was a problem hiding this comment.
if you define getBlockPrefix properly (and you should, otherwise you break BC for form theming), you would not need to use createNamed, and would be able to use the shortcut here too
There was a problem hiding this comment.
it uses the block prefix as the default name
There was a problem hiding this comment.
Then I might've hit a bug here, but tests fail if I change this line to ? $this->createForm('Craue\ConfigBundle\Form\ModifySettingsForm', $formData) while getBlockPrefix returns craue_config_modifySettings as well since modify_settings_form is actually used.
There was a problem hiding this comment.
hmm, indeed. FormFactory::create does not deal with customized block prefixes properly. It always turns the FQCN to a block prefix (similar to the default implementation of the method).
This should be reported to Symfony as a bug in the 2.8 implementation.
There was a problem hiding this comment.
Done in symfony/symfony#15760.
There was a problem hiding this comment.
I'm merging the PR now, will change this line once the mentioned issue is resolved.
There was a problem hiding this comment.
Done in commit f1cc739, now that the issue is fixed by symfony/symfony#16758.
further improvements based on comments to #22
This improves the code based on comments to #22.