8000 [Reference][Forms] deprecate model_timezone and view_timezone options by xabbuh · Pull Request #4763 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

[Reference][Forms] deprecate model_timezone and view_timezone options #4763

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
Closed
wants to merge 1 commit into from
Closed

[Reference][Forms] deprecate model_timezone and view_timezone options #4763

wants to merge 1 commit into from

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Jan 4, 2015
Q A
Doc fix? yes
New docs? no
Applies to 2.6+
Fixed tickets #4417

@xabbuh xabbuh added the On hold label Jan 4, 2015
@xabbuh
Copy link
Member Author
xabbuh commented Jan 4, 2015

This replaces #4417 in case that symfony/symfony#13241 gets merged.

@xabbuh
Copy link
Member Author
xabbuh commented Jan 4, 2015

According to @weaverryan's comment we should probably consider merging this into the 2.3 branch.

@jakzal jakzal mentioned this pull request Jan 5, 2015
Closed
@xabbuh
Copy link
Member Author
xabbuh commented Jan 11, 2015

symfony/symfony#13241 was merged. So this is ready to be reviewed/merged.

8000
@xabbuh xabbuh mentioned this pull request Jan 11, 2015
Merged
@xabbuh
Copy link
Member Author
xabbuh commented Jan 11, 2015

@weaverryan While I would agree with you on #4417 (comment), I think that we actually cannot do this given that symfony/symfony#12911 was only merged into Symfony 2.6. Am I right that before Symfony 2.6, you would still have to use the model_timezone and view_timezone options to avoid issues like symfony/symfony#12808? Should it otherwise be considered to backport symfony/symfony#12911 to Symfony 2.3?

@weaverryan
Copy link
Member

@xabbuh :) I'm not sure about your questions. My impression was that before 2.6, life was "good", except that if you think about it hard enough, these timezone options didn't make sense, and so they were removed. I think the issue in symfony/symfony#12808 was only introduced when these options were removed, so there was no bug before this.

That's a long way to say:

  1. I'm not clear what the long-term vision is for these options. It actually doesn't look like they're deprecated in core still. And I'm still a bit confused as to if they do anything and can be removed in 3.0. I believe they should be deprecated, and we kept them only now because some people started accidentally using them, and they do have side-effects.

  2. Once we know (1), we'll know better if these options are truly deprecated or not. For now, since there is no deprecation in the core, the documentation is accurate.

Thanks!

@xabbuh
Copy link
Member Author
xabbuh commented Mar 8, 2015

@fabpot @webmozart @stof What do you think about this? Does it make sense to you to backport the changes to Symfony 2.3 or should we just deprecate the options starting with Symfony 2.6?

@weaverryan
Copy link
Member

@xabbuh I'm just re-reviewing this again, and think I'm more clear now, but I need your help:

  1. Prior to 2.6, the TimeType, DateType and BirthdayType had these 2 options. But since these are time-only or date-only fields, having a timezone didn't make sense: if you want "4PM" to be a form field, just use a DateTime object set to 4PM - the form shouldn't do any transformations (see [Form] DST issues in TimeType symfony#7187).

  2. For 2.6, these options were removed from these 3 types, and (after Fix wrong DateTransformer timezone param for non-UTC configuration symfony#12911). Yay! No more "needless" timezone transformation on these fields.

  3. But people were still using these options and getting errors. So, you put them back at [Form] add back model_timezone and view_timezone options symfony#13241. BUT, you didn't really put these back. If you look at the diff, you added the options back, but they're not actually being used. So, you suppressed the "option does not exist" error people got, but there is still a behavior change from 2.5->2.6 - the options are now being ignored. The only thing that's confusing me still is this test: https://github.com/symfony/symfony/pull/13241/files#diff-fbbd0dcfdc8319ceb89c18435e590fd2R286. As I understand things, this should fail - it should STILL return 12, 4 and 5 (as before). And in fact, this test IS failing for me locally. And I think it's probably failing in Symfony core too, except that it's being skipped (see the large S section for forms - due to IntlTestHelper::requireIntl).

@xabbuh - does this sound accurate? If so, we should:

A) Deprecate these options, as they don't do anything anymore anyways
B) Fix the above tests (and fix all the skips).

Thanks!

@xabbuh
Copy link
Member Author
xabbuh commented Apr 23, 2015

@weaverryan Sorry for coming back to you lately. As far as I can see, this issue should have been fixed now with symfony/symfony#14256. Do you agree?

@xabbuh
Copy link
Member Author
xabbuh commented May 17, 2015

ping @weaverryan

@weaverryan
Copy link
Member

Thanks for the ping @xabbuh!

After looking at symfony/symfony#14256, it looks like these timezone options have been fully added back and are in fact not deprecated. That would mean that the docs need no changes as we can close this.

If you agree, go ahead and close the PR. What a whirlwind :)

@xabbuh
Copy link
Member Author
xabbuh commented Jun 19, 2015

I fully agree with you Ryan. :)

@xabbuh xabbuh closed this Jun 19, 2015
@xabbuh xabbuh deleted the symfony-13241 branch June 19, 2015 16:00
@weaverryan
Copy link
Member

awesome!

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
Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xabbuh @weaverryan
0