ad (1): HTML4 "id" attributes are limited to strings starting with a letter and containing only letters, digits, underscores, hyphens, periods and colons.
ad (2): Property paths contain three special characters needed for correct parsing: left/right bracket and period.
The rules for form naming are:
* Names may start with a letter, a digit or an underscore. Leading digits or underscores will be stripped from the "id" attributes.
* Names must only contain letters, digits, underscores, hyphens and colons.
* Root forms may have an empty name.
Solves #1919 and #3021 on a wider scope.
* 2.0:
[Tests] Skip segfaulting form test
Rename test file
[BrowserKit] added missing @return PHPDoc for the Client::submit() method.
also test PHP 5.3.2, since this is the official lowest supported PHP version
Commits
-------
49d2685 [Form] Add default validation to TextType field (and related)
Discussion
----------
[Form] Add default transformer to TextType field (and related)
Bug fix: yes&no (?)
Feature addition: yes (?)
BC break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1962.
---------------------------------------------------------------------------
by stloyd at 2011/12/19 03:43:37 -0800
@fabpot ping ;-)
---------------------------------------------------------------------------
by fabpot at 2011/12/19 10:58:20 -0800
Is it really needed? I have a feeling that it enforces unneeded constraints, but I can be wrong of course.
---------------------------------------------------------------------------
by hlecorche at 2011/12/20 02:31:03 -0800
It's needed because with TextType field, and without the ValueToStringTransformer, the user data (when sending the form) can be an array !!!
For example:
- if there is a TextType field
- and if there is a MaxLengthValidator
- and if the user data (when sending the form) is an array
So the exception "Expected argument of type string, array given in src\Symfony\Component\Validator\Constraints\MaxLengthValidator.php at line 40" is thrown
* 2.0:
[Form] fixed previous merge
[Form] simplified previous merge
Also identify FirePHP by the X-FirePHP-Version header
[TwigBundle] Extract output buffer cleaning to method
[TwigBundle] Do not clean output buffering below initial level
Fixed rendering of FileType (value is not a valid attribute for input[type=file])
Added tests for string fix in DateTimeToArrayTransformer (8351a11286).
Added check for array fields to be integers in reverseTransform method. This prevents checkdate from getting strings as arguments and throwing incorrect ErrorException when submitting form with malformed (string) data in, for example, Date field. #2609
[Translation] removed unneeded methods
[Translation] added detection for circular references when adding a fallback catalogue
[DomCrawler] trim URI in getURI
[Yaml][Tests] Fixed missing locale string for Windows platforms which caused test to fail
Commits
-------
e83e00a Fixed rendering of FileType (value is not a valid attribute for input[type=file])
Discussion
----------
Fixed rendering of FileType
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
According to the W3C validator, `value` is not a valid attribute for `input[type=file]`.
---------------------------------------------------------------------------
by fabpot at 2011/11/10 23:01:24 -0800
Instead of creating yet another block, what about modifying the `field_widget` to not render the `value` attribute if the value is empty? Also, the PHP template must be fixed too.
---------------------------------------------------------------------------
by jalliot at 2011/11/11 02:02:52 -0800
@fabpot Changed ;)
Commits
-------
d08ec5e Add DelegatingValidator tests
e1822e7 Enable dynamic set of validation groups by a callback or Closure
Discussion
----------
[Form][Validator] Enable dynamic set of validation groups based on callback
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Symfony2 tests written for new feature: yes
closes tickets: #2498#1151
This will allow developer to pass a Closure or a callback array as a Validation groups option of a form. Eg:
```
class ClientType extends AbstarctType
{
// ...
public function getDefaultOptions(array $options)
{
return array(
'validation_groups' => function(FormInterface $form){
// return array of validation groups based on submitted user data (data is after transform)
$data = $form->getData();
if($data->getType() == Entity\Client::TYPE_PERSON)
return array('Default', 'person');
else
return array('Default', 'company');
},
);
}
// ...
}
```
```
class ClientType extends AbstarctType
{
// ...
public function getDefaultOptions(array $options)
{
return array(
'validation_groups' => array(
'Acme\\AcmeBundle\\Entity\\Client',
'determineValidationGroups'
),
);
}
// ...
}
```
This will make developers life easier !
---------------------------------------------------------------------------
by schmittjoh at 2011/10/27 06:39:56 -0700
Does that work if your ClientType were added to another form type?
e.g.
```php
<?php
class MyComplexType extends AbstractType
{
public function buildForm(FormBuilder $builder, array $options)
{
$builder->add('client', new ClientType());
}
// ...
}
```
---------------------------------------------------------------------------
by canni at 2011/10/27 06:44:33 -0700
This is doing nothing more than injecting array of validation groups, should work, but I have not tested this use case.
---------------------------------------------------------------------------
by canni at 2011/10/28 01:58:26 -0700
PHPUnit output
```
OK, but incomplete or skipped tests!
Tests: 5011, Assertions: 12356, Incomplete: 36, Skipped: 32.
```
---------------------------------------------------------------------------
by canni at 2011/11/02 11:37:47 -0700
Now functionality is complete, test are written, and implementation is clean. :)
---------------------------------------------------------------------------
by stloyd at 2011/11/02 11:50:44 -0700
Can tou `squash` your commits ? Thanks.
---------------------------------------------------------------------------
by canni at 2011/11/02 11:58:41 -0700
Done
---------------------------------------------------------------------------
by fabpot at 2011/11/07 07:51:18 -0800
Can you add some tests for the `DelegatingValidator` class, which is where we can ensure that the new feature actually works as expected?
---------------------------------------------------------------------------
by canni at 2011/11/07 13:53:16 -0800
OK, I've written proof-of-concept tests, also I've squashed few commits to make things clear.
Personally I think this should go straight into 2.0 series, as it do not beak BC, and a feature is really nice to use.
---------------------------------------------------------------------------
by stof at 2011/11/07 14:17:15 -0800
@canni the 2.0 branch is for bug fixes, not for new features. This is the difference between maintenance releases and minor releases.
This will enable developer, to set a callback or a closure as a `'validation_groups'` form option,
this is usefull when we have to determine validation groups based on a client submitted data.
Commits
-------
fbd2a0e make suggested changes for default value
c507b1d update variable name to match the option name
b53f000 add the ability to set the form prototype name in CollectionType. this will aid in handling nested collections in forms
Discussion
----------
[Form] Collection
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: [#1324](https://github.com/symfony/symfony/issues/1324)
add the ability to set the form prototype name in CollectionType. this will aid in handling nested collections in forms
---------------------------------------------------------------------------
by IamPersistent at 2011/10/17 02:54:45 -0700
Actually, as an afterthought, I looked over at the issues and this is basically https://github.com/symfony/symfony/issues/1324 just adding an extra option instead of using the prototype option.
@stloyd, my thought was handling the case where someone was "clever" enough to set the 'prototype_name' option to "". But I could be over-thinking :)
---------------------------------------------------------------------------
by stloyd at 2011/10/17 03:00:23 -0700
@IamPersistent IMO if someone is setting this option, he should be aware of that problem, also AFAIK `$$$$` could be *valid* name too ;-)
---------------------------------------------------------------------------
by IamPersistent at 2011/10/17 03:02:14 -0700
@stloyd, I'm fine with changing it, I'll wait to see what everyone else has to say about this request vs using the prototype option for the name in 1324
---------------------------------------------------------------------------
by IamPersistent at 2011/10/17 03:28:49 -0700
@stloyd, @stof, I made the suggested changes, now I suppose, let the debate begin over PR1324 vs this
---------------------------------------------------------------------------
by stloyd at 2011/10/17 03:47:53 -0700
IMO this PR makes changing prototype name in more clean way, so I would prefer this one over that proposed in #1324.
Commits
-------
c9d05d7 Let NumberFormatter handle integer type casting
Discussion
----------
Let NumberFormatter handle integer type casting
The integer to localised string transformer is currently casting everything it gets to an integer, even if it is not a number. This responsibility should be passed off to NumberFormatter.
Partially addresses #2389 by not mistakenly typecasting a boolean false into an integer 0
---------------------------------------------------------------------------
by mrtorrent at 2011/10/30 15:04:28 -0700
Apologies, forgot the template:
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2389 (partial)
Commits
-------
95049ef [Form] Added type check to `ScalarToChoiceTransformer`
Discussion
----------
[2.0][Form] Added type check to ScalarToChoiceTransformer
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
* 2.0:
[Validator] added support for grapheme_strlen when mbstring is not installed but intl is installed
removed separator of choice widget when the separator is null
Commits
-------
67c33a8 Rebased with master, and fixed wrong behavior with proper tests coverage
f8a6a4b Be sure that both fields have same value for required option in RepeatedType
0679220 Additional test coverage for changes in RepeatedType
b23d47d moved options test form from class->method scope
5fe5556 fixed accidental permission change
a969434 [Form] fixed CS, merged options, added tests
8819db3 [Form] Allow setting different options to repeating fields
Discussion
----------
[2.1] [Form] Allow setting different options at RepeatedType fields
This an test covered version of #1348 (rebased with master).
---------------------------------------------------------------------------
by stloyd at 2011/06/27 04:18:19 -0700
@fabpot What do you think about this ? I'm just not sure that we should allow setting `required` per field, IMO better would be forcing this option from default `$options['options']` and ignore that field in `$options['first_options']` and/or `$options['second_options']`.
---------------------------------------------------------------------------
by stloyd at 2011/07/02 00:00:04 -0700
@fabpot ping.
---------------------------------------------------------------------------
by fabpot at 2011/07/06 05:45:56 -0700
Let's discuss this new feature for 2.1.
---------------------------------------------------------------------------
by stloyd at 2011/08/24 01:12:59 -0700
Rebased with master.
---------------------------------------------------------------------------
by stof at 2011/09/04 05:02:42 -0700
@fabpot What do you think about this feature ? It is now time to discuss it :)
---------------------------------------------------------------------------
by fabpot at 2011/09/22 00:18:29 -0700
Tests do not pass.
---------------------------------------------------------------------------
by stloyd at 2011/09/24 01:54:42 -0700
@fabpot Should be ok now.
Commits
-------
c29fa9d [Form] Fix for treatment zero as empty data. Closes#1986
Discussion
----------
[Form] Fix for treatment zero as empty data. Closes#1986
For more info please read #1986.
Revert "[Form] CollectionType now checks for data_class parameter instead of only class."
This reverts commit 2e024f87a3.
Conflicts:
tests/Symfony/Tests/Component/Form/Extension/Core/Type/CollectionTypeTest.php
Revert "[Form] Added ObjectFactoryListener. Fixes #1746."
This reverts commit 0327beb0b9.
Conflicts:
tests/Symfony/Tests/Component/Form/Extension/Core/Type/CollectionTypeTest.php
Commits
-------
52fdd53 [Bridge/Twig] Add required class to labels that match required fields
Discussion
----------
[Bridge/Twig] Add required class to labels that match required fields
I have used this to simply style labels that are required with a red star behind them using this CSS:
``` css
label.required::after {
content: " *";
color: #c00;
}
```
The problem is that you can't use `input[required] + label::after` as a selector since the label is typically rendered before the input. There is no way to check for an element that is *followed by* another, only elements *following*.
Of course this CSS in particular won't work except in the latest browsers, but you could still use the `label.required` selector to add a background image and so on. I think this is a very common use case and therefore I think it'd benefit the core framework.
---------------------------------------------------------------------------
by fabpot at 2011/07/05 01:27:49 -0700
Can you also update the PHP templates?
---------------------------------------------------------------------------
by schmittjoh at 2011/07/05 01:43:33 -0700
How about namespacing these css classes, like for example "sf-form-required"?
---------------------------------------------------------------------------
by stloyd at 2011/07/05 01:50:58 -0700
I would prefer an @schmittjoh naming, or even adding ability to setup it thought options.
---------------------------------------------------------------------------
by fabpot at 2011/07/05 01:54:36 -0700
Please, do not add more options. Prefix with `sf` is actually a good idea but people will argue that this is not a good idea because it gives too much information about the technology used to create the website (that's one of the things that came up pretty often in symfony1).
---------------------------------------------------------------------------
by schmittjoh at 2011/07/05 02:00:11 -0700
An option is not such a good idea imo since you likely want to have a uniform naming strategy across your entire site. How about adding a new service CssNamingStrategy?
---------------------------------------------------------------------------
by stloyd at 2011/07/05 02:01:19 -0700
Then this can be some simpler one not giving such informations i.e.: `form-label-required`, `label-required`, `framework-form-required`, `form-required` or whatever else ;-)
---------------------------------------------------------------------------
by fabpot at 2011/07/05 02:16:41 -0700
It cannot be configurable as it would potentially break bundles that come with stylesheets.
---------------------------------------------------------------------------
by stloyd at 2011/07/05 02:21:10 -0700
IMO if we decide to add this one, we could add same to `inputs/selects/etc` with `required` option.
---------------------------------------------------------------------------
by schmittjoh at 2011/07/05 02:21:59 -0700
I think it can, consider an interface like this:
```php
interface CssNamingStrategyInterface
{
function getCssName($class);
}
```
This will give people a lot of flexibility, and it also does allow them to exclude classes which for example are provided by third-party bundles.
---------------------------------------------------------------------------
by Seldaek at 2011/07/05 02:47:54 -0700
Wow guys, if this turns into a full blown class generator solution, I'm happy to close the PR.
"required" is not a name that's commonly used for main page elements, it's typically associated with forms, and therefore I don't see the need to make it unnecessary longer/namespaced. Similarly I don't see the need to add it to the input/select/.., because they already have an attribute, which you can very easily select as: `input[required]` in CSS. That works everywhere except IE6, but we can't build for the future on very old browsers. If you really want support for IE6, you can override the templates imo. But core should be looking forward, as it already is with HTML5, form markup, etc.
As for calling it form-label-required or label-required, again, I don't see the benefit, you can use `label.required` if you want to avoid conflicts with non-label elements having a required class, or a safer `form .required`. There are plenty of options in CSS itself, let's not make this overly complex.
---------------------------------------------------------------------------
by schmittjoh at 2011/07/05 02:52:17 -0700
see
http://code.google.com/intl/de-DE/speed/page-speed/docs/rendering.html#UseEfficientCSSSelectors
On Tue, Jul 5, 2011 at 11:47 AM, Seldaek <
reply@reply.github.com>wrote:
> Wow guys, if this turns into a full blown class generator solution, I'm
> happy to close the PR.
>
> "required" is not a name that's commonly used for main page elements, it's
> typically associated with forms, and therefore I don't see the need to make
> it unnecessary longer/namespaced. Similarly I don't see the need to add it
> to the input/select/.., because they already have an attribute, which you
> can very easily select as: `input[required]` in CSS. That works everywhere
> except IE6, but we can't build for the future on very old browsers. If you
> really want support for IE6, you can override the templates imo. But core
> should be looking forward, as it already is with HTML5, form markup, etc.
>
> As for calling it form-label-required or label-required, again, I don't see
> the benefit, you can use `label.required` if you want to avoid conflicts
> with non-label elements having a required class, or a safer `form
> .required`. There are plenty of options in CSS itself, let's not make this
> overly complex.
>
> --
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/1519#issuecomment-1502560
>
---------------------------------------------------------------------------
by Seldaek at 2011/07/05 03:11:50 -0700
Really? Come on, we're talking about forms, it's not like you have billions of form/input tags per page that have to be parsed by the browser when you select that. Also you don't have to select the elements, if you want true performance just use no stylesheet, your users will thank you.
---------------------------------------------------------------------------
by schmittjoh at 2011/07/05 03:30:40 -0700
Your CSS selectors not only affect the performance of form elements, but of all elements that have a "required" class. Likewise, the same applies if we decide to add more classes.
Why close the door for people who care about performance? We can easily avoid this by making the css class more specific as suggested earlier. The idea with the renaming strategy is one step further and allows people to "obfuscate" which tool was used to generate the form, or do additional optimizations like shortening the css name.
---------------------------------------------------------------------------
by lenar at 2011/07/05 03:34:34 -0700
@Seldaek: Just for remark I've seen matrix forms spanning multiple screenfuls horizontally and vertically
containing tens of thousands inputs. Not pretty, but they do exist. Basically "poor" man's/company's excel
emulation or something.
---------------------------------------------------------------------------
by Seldaek at 2011/07/05 04:19:13 -0700
@schmittjoh, @lenar: We're catering to the most common use case, for which this will be more than fast enough. Small/medium scale websites don't have to optimize on CSS rules parsing, they usually have much bigger issues to deal with. If you really care about it, overriding the block to remove the class is just as easy as it was to for me to add it, but IMO this is the edge case.
---------------------------------------------------------------------------
by schmittjoh at 2011/07/05 05:37:19 -0700
IMO Symfony should follow best practices by encouraging to use class selectors, and not tag selectors for the reasons explained on the page I linked.
Anyway, I think everybody made his points. Time for @fabpot to make a decision :)
Commits
-------
ef022c0 Removed the magical guessing of the type name to avoid WTF issues
Discussion
----------
Removed the magical guessing of the type name to avoid WTF issues
As discussed on IRC, this removes the magical method to avoid breaking the user-land code by reusing the same name than another type which overrides it. This makes the user aware that a type should have a name as they now have to implement the method themselves.
---------------------------------------------------------------------------
by jalliot at 2011/07/06 05:35:30 -0700
Doc and generator should be modified as well if it's merged.
Commits
-------
d08a688 [Form] Fixed CS
954bdb5 [Form] Updated DateTimeType to accept a custom date pattern for the DateType child * Added a test also about this change
e7e744f [Form] Synced changes in this branch with current Symfony master branch
436cb95 [Form] Changed to a CreateException when the 'format' option is invalid * Updated DateTypeTest also
0045ffe [Form] Added tests to check that the date format option is validated correctly * Format option must be either a IntlDateFormatter constants (FULL, LONG, MEDIUM, SHORT) or a string
a815232 [Form] The IntlDateFormatter pattern can now be passed via the format option * Also changed the default value of the calendar paramter to \IntlDateFormatter:GREGORIAN in DateTimeToLocalizedStringTransformer which is the same as the default value in StubIntlDateFormatter
58f869a [Form] Synced custom pattern tests with master branch
c20edde [Form] Added some tests * Tests to check if the pattern option is handled correctly by DateType * Tests to check if the pattern parameter is handled correctly by DateTimeToLocalizedStringTransformer
52a1e1d moved date_pattern to IntlDateFormatter
dd104bc added code to use custom date_pattern
Discussion
----------
[Form] Added code to use custom date_pattern
Current DateType doesn't make use of the `date_pattern` option. Added code to use the custom `date_pattern` if provided.
---------------------------------------------------------------------------
by maoueh at 2011/05/02 21:52:37 -0700
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@01ae75dd84) 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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/02 22:02:14 -0700
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
---------------------------------------------------------------------------
by kertz at 2011/05/02 23:36:05 -0700
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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 00:02:45 -0700
Here the regex I use now and working with IntlDateFormatter::FULL
https://gist.github.com/952929
---------------------------------------------------------------------------
by maoueh at 2011/05/03 07:13:01 -0700
@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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 07:57:34 -0700
@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.
---------------------------------------------------------------------------
by kertz at 2011/05/03 11:15:23 -0700
Ah well I guess I screwed up the whole commit log!
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 11:19:08 -0700
wow !
---------------------------------------------------------------------------
by maoueh at 2011/05/03 11:23:51 -0700
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?
---------------------------------------------------------------------------
by kertz at 2011/05/03 11:33:34 -0700
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.
---------------------------------------------------------------------------
by maoueh at 2011/05/03 11:46:09 -0700
Yep it is much better now :) I think it would be a good idea to remove this commit from your PR kertz/symfony@e47cfb6d49.
The other PR is [PR751](https://github.com/symfony/symfony/pull/751). 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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 12:00:06 -0700
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
---------------------------------------------------------------------------
by kertz at 2011/05/03 12:02:18 -0700
Thanks for the changes in DateTimeToLocalizedStringTransformer.
Is it really necessary to remove the initial commit?
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 12:06:41 -0700
What does it mean CS ?
---------------------------------------------------------------------------
by maoueh at 2011/05/03 12:08:44 -0700
@dot-i-fy: It means Code Style, the comma is not placed correctly
---------------------------------------------------------------------------
by kertz at 2011/05/03 12:11:24 -0700
`Coding Standards` seems right :) http://symfony.com/doc/2.0/contributing/code/standards.html
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 12:11:27 -0700
ah ok, thx
---------------------------------------------------------------------------
by maoueh at 2011/05/03 12:12:10 -0700
@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 :)
---------------------------------------------------------------------------
by maoueh at 2011/05/03 12:30:00 -0700
@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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/03 13:04:35 -0700
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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/04 00:43:46 -0700
@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
---------------------------------------------------------------------------
by RapotOR at 2011/05/04 00:57:03 -0700
@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!
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/04 01:11:21 -0700
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 ???@}#
---------------------------------------------------------------------------
by maoueh at 2011/05/05 08:22:40 -0700
@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?
---------------------------------------------------------------------------
by kertz at 2011/05/05 23:58:21 -0700
@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?
---------------------------------------------------------------------------
by bschussek at 2011/05/18 12:20:56 -0700
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.
---------------------------------------------------------------------------
by maoueh at 2011/05/18 12:28:22 -0700
@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.
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/18 12:33:33 -0700
``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.
---------------------------------------------------------------------------
by bschussek at 2011/05/18 13:39:07 -0700
I think we can safely add this level of abstraction.
---------------------------------------------------------------------------
by kertz at 2011/05/18 20:29:11 -0700
@maoueh Would be great if you can make the changes :)
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/19 09:09:41 -0700
@bschussek : I saw you removed pattern option, what are further intentions ?
---------------------------------------------------------------------------
by maoueh at 2011/05/19 09:42:40 -0700
@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.
---------------------------------------------------------------------------
by kertz at 2011/05/20 06:45:22 -0700
@maoueh I've merged the changes.
---------------------------------------------------------------------------
by kertz at 2011/05/20 07:11:36 -0700
@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.
---------------------------------------------------------------------------
by maoueh at 2011/05/20 09:03:52 -0700
@kertz: I will resend them along with the code modifications to use `format` instead of `pattern`.
---------------------------------------------------------------------------
by maoueh at 2011/05/20 19:11:28 -0700
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](https://github.com/kertz/symfony/pull/2) 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
---------------------------------------------------------------------------
by kertz at 2011/05/20 20:38:32 -0700
Merged the changes. I have not tested this yet... Thanks @maoueh :)
---------------------------------------------------------------------------
by dot-i-fy at 2011/05/20 22:48:47 -0700
nice job @maoueh
---------------------------------------------------------------------------
by mprizmic at 2011/06/16 14:06:47 -0700
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
---------------------------------------------------------------------------
by maoueh at 2011/06/16 14:40:37 -0700
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
---------------------------------------------------------------------------
by stloyd at 2011/07/05 13:41:36 -0700
@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" ?
---------------------------------------------------------------------------
by fabpot at 2011/07/06 05:44:26 -0700
@stloyd: I need to review the patch first.
Commits
-------
3917ed7 Revert "* DateType, DateTimeType, TimeType: - a bit changed readability"
c85b815 Fixed few issues with Date and Time:
Discussion
----------
[Form] Fixed few issues with Date and Time
Fixed few issues with Date and Time:
* TimeType:
- seconds are no longer populated if "with_seconds" = false
- "widget = text" is now properly rendered (closes#1480)
* DateTimeToStringTransformer:
- fixed using not default "format" (probably fix#1183)
* DateType, DateTimeType, TimeType:
- fixed "input = datetime" and test covered
* TimeType:
- seconds are no longer populated if "with_seconds" = false
- "widget = text" is now properly rendered (closes#1480)
* DateTimeToStringTransformer:
- fixed using not default "format" (probably fix#1183)
* DateType, DateTimeType, TimeType:
- fixed "input = datetime" and test covered
- a bit changed readability
