[Form] Add option with_minutes to the DateTimeType & TimeType #3846
Conversation
|
You should also extend tests for those |
|
Oups, I have looked at tests but I didn't find it at my first reading. I will do it :) |
|
@egeloen Here you can find tests for Form Types: https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Form/Tests/Extension/Core/Type |
|
@stloyd I have added tests. Can you give me some feedbacks ? |
|
@egeloen I'm not sure if we should allow user to set |
|
Yes, you're right. But I'm unsure how can I do this following the good way. |
|
Just make it throw an InvalidConfigurationException.php exception, no? :) |
|
I have updated the PR in order to throw an |
| @@ -42,24 +53,36 @@ public function buildForm(FormBuilderInterface $builder, array $options) | |||
| $hourOptions = $minuteOptions = $secondOptions = array(); | |||
|
|
|||
| if ('choice' === $options['widget']) { | |||
| $hours = $minutes = array(); | |||
| if (is_array($options['empty_value'])) { | |||
There was a problem hiding this comment.
As far as I see, lines 56-60 are useless, as they are covered by the $emptyValueFilter in the method setDefaultOptions. Can you please remove them? Otherwise this patch looks good.
|
@bschussek I have removed the useless code. I think I have found an issue about my PR. I have added 3 tests in order to show it. It seems if we disable minutes, the text widget is broken. |
|
@egeloen can you rebase your PR as it conflicts with master ? |
|
@stof rebase Like explain previously, my PR is still failling if we disable minutes & use the text widget. |
|
I have fixed the last issue. IMO, the PR can now be merge. |
|
@bschussek @fabpot ping |
|
@bschussek Do yo think this PR can be merge? |
|
@fabpot is there something missing before merging? |
|
I'm waiting for @bschussek approval. |
|
I really need the |
|
@geoffrey-brier you could do you own FieldType that extends the current one and add the option your self. |
| 'with_minutes' => false, | ||
| )); | ||
|
|
||
| $form->bind('03:04:05'); |
|
@bschussek Yes... :) I have updated the PR according to your feedback. I needed to update the By the way, why does it first try to create a |
|
@egeloen Good question, I think the transformer is a bit flawed there. I'm working on that. The rest of the PR looks good. Thank you! |
|
I fixed the transformer in #6333. Once that is merged into 2.1, and once 2.1 is merged into master after that, you can rebase this PR on master. Then we can merge it. |
|
I have rebased & squashed commits. The PR is ready to merge. ping @fabpot |
This PR was merged into the master branch. Commits ------- bf9e238 [Form] Add options with_minutes to DateTimeType & TimeType Discussion ---------- [Form] Add option with_minutes to the DateTimeType & TimeType Bug fix: no Feature addition: yes Backwards compatibility break: no Fixes the following tickets: - Todo: - Hey, One of my project requires the datetime usage only with hours. I have submit a patch allowing to disable minutes like seconds are disabled. --------------------------------------------------------------------------- by stloyd at 2012-04-09T16:26:11Z You should also extend tests for those `Types` --------------------------------------------------------------------------- by egeloen at 2012-04-09T16:31:51Z Oups, I have looked at tests but I didn't find it at my first reading. I will do it :) --------------------------------------------------------------------------- by stloyd at 2012-04-09T16:34:42Z @egeloen Here you can find tests for Form Types: https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Form/Tests/Extension/Core/Type --------------------------------------------------------------------------- by egeloen at 2012-04-09T16:42:42Z @stloyd I have added tests. Can you give me some feedbacks ? --------------------------------------------------------------------------- by stloyd at 2012-04-09T16:46:33Z @egeloen I'm not sure if we should allow user to set `with_minutes=false` and `with_seconds=true`. But in overall seems quite ok. --------------------------------------------------------------------------- by egeloen at 2012-04-09T16:51:37Z Yes, you're right. But I'm unsure how can I do this following the good way. --------------------------------------------------------------------------- by inanimatt at 2012-05-03T15:46:02Z Just make it throw an InvalidConfigurationException.php exception, no? :) --------------------------------------------------------------------------- by egeloen at 2012-06-09T18:27:41Z I have updated the PR in order to throw an ``InvalidConfigurationException`` if we enable seconds & disable minutes. --------------------------------------------------------------------------- by egeloen at 2012-07-09T19:08:11Z @bschussek I have removed the useless code. I think I have found an issue about my PR. I have added 3 tests in order to show it. It seems if we disable minutes, the text widget is broken. --------------------------------------------------------------------------- by stof at 2012-10-13T16:00:43Z @egeloen can you rebase your PR as it conflicts with master ? --------------------------------------------------------------------------- by egeloen at 2012-10-13T17:15:22Z @stof rebase Like explain previously, my PR is still failling if we disable minutes & use the text widget. --------------------------------------------------------------------------- by egeloen at 2012-10-13T18:09:03Z I have fixed the last issue. IMO, the PR can now be merge. --------------------------------------------------------------------------- by stof at 2012-10-13T18:20:00Z @bschussek @fabpot ping --------------------------------------------------------------------------- by egeloen at 2012-10-16T18:13:00Z @bschussek Do yo think this PR can be merge? --------------------------------------------------------------------------- by egeloen at 2012-10-30T19:14:00Z @fabpot is there something missing before merging? --------------------------------------------------------------------------- by fabpot at 2012-10-31T08:22:55Z I'm waiting for @bschussek approval. --------------------------------------------------------------------------- by geoffrey-brier at 2012-11-13T10:49:52Z I really need the `with_minute => false` enhancement on a project as I don't want to write CSS/JS hacks, could @bschussek approve/disapprove it so that I can make a decision? --------------------------------------------------------------------------- by henrikbjorn at 2012-11-13T10:52:12Z @geoffrey-brier you could do you own FieldType that extends the current one and add the option your self. --------------------------------------------------------------------------- by egeloen at 2012-11-13T13:20:44Z @bschussek Yes... :) I have updated the PR according to your feedback. I needed to update the `DateTimeToStringTransformer` because it tries to create a `DateTime` only from the value (with no format). In my case, the `'03'` value is not enougt to create it. So, if the date time creation fails, it then try to create the datetime from the format. I don't know if it is the best approach but it works well. By the way, why does it first try to create a `DateTime` without format, **then only** try to use the format ? --------------------------------------------------------------------------- by bschussek at 2012-11-13T14:20:13Z @egeloen Good question, I think the transformer is a bit flawed there. I'm working on that. The rest of the PR looks good. Thank you! --------------------------------------------------------------------------- by bschussek at 2012-12-13T18:14:58Z I fixed the transformer in #6333. Once that is merged into 2.1, and once 2.1 is merged into master after that, you can rebase this PR on master. Then we can merge it. --------------------------------------------------------------------------- by egeloen at 2012-12-22T14:54:38Z I have rebased & squashed commits. The PR is ready to merge. ping @fabpot
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Fixes the following tickets: -
Todo: -
Hey,
One of my project requires the datetime usage only with hours. I have submit a patch allowing to disable minutes like seconds are disabled.