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

DateType storing wrong format date #24791

Closed
shakaran opened this issue Nov 2, 2017 · 11 comments
Closed

DateType storing wrong format date #24791

shakaran opened this issue Nov 2, 2017 · 11 comments

Comments

@shakaran
Copy link
Contributor

shakaran commented Nov 2, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 3.3.10

I am using a custom type based in DateType and when it stores the date in the database is changing the format, even if I indicate the format in the widget.

This is my code:

<?php

namespace MyApp\Form\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Form\Extension\Core\Type\DateType;
use Symfony\Component\Form\Extension\Core\Type\TextType;

class DatePickerType extends AbstractType
{
    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults(
                [
                    'data_class' => null,
                    'widget' => 'single_text',
                    'format' => 'dd/mm/yyyy', // Note here, it will change to default {{ year }}{{ month }}{{ day }} but dd/MM/yyyy will work
                                'html5' => FALSE,
                                'input' => 'datetime',
                                'model_timezone' => 'Europe/Madrid',
                                'view_timezone' => 'Europe/Madrid',
                                 'attr' => [
                                ]
                 ]
                );
    }

    public function getParent()
    {
        return DateType::class;
    }
}  

Note that using 'format' => 'dd/mm/yyyy' will fail because this line that make a preg_match it is not parsing or taking in mind the month in lowercase format and resulting as fallback as {{ year }}{{ month }}{{ day }} which stores in database like yyyy-MM-dd, so I always get corrupted the month value.

So a possible fix could be allow "M" or "m", thats is (M|m) in the regex:

if (preg_match('/^([y(M|m)d]+)[^y(M|m)d]*([y(M|m)d]+)[^y(M|m)d]*([y(M|m)d]+)$/', $pattern)) {
                $pattern = preg_replace(array('/y+/', '/M+/', '/d+/'), array('{{ year }}', '{{ month }}', '{{ day }}'), $pattern);
            }
@Simperfit
Copy link
Contributor

As i'm reading the comments on the preg_match, I followed the link for ICU, if you look at this, it will tell you that you will need to use MMM or MMMM, for exemple you can look at the test about MMM in de_AT

@shakaran
Copy link
Contributor Author

shakaran commented Nov 2, 2017

@Simperfit interesting, the problem is that Symfony don't throw or log any message falling in the fallback case, so I didn't notice this until I debug or experience problems with corrupted data in applications. It would be nice if Symfony add at least some warning or even throws a exception if the format is invalid or don't verify the ICU format

@Simperfit
Copy link
Contributor

What could be the best behaviour here ? I guess throwing will be a little too much, just logging a warning maybe the same type when the translation is missing ?

Something like, 'The DateTime format %s is not supported in ICU falling back to default format.'

@shakaran
Copy link
Contributor Author

shakaran commented Nov 4, 2017

@Simperfit yep, logging the warning at least could be more easily to detect in development or production for avoid the data loss or wrong dates set

@Simperfit
Copy link
Contributor

Simperfit commented Nov 5, 2017

@shakaran Could you provide the PR adding this behaviour ?

@shakaran
Copy link
Contributor Author

shakaran commented Nov 9, 2017

@Simperfit it seems that you already address some PR to that with trigger_error, which could be a nice solution unless that someone purpose other way to do it

@ostrolucky
Copy link
Contributor

Hold on, something doesn't add up here.

You have specified single_text as widget option. Code you linked is evaluated only if formatter option is set. And formatter option is set only if widget is not set to single_text.

@Simperfit
Copy link
Contributor

@shakaran Could you please look what @ostrolucky said, it seems that's more of a misunderstanding than a bug.

@shakaran
Copy link
Contributor Author

shakaran commented Apr 4, 2018

@ostrolucky @Simperfit my intention is clear. Use single_text as widget and user formatter option set. I think that it is not a misunderstanding.

The formatter option is set in my code as:

'format' => 'dd/mm/yyyy'

And not:

'format' => 'dd/MM/yyyy'

The regex in the code will not allow "m", only "M".

So if a user puts "m", at least needs trigger a user error or something because if not it is very difficult to detect the data loss in database, because it will be fallback converted

@ostrolucky
Copy link
Contributor

@shakaran I wasn't talking about format option, but formatter attribute.

Once again, this block is not supposed to be executed in your case, because that condition fullfills only when this line executes And that whole branch is executed only for non single_text widgets. So this is impossible to reproduce for us with given information. Are you able to create a small example application that allows to reproduce your issue?

@xabbuh
Copy link
Member

xabbuh commented Apr 26, 2018

I am going to close here for now due to the lack of feedback. Please let us know when you have more information and we can consider to reopen.

@xabbuh xabbuh closed this as completed Apr 26, 2018
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

6 participants