[Form] Added code to use custom date_pattern

[arghav]

Current DateType doesn't make use of the date_pattern option. Added code to use the custom date_pattern if provided.

[maoueh]

You should also pass the pattern option to the DateTimeToLocalizedStringTransformer so the pattern is taken in consideration when converting from and to localized string. You can check the commit I did on my repository (maoueh/symfony@01ae75dd842bc74be270) which do the same as yours for the DateType but also includes the modification needed by the DateTimeToLocalizedStringTransformer class.

Not sure if there is more work needed to fully support the pattern option. Moreover, I did not run the tests to check if everything pass correctly. It would also be a good idea to add some tests to check that the date_pattern is working as expected.

[ghost]

There is also a problem with the regex, but this is not regarding the pattern option. If you set the 'format' option to 0, it returns the IntlDateFormatter::FULL but it does not pass the regex because there is first EEEE in the standard pattern so it comes on the fallback pattern year month day

[arghav]

Thanks for your suggestions @maoueh and @dot-i-fy, I will check them.
I found the date_pattern doesn't work when using text widget. I will try to fix this.

[ghost]

Here the regex I use now and working with IntlDateFormatter::FULL

https://gist.github.com/952929

[maoueh]

@Kertz: It is working for me using the text widget on my development machine. Don't know what is the problem on our side but it is working correctly for me. I'm willing to help track this problem if you want. Just let me know.

[ghost]

@Kertz : It is also working for me, we just have to pay attention about the format to be used in the text input regarding IntlDateFormatter.

[arghav]

Ah well I guess I screwed up the whole commit log!

[ghost]

wow !

[maoueh]

Hell yeah! :) You should do a rebase the next time instead of merging Symfony master branch into yours:
git rebase symfony/master date_pattern and when you need to push your changes to your repository, do git push -f origin date_pattern. This way, it will be easier for the core team to handle the merge process.

Since @dot-i-fy opened a pull request for fixing the same issue as here, maybe it would be a good idea to close this PR?

[arghav]

Well, it's done. I just pushed it a little earlier that I should have! Well I didn't know that @dot-i-fy opened a PR, where is it? This patch currently works for me.

Regarding the change in regex, well I think it should also have a ChoiceList for week days, otherwise it's not useful. Probably that alone can be a separate PR.

[maoueh]

Yep it is much better now :) I think it would be a good idea to remove this commit from your PR kertz/symfony@e47cfb6d49f84a05780c.

The other PR is PR751. Maybe @dot-i-fy could change is PR so it will only be about the regex? In both cases, you should coordinate with each other I think.

Thanks for merging the changes I made to the DateTimeToLocalizedStringTransformer class.

[ghost]

Yeah, I will modify my commit to take only the regex in account.

Would someone work with me on the TimeType ? The pattern is also not working there but it looks like a little bit more complicated!

Thanks

[arghav]

Thanks for the changes in DateTimeToLocalizedStringTransformer.

Is it really necessary to remove the initial commit?

[igorw]

CS

[arghav]

Ah... missed! will fix it :)

[ghost]

What does it mean CS ?

[maoueh]

@dot-i-fy: It means Code Style, the comma is not placed correctly

[arghav]

Coding Standards seems right :) http://symfony.com/doc/2.0/contributing/code/standards.html

[ghost]

ah ok, thx

[maoueh]

@Kertz: I don't think it is strictly necessary to remove this commit. But I think it is a good idea since you kinda reverted it with some changes in a later commit. The core team might ask you to remove it. So leave it for now, and change it if they ask you to do so.

Hehe right, Coding Standards looks way better :)

[maoueh]

@dot-i-fy: I will check later in the evening what can be done about the pattern in the TimeType class. I'm also planning on adding some tests about this PR. If I do so, I will send a PR to your repository @Kertz so you will be able to add the tests to this PR.

[ghost]

I had some problems with PHPUnit, seems to be ok now. Another problem, damned, APC is showing in my browser when in app_dev.php, not in prod. Any idea ? Can't get html output in dev env.

[ghost]

@RapotOR : Are you talking about the dateType method in IntlDateFormatter ? If no can you make a snippet in gist with an example ?

If yes ...The option "format" already allows an IntlDateFormatter input, you can either set a \IntlDateFormatter::MEDIUM for example or an integer representing the dateType to use (0 -> full, 1 -> long, ... ).

The problem we are reveling here is the pattern based off the dateFormat option :

the $formatter look for Locale -> then for format option and based off these options he generate a pattern who can be different regarding the Locale. For example, for me I have my locale in php set to fr_BE, so my dates are always d-m-Y formatted but if I want to modify that it is momently not possible.

Grtz

[RapotOR]

@dot-i-fy : my thought was more about having a full control of IntlDateFormatter from outside; instead of defining every variables. It is more like a DI way... especially if you have more than one DateType field!

[ghost]

Oh so, it may be indeed a nice way to follow. Don't hesitate to submit your code suggestions.
But I think that if we go deeper in the core modifications, it is maybe a solution to take the $formatter out of the dateType class and put it in a own class and call it from the dateType class ???@}#

[maoueh]

@mweimerskirch: Maybe it would be better to rename your option html_pattern? Since the pattern is not strictly associated to a date type, it would be a better name in my opinion if it was named html_pattern or html5_pattern?

I think it would lead to more misunderstandings if we had a date_pattern and a pattern option. In both cases, I agree totally that one or the other needs a renaming. Moreover, if we change the pattern option in the date type, I think the format option should be changed also to date_format.

What do you think?

[arghav]

@mweimerskirch

I too think when we specify pattern in DateType everyone expects it to be the date pattern. So maybe renaming the HTML5 pattern to html_pattern would be more appropriate?

[webmozart]

Looks good. Why don't we reuse the "format" option for this? If "format" is not one of the predefined constants, we could treat it as a custom date format.

[maoueh]

@bschussek: I think it is a good idea indeed. When I first played with forms, I thought that the purpose of the format option was exactly for this but I soon realized it wasn't. I'm +1 for this.

@Kertz: Let me know if you don't have time to do this switch, I will gladly submit the changes to you own repo if we agree to use "format" option instead of the "pattern" one.

[ghost]

format and pattern options are related to the IntlDateFormatter, I also agree with you but we have to keep in mind that we will loose the symmetry with the IntlDateFormatter class.

[webmozart]

I think we can safely add this level of abstraction.

[arghav]

@maoueh Would be great if you can make the changes :)

[ghost]

@bschussek : I saw you removed pattern option, what are further intentions ?

[maoueh]

@Kertz: I will do the changes needed tomorrow as I have some spare times. You will need to sync your branch with current master or I will need to send another PR since @bschussek removed the pattern option which will cause bad conflicts with this implementation.

[arghav]

@maoueh I've merged the changes.

[arghav]

@maoueh I just removed a couple of commits from the log. Seems like the tests you committed are now missing from the PR. Can you please send the tests once again? Sorry about that.

[maoueh]

@Kertz: I will resend them along with the code modifications to use format instead of pattern.

[maoueh]

I did the changes to make it possible to pass a custom pattern via the format option. I sent a PR on @Kertz repository here that will be merge eventually in this PR.

I have two small questions about it. First, since format option can now be a string, the allowed values for the option format have been removed from the array returned by getAllowedOptionValues. The check is done instead in the method buildForm directly. The 'format' option can be either one of the IntlDateFormatter constants (FULL, LONG, MEDIUM, or SHORT) or a string. If those conditions are not respected, a FormException is thrown. Is this correct?

When the format option is a custom pattern, the IntlDateFormatter needs a valid format even if it will not be used. So I retrieved the default format option by using the getDefaultOptions method. Is this correct or should I specify the default value directly:

this (specify directly):

$format = \IntlDateFormatter::MEDIUM;

instead of (use predefined defaults):

$defaultOptions = $this->getDefaultOptions($options);
$format = $defaultOptions['format'];

Also added more tests to verify that the format option is validated correctly and updated previous ones.

Regards,
Matt

[arghav]

Merged the changes. I have not tested this yet... Thanks @maoueh :)

[ghost]

nice job @maoueh

[vicb]

May be this should be a CreationException to match how FormFactory works ?

[maoueh]

That what I was about to do but when I checked in DateType.php, I noticed that the validation for the input option is throwing a FormException. Should I changed both? Here the line in question:

} else if ($options['input'] !== 'datetime') {
    throw new FormException('The "input" option must be "datetime", "string", "timestamp" or "array".');
}

In fact, I think this line is a dead end with the addition of the getAllowedOptionValues that check if the input option has a correct value. I will change the exception to a CreationException and wait to see if the line should be removed or changed.

[vicb]

You are absolutely right. The code should have been updated with the introduction of getAllowedOptionValues and CreationException. The best would be to remove the dead if case (This is no more required and can even prevent from being able to extend the date type).

[maoueh]

I changed the FormException to a CreationException in the code found in this PR. I noticed @Kertz about it so he could merge it in his branch.

Also sent another PR #1030 to remove the dead code and the use statement as you suggested.

[arghav]

@maoueh Merged, thank you. :)

[mprizmic]

what do you think about
$pattern = $form->getAttribute('pattern');
instead of
$pattern = $form->getAttribute('formatter')->getPattern();
in line 96 of
symfony/component/form/extension/core/type/datetype.php

[maoueh]

It is simpler to use the pattern of the formatter directly I think. The reason is that if the option to change the format is null, the default pattern of the formatter will be available and will be the right pattern to use. If the option is set, we modify the formatter before hand so getting the pattern from it will return the custom pattern we have set in the options.

So in both cases, no conditional is required to verify if it is set or not when retrieving the pattern from the formatter. Moreover, I think the pattern must be set as an attribute of the form for your suggestion to work correctly which is not the case right now.

Regards,
Matt

[stloyd]

@fabpot What we do with that ? I have tried to rebase it with master, but my (Windows) git always gets insane and this ends up with unfixable error. This feature should be in Symfony2 before final. Should I start work on it from "none" ?

[fabpot]

@stloyd: I need to review the patch first.

[maoueh]

Hi,

Thank you to have merged this patch. I won't need to package a custom symfony release on my own anymore :)

Regards and have a great day,
Matt