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] Date field missing from submitted form data when value supplied #28584

Closed
zanbaldwin opened this issue Sep 24, 2018 · 13 comments
Closed

Comments

@zanbaldwin
Copy link
Member

zanbaldwin commented Sep 24, 2018

Symfony version(s) affected: 4.1.2.3 (that's the only version I've checked so far).

Description

A date field is missing from the submitted form data when a value is supplied. This behaviour doesn't happen for form types such as TextType.

How to reproduce

See zanbaldwin/symfony-issue-28584:master

  1. Create new project from symfony/skeleton:v4.1.2.3.
  2. composer req form
  3. Create JSON payload listener.
  4. Create form type with date field.
  5. Create controller (and route definition).
  6. Start local PHP webserver (php -S 127.0.0.1:8000 -t public)
  7. Send test payloads (I used HTTPie via the command: echo '$DATA' | http --json --print b POST ":8000")

Example Payloads

Request Data ($DATA) Response Data
{"my_date":null} array(1) { ["my_date"]=> NULL }
{"my_date":""} array(0) { }
{"my_date":"2018-01-01"} array(0) { }

Possible Solution

Haven't really dug into this yet, I'll update with anything I find.

@xabbuh
Copy link
Member

xabbuh commented Sep 24, 2018

The default widget for a DateType is choice which means that the field will be rendered with three select elements in an HTML document. This also means that you need to submit three values instead of a single string:

{
    "my_date": {
        "day": "1",
        "month": "1",
        "year": "2018"
    }
}

Alternatively, you should also be able to set the widget to single_text and configure the expected date format using the format option.

@zanbaldwin
Copy link
Member Author

zanbaldwin commented Sep 24, 2018

Yes, setting ['widget' => 'single_text'] does fix the third example - I can't believe I missed that (I even used that option today).

Although my use-case is completely fine, I would expect the second example to have the date field in the array with an empty value, and the third example to cause a form error 🤔

@xabbuh
Copy link
Member

xabbuh commented Sep 24, 2018

The last two examples end with the exception in your controller, don't they? That's because the view transformer of the date type will fill and thus the form is not valid in the cases.

@zanbaldwin
Copy link
Member Author

No, the last two examples are still being var_dump'd (so the form reports as being submitted and valid).

@xabbuh
Copy link
Member

xabbuh commented Sep 27, 2018

Thank you for providing the reproducer. I had a look at it and this one is indeed a bit tricky. What's happening here is that the view transformer for the date field indeed throws exceptions as expected for the last two examples. That's why the data is not returned from the form. The reason that the form is not marked as invalid despite the transformation failure is probably due to the same reason as in #20916. I try to look at it soon to see how we can fix both this issue and #20916 (maybe they are even just the same).

@xabbuh
Copy link
Member

xabbuh commented Oct 31, 2018

@zanbaldwin Can you confirm that #28731 solves your issue?

@zanbaldwin
Copy link
Member Author

I'll take a look after this morning's meeting! 🙂

@zanbaldwin
Copy link
Member Author

This may take a while. I got angry because that pull request is for 2.8 before DotEnv component was added, and my reproducible stopped working. It's been one of those days.

What's an easy way of testing an application against 2.8 now that symfony/symfony-skeleton is no longer a thing? 😩

@xabbuh
Copy link
Member

xabbuh commented Oct 31, 2018

@zanbaldwin You can use https://github.com/xabbuh/symfony/tree/issue-20916-4.1 for testing which is the same patch rebased on the 4.1 branch.

@zanbaldwin
Copy link
Member Author

I don't think xabbuh/symfony@14fba95 works for this case unfortunately, but I'm not sure if I tested correctly. My steps to test were:

$ git clone "git://github.com/zanbaldwin/symfony-issue-28584.git"
$ cd "symfony-issue-28584"
$ composer install
$ git clone "git://github.com/xabbug/symfony.git"
$ cd symfony
$ git checkout "issue-20916-4.1"
$ cd ..
$ rm -rf "vendor/symfony/form"
$ ln -s "symfony/src/Symfony/Component/Form" "vendor/symfony/form"
$ bin/console cache:clear
$ php -S "localhost:8888" -t "public"
$ echo '{"my_date":null}' | http --json --print b POST ":8888"
array(1) {
  ["my_date"]=>
  NULL
}

$ echo '{"my_date":""}' | http --json --print b POST ":8888"
array(0) {
}

$ echo '{"my_date":"2018-01-01"}' | http --json --print b POST ":8888"
array(0) {
}

@xabbuh
Copy link
Member

xabbuh commented Nov 1, 2018

You will also need to make sure to use the FrameworkBundle from the patch:

$ rm -rf "vendor/symfony/framework-bundle"
$ ln -s "symfony/src/Symfony/Bundle/FrameworkBundle" "vendor/symfony/framework-bundle"

Otherwise the extension will not be registered.

@zanbaldwin
Copy link
Member Author

Urgh. Knew I was doing something wrong! I've retested and can confirm that it works!

I can now confirm that xabbuh/symfony@14fba95 DOES work for this use case! 😄

@xabbuh
Copy link
Member

xabbuh commented Nov 1, 2018

👍 Great, thank you very much for the example application that allowed me to find a solution and for trying the patch.

@fabpot fabpot closed this as completed Nov 12, 2018
fabpot added a commit that referenced this issue Nov 12, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

[Form] invalidate forms on transformation failures

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20916, #21242, #28584
| License       | MIT
| Doc PR        |

Commits
-------

385d9df invalidate forms on transformation failures
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

4 participants