8000 [Form] Add generic way for overriding nested options · Issue #9177 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form] Add generic way for overriding nested options #9177

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 Sep 30, 2013 · 21 comments
Closed

[Form] Add generic way for overriding nested options #9177

webmozart opened this issue Sep 30, 2013 · 21 comments

Comments

@webmozart
Copy link
Contributor

We currently use various different ways for influencing the options of the children of an added type:

a) By letting one option accept multiple values for the children (e.g. the "empty_value" option in DateType)

$form->add('updatedAt', 'date', array(
    'empty_value' => array(
        'day' => 'The day...',
    ),
));

b) By having dedicated "options" options (e.g. "options" in CollectionType, "first_options"/"second_options" in RepeatedType)

$form->add('password', 'repeated', array(
    'type' => 'password',
    'first_options' => array(
        'required' => true,
    ),
));

And in #3825, a third possibility was suggested:

c) By having one dedicated option per child and option (e.g. "day_widget", "month_widget" etc.)

$form->add('updatedAt', 'date', array(
    'day_widget' => 'text',
    'month_widget' => 'choice',
));

These different alternatives

  • are inconsistent
  • need to be implemented manually
  • need to be changed whenever a new child option should be supported

Therefore I propose adding a generic way for overriding nested options by supporting a special syntax in the options array:

@child(/nestedChild)*[#option]

Examples:

$builder->add('updatedAt', 'datetime', array(
    '@date' => array(
        'required' => true,
        '@day' => array(
            'empty_value' => 'The day...',
        ),
    ),
));

// equivalent to

$builder->add('updatedAt', 'datetime', array(
    '@date' => array(
        'required' => true,
    ),
    '@date/day' => array(
        'empty_value' => 'The day...',
    ),
));

// equivalent to

$builder->add('updatedAt', 'datetime', array(
    '@date#required' => true,
    '@date/day#empty_value' => 'The day...',
));

Additionally, setting an option for all children at once should be supported:

$builder->add('updatedAt', 'datetime', array(
    '@date/*#empty_value' => '...',
));

References:

@stof
Copy link
Member
stof commented Sep 30, 2013

Will this require making each form type aware of this syntax when adding its children or would it be handled automatically in the Form component for any form type ?

@webmozart
Copy link
Contributor Author

@stof This should be handled automatically for any form type.

@stof
Copy link
Member
stof commented Sep 30, 2013

@bschussek at which level would it be implemented ?

@webmozart
Copy link
Contributor Author

@stof Not sure yet

@dewos
Copy link
dewos commented Sep 30, 2013

Star symbol for children settings? Why not a classic hierarchical extension "parent to children" with the opportunity to override into the child option?

@peterrehm
Copy link
Contributor

Why @Date? Shouldn't it be @updatedAt?

@webmozart
Copy link
Contributor Author

@dewos @<childName> in my example refers to the child <childName>, @* refers to all children.

@peterrehm The "updatedAt" field in the example is of type "datetime" and has two children, "date" and "time". In this example, I configured the options for the nested "date" field.

@dewos
Copy link
dewos commented Oct 1, 2013

Hi @bschussek. Ok fine, but maybe the @* "parent to children" can be automatically implicit, with the option to explicit override the setting in the child.

@stof
Copy link
Member
stof commented Oct 1, 2013

@dewos you mean that all options should be forwarded to children by default ? This is a bad idea IMO as available options for the children are different than available options for the type itself.

@dewos
Copy link
dewos commented Oct 2, 2013

@stof yeah it's true, but mostly they're the same, and for @* is problematic too, i think.

@stof
Copy link
Member
stof commented Oct 3, 2013

@dewos No. For @*, we are explicitly stating that we want these options to be forwarded (and they don't need to be valid options for the parent). And using * to match all children is of course possible only for options being valid for all children.

@dewos
Copy link
dewos commented Oct 3, 2013

I see. Thanks @stof.

@webmozart
Copy link
Contributor Author

Some more thoughts: This would probably be easier to implement if the syntax was this:

$builder->add('updatedAt', 'datetime', array(
    '@date' => array(
        'required' => true,
        '@day' => array(
            'empty_value' => 'The day...',
        ),
    ),
));

// equivalent to

$builder->add('updatedAt', 'datetime', array(
    '@date' => array(
        'required' => true,
    ),
    '@date/@day' => array(
        'empty_value' => 'The day...',
    ),
));

// equivalent to

$builder->add('updatedAt', 'datetime', array(
    '@date/required' => true,
    '@date/@day/empty_value' => 'The day...',
));

Then we can simply split by /. Maybe even the first version is sufficient, where we just need to test the first character of each option.

One important point is that this syntax also needs to be usable in setDefaultOptions(). This is not really useful if a form type also calls add(), because then the option can just be added in add(). But for type extensions, this could come in handy.

public function setDefaultOptions(OptionsResolverInterface $resolver)
{
    $resolver->setDefaults(array(
        '@day/required' => true,
    ));
}

@gnat42
Copy link
Contributor
gnat42 commented Jan 30, 2015

This would be a fantastic feature to land in 2.7 if possible. Its a real annoyance to work around.

@backbone87
Copy link
Contributor

i think the inherit options should get their own option entry:

    $resolver->setDefaults(array(
        'inherit_options' => array(
            '@day/required' => true,
        ),
    ));

this solves multiple problems:

  • less to none refactoring of the options component
  • less conflicts with legacy code (only 1 new common option entry)
  • provides an explict ordering of the inheritance "instructions", which solves overwrite problems ('@*/required' => false, '@day/required' => true,), since the options component does not maintain a comprehensive option entry order

@backbone87
Copy link
Contributor

what i also noticed: this feature does not resolve #12314 in any means, since the prototype is its "own" form, that is not connected to the original, except for sharing some configuration

@backbone87
Copy link
Contributor

some more thoughts:
i think the following syntax, is most likely to avoid BC issues:
^ ( (@[^/]+|\*)(/@[^/]+|/\*)*(/\*\*)? | \*\* )? # (.+) $

$resolver->setDefaults(array(
  'inherit_options' => array(
    '#required' => false,
    '#inherit_options' => array(
      '@child/**#required' => false
    ),
    '@day#required' => true,
  ),
));

@xabbuh
Copy link
Member
xabbuh commented Jul 22, 2019

I am not convinced that we need this. For example, in many core types where we need to provide options that are ultimately passed through to a nested type we first perform some validation as not all options are useful in all contexts, but also depend on some other option set for the parent type. Thus, this feature wouldn't save as much here, but is something that still needs to be maintained.

@symfony/deciders What do you think?

@yceruto
Copy link
Member
yceruto commented Jul 22, 2019

Could it be already cover by the feature "Nested Options Definition" introduced since Symfony 4.2?

This would at least solve the issue of override some nested options.

@linaori
Copy link
Contributor
linaori commented Jul 30, 2019

I'm not in favor of this. The problem I see here is that if I have a form type that explicitly doesn't provide an option for an inner type, it can be bypassed and change the behavior of the form.

I'm of opinion that if you want to have the option to configure an inner field, the wrapper form should provide this configuration option. Yes it's slightly repetitive, yet makes it possible to restrict configuration where needed.

Any configuration for nested types should be configured through the wrapper.

@xabbuh
Copy link
Member
xabbuh commented Jul 30, 2019

let's close here then

@xabbuh xabbuh closed this as completed Jul 30, 2019
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

9 participants
0