Skip to content

[Form] Fixed edge case where empty data may not be a string and throws TransformationFailedException #13598

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

guilhermeblanco
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

I just can't recreate this error, by any means.
I somehow have a form with an integer type which happens to submit "null" as viewData (somehow) and it reuses empty_data option. Now my field was configured like:

$builder->add('page', 'integer', array(
    'empty_data' => 1,
));

And since the assignment of viewData is the empty_data, one of the normalizers (while calling viewToNorm()) throws a TransformationFailedException with the following message: Expected a string.

… it throws a TransformationFailedException with message: 'Expected a string.'.
@guilhermeblanco
Copy link
Contributor Author

After investigating more on how I hit this corner case, I have more information.
I have a form with 2 fields integer fields. I'm submitting a GET request with 1 field only and forcing $clearMissing = true, which will then call $form->submit() for the second field with NULL data.
When it requires to fetch empty_data, it retrieves the integer value, but the viewToNorm transformation fails because it expects a string.
That's how I think I hit this very interesting weird corner case. =)

@Tobion
Copy link
Contributor

Tobion commented Feb 4, 2015

Definitely needs tests

@Tobion
Copy link
Contributor

Tobion commented Feb 4, 2015

Also see #5939
What you probably need is a string in view data via 'empty_data' => '1'

@guilhermeblanco
Copy link
Contributor Author

@Tobion I completely understand that quoted value leads to proper response. That's what I'm using internally in address this issue.
However, this patch introduces the same behavior as $submittedData, which means that likely some of the previously reported issues would be addressed by this patch. Please understand that currently empty_data option exposes itself supporting mixed values. http://symfony.com/doc/current/reference/forms/types/integer.html#empty-data

@fabpot fabpot added the Form label Feb 5, 2015
@stof
Copy link
Member

stof commented Mar 19, 2015

In any case, this cannot be accepted without tests

@fabpot
Copy link
Member

fabpot commented May 16, 2015

@guilhermeblanco Any chance you can add some tests?

@guilhermeblanco
Copy link
Contributor Author

@fabpot I need to remember the situation that I hit it, but yes, I can add tests for this. =)
IIRC it's due to the fact that empty_data must be an string to work, otherwise it sets the form value to null. According to the docs, it's supposed to support mixed.
I'll double check and add tests for this. =)

@webmozart
Copy link
Contributor

Thank you for submitting this PR @guilhermeblanco! This is a duplicate of #12470. The underlying problem here is #4715, which can't be fixed without breaking BC. Every other solution is unfortunately a hack. I recommend that you use a custom model transformer that sets your default value instead until #4715 is fixed.

@webmozart webmozart closed this Jun 18, 2015
@guilhermeblanco
Copy link
Contributor Author

@webmozart Yes, it is a duplicate. However, can you explain me how does this solution qualify as a BC break?
Currently, it is the same approach taken a few lines before https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Form/Form.php#L535 and there's no way to get non-string values to work. The only BC I can realize here is a not working situation to a working situation.

@webmozart webmozart reopened this Nov 6, 2015
@webmozart
Copy link
Contributor

Reopened after talking to @guilhermeblanco in person. This solution makes sense. Could you please rebase this on 2.3 and add tests?

@fabpot
Copy link
Member

fabpot commented Jan 25, 2016

@guilhermeblanco Any news on this one?

@guilhermeblanco
Copy link
Contributor Author

@fabpot should be able to do it in the next few days. =)

@fabpot
Copy link
Member

fabpot commented Feb 18, 2016

@guilhermeblanco Sorry for being pushy, but we try hard to merge/close old PRs. Any chance you can have a look at this one?

@guilhermeblanco
Copy link
Contributor Author

@fabpot I've spent a substantial amount of hours trying to recreate this scenario on both SimpleFormTest and CompoundFormTest without success.
The issue is however easily reproducible if followed the steps I detailed in the ticket and first comment. Feel free to close if you think appropriate, but this patch solves the bug in my app.

@HeahDude
Copy link
Contributor

I've tried to add tests in IntegerTypeTest but phpunit skip them... I don't understand.

Here's what I've tried :

public function testSubmitNull()
{
    $form = $this->factory->create('integer');

    $form->submit(null);

    $this->assertNull($form->getData());
    $this->assertNull($form->getViewData());
}

public function testSubmitNullWithEmptyData()
{
    $form = $this->factory->create('integer', null, array(
        'empty_data' => 1,
    ));

    $form->submit(null);

    $this->assertSame(1, $form->getData());
    $this->assertSame('1', $form->getViewData());
}

public function testSubmitNullWithEmptyDataWhenNested()
{
    $form = $this->factory->create('form')
        ->add('age', 'integer', array(
            'empty_data' => 1,
        ));

    $form->submit(null);

    $this->assertSame(1, $form->get('age')->getData());
    $this->assertSame('1', $form->get('age')->getViewData());
}

@jakzal
Copy link
Contributor

jakzal commented Mar 1, 2016

@HeahDude IntegerTypeTest requires the intl extension (have a look at the setUp() method).

@HeahDude
Copy link
Contributor

HeahDude commented Mar 1, 2016

@jakzal thanks, will take another look though.

@HeahDude
Copy link
Contributor

HeahDude commented Mar 1, 2016

@jakzal, I've checked and I have intl installed, but tests are skipped even with my global phpunit.

Any lead ?

@jakzal
Copy link
Contributor

jakzal commented Mar 1, 2016

@HeahDude if I remember correctly it needs to be installed in a specific version. You'll need to look into the code that makes the check.

@webmozart
Copy link
Contributor

@HeahDude PHPUnit tells you why a test was skipped if you pass the -v option. In this particular case, the test is skipped because it requires the intl extension with ICU in version 52.1 to be installed. However, current PHP versions ship with ICU 55.1, which is why the tests are being skipped.

#14260 proposes to upgrade to ICU 55.1, which would solve your problem. Would you like to work on that ticket?

@fabpot
Copy link
Member

fabpot commented Mar 3, 2016

IIUC, the change is good but we don't know (yet) how to write good unit tests to avoid regressions, is that correct?

@guilhermeblanco
Copy link
Contributor Author

@fabpot correct. Patch addresses nicely the problem in the known scenario: 2 integer fields with empty_data as integer values and submitting only one of the values (thus requiring the second to use the empty_data).
The patch also uses the same solution for submitted data conversion already in codebase ( https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Form/Form.php#L532 ), but in the context of empty data (which means when submitted data is not present).

@fabpot
Copy link
Member

fabpot commented Mar 4, 2016

Understood. So, instead of waiting for tests, and because this is a bug fix that is waiting here for so long, I propose to merge this as is for now. ping @symfony/deciders

@webmozart
Copy link
Contributor

@fabpot I'll talk to @guilhermeblanco and try to create some tests. If we don't, chances are that this gets refactored away again when optimizing the Form class.

@HeahDude
Copy link
Contributor

HeahDude commented Mar 4, 2016

@guilhermeblanco, is the form compound in your scenario ? I think it may be either not relevant or some cause of this problem.

@weaverryan
Copy link
Member

I'm fine to have this merged - as long as we don't have any BC concerns. If this introduces an edge-case behavior change, I'm not clear what that change is - I'd like to see the before/after code in an UPGRADE file. I want to avoid this causing upgrade issues - it sounds like something that would be quite hard to track down.

@xabbuh xabbuh mentioned this pull request Mar 7, 2016
@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

I took @guilhermeblanco's commit and the tests suggested by @HeahDude (with a minor change where the view data are not expected to be null but the empty string instead) to finish this PR in #18047. I can confirm that the tests were failing before this change and that they do pass now. Should add a special test for the checkbox type too (as the issue initially talked about checkboxes)? And how would such a test look like?

@HeahDude
Copy link
Contributor

HeahDude commented Mar 7, 2016

Nice job! I would suggest:

// CheckBoxTypeTest.php

public function testSubmitNullWithEmptyDataTrueWhenNested()
{
    $form = $this->factory->create('form')
        ->add('agree', 'checkbox', array(
            'empty_data' => true,
        ));

    $form->submit(null);

    $this->assertTrue($form->get('agree')->getData());
    $this->assertSame('1', $form->get('agree')->getViewData());

    $view = $form->get('agree')->createView();

    $this->assertTrue($view->vars['checked']);
}

public function testSubmitNullWithEmptyDataFalseWhenNested()
{
    $form = $this->factory->create('form')
        ->add('agree', 'checkbox', array(
            'empty_data' => false,
        ));

    $form->submit(null);

    $this->assertFalse($form->get('agree')->getData());
    $this->assertEmpty($form->get('agree')->getViewData());

    $view = $form->get('agree')->createView();

    $this->assertFalse($view->vars['checked']);
}

And I confirm those tests fail without the fix.

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

@HeahDude Thanks, test added.

@webmozart
Copy link
Contributor

Closed for the reasons discussed in #18047.

@webmozart webmozart closed this Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants