Skip to content

[Form] Rename "virtual" to "inherit_data" #5899

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

Closed
webmozart opened this issue Nov 3, 2012 · 10 comments
Closed

[Form] Rename "virtual" to "inherit_data" #5899

webmozart opened this issue Nov 3, 2012 · 10 comments

Comments

@webmozart
Copy link
Contributor

I propose to rename the "virtual" option to "inherit_data". The reasoning is quite simple: While it is necessary to explain what a "virtual form" is, the name "inherit_data" is self-explanatory, namely that the form inherits the data from its parent form.

$data = array(
    'name' => 'Foo',
    'street' => 'Bar'
);

$builder
    ->add('name', 'text')
    // e.g. a fieldset
    ->add('address', 'form', array('inherit_data' => true))
;

$builder->get('address')
    ->add('street', 'text')
;

Fieldsets can then be added to core by extending form, setting "inherit_data" to true and adding custom rendering code.

Thoughts?

Update:

BC will be kept until 2.3.

@schmittjoh
Copy link
Contributor

+1 from me; I guess we cannot support both for BC?

@webmozart
Copy link
Contributor Author

@schmittjoh We will support both until 2.3.

@havvg
Copy link
Contributor

havvg commented Nov 3, 2012

-1 on dropping virtual. There are many tools using the virtual naming as something that's "not actually part" of the referred structure or data.

@webmozart
Copy link
Contributor Author

@havvg This was the reason for choosing that name in the first place. The problem is that people can't apply their knowledge from databases here. They first have to learn how data mapping works in order to learn what "virtual" means in the form context. Once they know about data mapping, "inherit_data" should be much more intuitive.

@empire
Copy link
Contributor

empire commented Nov 3, 2012

+1, reasonable naming

@vicb
Copy link
Contributor

vicb commented Nov 4, 2012

-1, "inherit_data" does not sound much better to me. "Mixin" or "Trait" would probably be better than "Inherit" in term of OOP.

@stof
Copy link
Member

stof commented Nov 4, 2012

@vicb I don't see why mixin would be better. It is inheriting the data from the parent form.
The most similar thing I can think about is the inherit value for CSS rules, telling the browser to use the rule of the parent.

@webmozart
Copy link
Contributor Author

@vicb A virtual form is essentially the same as a form with

$builder->add('sub', 'form', array(
    'property_path' => '..',
));

only that this syntax doesn't (and can't) exist. So I really think that an option name related to data mapping should make it easier to understand (even if you like to think of such a form as mixin or trait).

@vicb
Copy link
Contributor

vicb commented Nov 5, 2012

@bschussek your comment made me think of something:

<?php
$builder->add('sub', 'form', array(
    'property_path' => '..',
));

but

<?php
$builder->include('form');

would be possible with a helper include()

@vicb
Copy link
Contributor

vicb commented Nov 6, 2012

but the include() helper could not be used in case of a collection of virtual forms (due to the delayed instantiation it has to be an option in such a case).

fabpot added a commit that referenced this issue Apr 19, 2013
This PR was merged into the master branch.

Discussion
----------

[2.3] [Form] Renamed option "virtual" to "inherit_data" and improved handling of such forms

Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #5899, #5720, #5578
Todo: -
License of the code: MIT
Documentation PR: symfony/symfony-docs#2107

This PR renames the option "virtual" to "inherit_data" for more clarity (the old option is deprecated and usable until 2.3). It also fixes the behavior of forms having that option set.

Forms with that option set will now correctly return their parents' data from `getData()`, `getNormData()` and `getViewData()`. Furthermore, `getPropertyPath()` was fixed for forms that inherit their parent data.

Commits
-------

1290b80 [Form] Fixed the deprecation notes for the "virtual" option
ac2ca44 [Form] Moved parent data inheritance from data mappers to Form
8ea5e1a [Form] Renamed option "virtual" to "inherit_data"
@fabpot fabpot closed this as completed Apr 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants