8000 [Form] Remove timezone options from DateType and TimeType by jakzal · Pull Request #12404 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Form] Remove timezone options from DateType and TimeType #12404

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

Closed
wants to merge 1 commit into from

Conversation

jakzal
Copy link
Contributor
@jakzal jakzal commented Nov 3, 2014
Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #12239
License MIT
Doc PR symfony/symfony-docs#4417

replaces #12380

@@ -63,6 +63,7 @@ CHANGELOG
* fixed CSRF error message to be translated
* custom CSRF error messages can now be set through the "csrf_message" option
* fixed: expanded single-choice fields now show a radio button for the empty value
* [BC BREAK] drop support for model_timezone and view_timezone options in TimeType and DateType
Copy link
Member

Choose a reason for hiding this comment

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

This must be moved to the proper section (2.6 or 2.7).

Copy link
Member

Choose a reason for hiding this comment

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

By the way, this also affect the BirthdayType.

@jakzal jakzal force-pushed the remove-timezone-options-master branch from fca119e to 9b23dfd Compare November 3, 2014 22:37
@fabpot
Copy link
Member
fabpot commented Nov 16, 2014

Thank you @jakzal.

fabpot added a commit that referenced this pull request Nov 16, 2014
…ype (jakzal)

This PR was submitted for the master branch but it was merged into the 2.6 branch instead (closes #12404).

Discussion
----------

[Form] Remove timezone options from DateType and TimeType

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12239
| License       | MIT
| Doc PR        | symfony/symfony-docs#4417

replaces #12380

Commits
-------

849fb29 Drop support for model_timezone and view_timezone options in TimeType and DateType.
@fabpot fabpot closed this Nov 16, 2014
@jakzal jakzal deleted the remove-timezone-options-master branch November 16, 2014 18:37
fabpot added a commit that referenced this pull request Nov 17, 2014
…xabbuh)

This PR was merged into the 2.6 branch.

Discussion
----------

[Form] *_timezone changes also affect the BirthdayType

| Q             | A
| ------------- | ---
| Fixed tickets |
| License       | MIT

The changes from #12404 also affect the `BirthdayType`.

Commits
-------

7faee60 *_timezone changes also affect the BirthdayType
@wouterj
Copy link
Member
wouterj commented Dec 13, 2014

How could this PR be merged? This is a huge BC break. Not only does it change the behaviour of the field type, it also makes applications fail in case these options were configured.

Also, the docs aren't updated yet and it didn't got enough priority from us (I've to admit that I missed it completely). I've had at least one on IRC with this problem, one on StackOverflow and many on GitHub.

Refs #12808 http://stackoverflow.com/questions/27463458/the-options-model-timezone-view-timezone-do-not-exist

@xabbuh
Copy link
Member
xabbuh commented Dec 14, 2014

#12911 probably fixes the issue, doesn't it?

@joshuataylor
Copy link

This has broken our production app, so docs are needed ASAP-ASAP for this.

@ghost
Copy link
ghost commented Dec 15, 2014

shouldn't you just rollback your app now?

@joshuataylor
Copy link

Haha, I did, which is another reason i <3 symfony2 :).

fabpot added a commit that referenced this pull request Dec 29, 2014
…guration (Soullivaneuh)

This PR was merged into the 2.6 branch.

Discussion
----------

Fix wrong DateTransformer timezone param for non-UTC configuration

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | N/A
| Fixed tickets | #12808
| License       | MIT
| Doc PR        | none

This PR is a fix of a little mistake on PR #12404.

Thanks @mvrhov for the clue! 👍

Pass 'UTC' param cause issue on single_text date type, `null` should be passed to get the default php timezone.

Don't know how to add test for #12808 issue, do someone know how to "change" properly timezone during tests? Where can I do this?

Thanks.

Commits
-------

0104f15 Fix wrong DateTransformer timezone param for non-UTC configuration. #12808
fabpot added a commit that referenced this pull request Dec 29, 2014
This PR was merged into the 2.6 branch.

Discussion
----------

[Form] Add further timezone tests for date type

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no (failing tests added)
| Fixed tickets | Adds tests for bug in #12808
| License       | MIT
| Doc PR        | ~

#12404 removed the timezone options from DateType and TimeType, however the tests don't cover all cases and there was a bug introduced as per #12808.

This PR adds tests for the expected behaviour but will not fix the bug.

Unfortunately, it seems that the tests are being skipped in the form component on Travis (and have been for a year) - probably due to the ICU version being changed.

Commits
-------

f919793 [Form] Add further timezone tests for date type
fabpot added a commit that referenced this pull request Jan 7, 2015
…ns (xabbuh)

This PR was merged into the 2.6 branch.

Discussion
----------

[Form] add back model_timezone and view_timezone options

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13224
| License       | MIT
| Doc PR        | TODO (deprecate the `model_timezone` and `view_timezone` options)

This reverts #12404.

Commits
-------

1c4a75a add back model_timezone and view_timezone options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0