Skip to content
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

[Form] RepeatedType with not mapped fields allows to have different values #36410

Closed
biozshock opened this issue Apr 10, 2020 · 4 comments
Closed

Comments

@biozshock
Copy link
Contributor

Symfony version(s) affected: 3.4

Description
If one (or both) fields in RepeatedType is not mapped the form allows to have different values

How to reproduce
Add to Tests/Extension/Core/Type/RepeatedTypeTest.php

public function testSubmitUnequalNotMapped(): void
{
    $input = ['first' => 'foo', 'second' => 'bar'];

    $form = $this->factory->create(static::TESTED_TYPE, null, [
        'type' => TextTypeTest::TESTED_TYPE,
        'first_options' => ['mapped' => false],
        'second_options' => ['mapped' => false],
    ]);

    $form->submit($input);

    $this->assertSame('foo', $form['first']->getViewData());
    $this->assertSame('bar', $form['second']->getViewData());
    $this->assertFalse($form->isSynchronized());
    $this->assertSame($input, $form->getViewData());
    $this->assertNull($form->getData());
}

Happens because \Symfony\Component\Form\Extension\Core\DataTransformer\ValueToDuplicatesTransformer is called with value === null on unmapped field.

@HeahDude
Copy link
Contributor

Hello @biozshock, thank you for opening this issue with a reproducer.

May I ask what is the use case that requires to not map repeated inner fields?

@biozshock
Copy link
Contributor Author

@HeahDude I have reused one of application types that has defaults set as not mapped.

I wouldn't say that this is the bug in the symfony itself, but it would be helpful to have an exception or other error of some sort, just to save time for the developer in case they use not mapped repeated fields.
Maybe some validation in configureOptions is the right place, but I'm not sure if that one can catch that inner fields have default not mapped or not.

@HeahDude
Copy link
Contributor

We agree this is not a bug, as this is the expected behavior and inner fields should always be mapped. But adding some kind of validation to it can be complex.

So to resume IIUC, your case is something like:

$builder
    ->add('email', RepeatedType::class, [
        'type' => CustomEmailType::class,
    ])
;

where CustomEmailType has a default mapped=false option, so you need to do:

$builder
    ->add('email', RepeatedType::class, [
        'type' => CustomEmailType::class,
        'first_options' => ['mapped' => true],
        'second_options' => ['mapped' => true],
    ])
;

in order to have it working properly, is that right?

@biozshock
Copy link
Contributor Author

biozshock commented Apr 10, 2020

Yes, that's how i solved this 15 minutes ago. Came here to write the workaround.

But i would say that setting mapped=true for both fields as default values you can not override would be a perfect solution.

@xabbuh xabbuh added the Form label Apr 11, 2020
nicolas-grekas added a commit that referenced this issue Apr 13, 2020
…(biozshock)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] RepeatedType should always have inner types mapped

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Doc PR| symfony/symfony-docs#13519 |
| Tickets       | Fix #36410
| License       | MIT

Always set mapped=true to override inner type mapped setting.
Throw an exception if inner types of RepeatedType has mapped=false

Commits
-------

728cd66 RepeatedType should always have inner types mapped
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

5 participants