8000 [Form] Fixed reverse transformation of values in DateTimeToStringTransformer by webmozart · Pull Request #6333 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fixed reverse transformation of values in DateTimeToStringTransformer #6333

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 13, 2012
Merged

[Form] Fixed reverse transformation of values in DateTimeToStringTransformer #6333

merged 1 commit into from
Dec 13, 2012

Conversation

webmozart
Copy link
Contributor

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -

The parts not given in the format are reset to the corresponding values of
the UNIX base timestamp. For example, when parsing with the format "Y-m-d",
parsing

"2012-05-18"

now results in the date

"2012-05-18 00:00:00 UTC"

instead of

"2012-05-18 12:58:27 UTC"

as before, where the time part corresponded to the local server time.

Another example: When parsing with the format "H:i:s", parsing

"12:58:27"

now results in

"1970-01-01 12:58:27 UTC"

instead of

"2012-12-13 12:58:27 UTC"

as before, where again the date part corresponded to the local server time.

This behavior is now consistent with DateTimeToArrayTransformer and
DateTimeToLocalizedStringTransformer.

@webmozart webmozart mentioned this pull request Dec 13, 2012
Merged
stof
stof reviewed Dec 13, 2012
View reviewed changes
src/Symfony/Component/Form/CHANGELOG.md
* TrimListener now removes unicode whitespaces
* made DateTimeToStringTransformer consistent with DateTimeToArrayTransformer and
DateTimeToLocalizedStringTransformer. Missing parts of a date now correspond to
the UNIX base time stamp 1970-01-01 00:00:00 instead of the local server time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this in the changelog for 2.2 while submitting the PR to the 2.1 branch looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@webmozart
[Form] Fixed reverse transformation of values in DateTimeToStringTran…
b20c5ca 
…sformer

The parts not given in the format are reset to the corresponding values of
the UNIX base timestamp. For example, when parsing with the format "Y-m-d",
parsing

    "2012-05-18"

now results in the date

    "2012-05-18 00:00:00 UTC"

instead of

    "2012-05-18 12:58:27 UTC"

as before, where the time part corresponded to the local server time.

Another example: When parsing with the format "H:i:s", parsing

    "12:58:27"

now results in

    "1970-01-01 12:58:27 UTC"

instead of

    "2012-12-13 12:58:27 UTC"

as before, where again the date part corresponded to the local server time.

This behavior is now consistent with DateTimeToArrayTransformer and
DateTimeToLocalizedStringTransformer.
fabpot added a commit that referenced this pull request Dec 13, 2012
@fabpot
merged branch bschussek/datetimetostringtransformer (PR #6333)
2447757 
This PR was merged into the 2.1 branch.

Commits
-------

b20c5ca [Form] Fixed reverse transformation of values in DateTimeToStringTransformer

Discussion
----------

[Form] Fixed reverse transformation of values in DateTimeToStringTransformer

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -

The parts not given in the format are reset to the corresponding values of
the UNIX base timestamp. For example, when parsing with the format "Y-m-d",
parsing

    "2012-05-18"

now results in the date

    "2012-05-18 00:00:00 UTC"

instead of

    "2012-05-18 12:58:27 UTC"

as before, where the time part corresponded to the local server time.

Another example: When parsing with the format "H:i:s", parsing

    "12:58:27"

now results in

    "1970-01-01 12:58:27 UTC"

instead of

    "2012-12-13 12:58:27 UTC"

as before, where again the date part corresponded to the local server time.

This behavior is now consistent with DateTimeToArrayTransformer and
DateTimeToLocalizedStringTransformer.
@fabpot fabpot merged commit b20c5ca into symfony:2.1 Dec 13, 2012
@fabpot
Copy link
Member
fabpot commented Dec 15, 2012

@bschussek Apparently, tests are broken because of this PR. see https://travis-ci.org/symfony/symfony/jobs/3676724 for instance.

@webmozart
Copy link
Contributor Author

@fabpot Thanks for the reminder, see #6371

We really need a working Travis again...

@norberttech
Copy link
Contributor

There is a warning "The parsed date was invalid" when parsing 2010-33 on php 5.3.7 so the exception is throwed and tests fails.

Removing 0 < $lastErrors['warning_count'] ||
from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToStringTransformer.php#L145 helps but I'm not sure this is the best solution.

@fabpot
Copy link
Member
fabpot commented Dec 15, 2012

@bschussek The only failing test on Travis is a Form test: https://travis-ci.org/symfony/symfony/jobs/3679013

@jakzal
Copy link
Contributor
jakzal commented Dec 20, 2012

Related issue: #6429

fabpot added a commit that referenced this pull request Dec 23, 2012
@fabpot
merged branch egeloen/date-time-type (PR #3846)
e20b7d9 
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
@webmozart webmozart mentioned this pull request Apr 19, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@webmozart @fabpot @norberttech @jakzal @ahundiak @stof
0