-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Add "prototype_data" option to collection type #12314
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
The failing test seems to be unrelated. |
Maybe I'm mistaken, but isn't this already possible and done when creating the form? |
a85c9d1
to
fc7b84c
Compare
@iltar this is about being able to initialize the prototype with a non-empty object, to have default values in fields. |
👍 Looks good. |
I hope this one can make 2.7! |
@webmozart is there anything preventing this from making to 2.7? |
👍 |
I don't think this is the right place to set the data in the prototype actually. It should be set when building the prototype instead |
IMO, the implementation done in #6910 looked better |
I think either implementation is fine. @webmozart closed #6910 in favor of #9177. However it looks like #9177 is yet in a very early state. It will IMHO make sense to move ahead with this way if #9177 makes it into 3.0 it can still be adjusted. I would agree that having the logic in buildForm() makes more sense than in buildView(). An option for 3.0 would be instantiate an entity as the prototype data as it works with an non embedded form by default. If you do not want this you can set the data as you want with the prototype_data option. I think this is normally the expected behavior but would break BC, therefore we can add this only in 3.0. |
shouldnt this cause problems, when you provide data in the child options? edit: #6910 does not have this problem, but another one: you cant provide prototype_data and item data at the same time, since the item data, will always overwrite the prototype_data |
The "data" option under the "options" option intended for the target form type would previously overwrite "prototype_data".
@peterrehm yep, instantiating an entity sounds the sanest way for me too. Presumably @backbone87 yes, You raise a very valid point. Do you mean this exception? I'll try whipping up a test to verify this. Turns out my initial solution also suffered from item data always overwriting prototype data so I went for something closer to #6910. Now the Also, this PR should probably be merged to |
No i mean this exception: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Form.php#L314` Regarding inherit data check: In my project i use the following check for prototype_data to be "injected" into the prototype: $prototypeOptions = array_replace(array(
'label' => $options['prototype_name'].'label__',
), $options['options']);
if($options['prototype_data'] !== null) {
$prototypeOptions['data'] = $options['prototype_data'];
}
$prototype = $builder->create($options['prototype_name'], $options['type'], $prototypeOptions);
$builder->setAttribute('prototype', $prototype->getForm()); So if you use "prototype_data" = null (the default), it means "dont use any special prototype data" and the previous behavior is fully restored. |
Regarding empty_data: |
Thanks @backbone87 that's a good tip, your solution works like a charm. For anyone wanting "a ready to go" code, here's a form type extension I use: use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolverInterface;
class CollectionTypeExtension extends AbstractTypeExtension
{
public function buildForm(FormBuilderInterface $builder, array $options)
{
if ($options['allow_add'] && $options['prototype']) {
$prototypeOptions = array_replace(array(
'label' => $options['prototype_name'].'label__',
), $options['options']);
if ($options['prototype_data'] !== null) {
$prototypeOptions['data'] = $options['prototype_data'];
}
$prototype = $builder->create($options['prototype_name'], $options['type'], $prototypeOptions);
$builder->setAttribute('prototype', $prototype->getForm());
}
}
public function setDefaultOptions(OptionsResolverInterface $resolver)
{
$resolver->setDefaults(array('prototype_data' => null));
}
public function getExtendedType()
{
return 'collection';
}
} Just register it in your services: services:
acme_demo_bundle.collection_type_extension:
class: Acme\DemoBundle\Form\Extension\CollectionTypeExtension
tags:
- { name: form.type_extension, alias: collection } and you can use cheers 🍻 |
Thanks a lot for working on this @kgilden! |
👍 Awesome to get that supported by default. |
@webmozart the way its currently implemented in this commit IS a BC break since the prototype_data will ALWAYS be used, even if legacy code not using prototype_data relies on the old behavior where the data option is used for the prototype form. i recommend revisiting this implementation and using code similar to the one i showed in my comment #12314 (comment) |
@backbone87 Please open a new issue if you think you have found a bug/a BC break as your comment easily gets lost in an already closed pull request. |
Makes it possible to have default data for collection prototypes.