8000 [Form] deprecate using the date and time types with date objects with not-matching timezones by xabbuh · Pull Request #46426 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] deprecate using the date and time types with date objects with not-matching timezones #46426

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
Jun 28, 2023
Merged

[Form] deprecate using the date and time types with date objects with not-matching timezones #46426

merged 1 commit into from
Jun 28, 2023

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented May 21, 2022 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? no
Deprecations? yes
Tickets
License MIT
Doc PR

@xabbuh xabbuh added this to the 6.2 milestone May 21, 2022
@xabbuh xabbuh requested a review from yceruto as a code owner May 21, 2022 14:59
@carsonbot carsonbot changed the title [WIP] [Form] deprecate using the TimeType with date objects with not-matching timezones [Form] [WIP]  deprecate using the TimeType with date objects with not-matching timezones May 21, 2022
@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch 2 times, most recently from 97a0564 to 95e119b Compare May 22, 2022 11:14
@xabbuh xabbuh changed the title [Form] [WIP]  deprecate using the TimeType with date objects with not-matching timezones [Form] deprecate using the TimeType with date objects with not-matching timezones May 22, 2022
@xabbuh
Copy link
Member Author
xabbuh commented May 22, 2022

Status: Needs Review

@HeahDude
Copy link
Contributor
HeahDude commented May 22, 2022 •
edited
Loading

What about other Date* types? Shouldn't this be done at the transformer level so such consistency can be checked in all of them?

EDIT: no model transformation to hook into for such cases

@HeahDude
Copy link
Contributor

Should we create a dedicated listener that can be used in other Date* types as well?

@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch 3 times, most recently from bc464f2 to 9baadc8 Compare May 27, 2022 06:33
@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch from 9baadc8 to bf7222a Compare May 27, 2022 06:52
@xabbuh
Copy link
Member Author
xabbuh commented May 27, 2022

Should we create a dedicated listener that can be used in other Date* types as well?

I don't think we need to extract this into a dedicated listener, but you are right that we should trigger the deprecation in these types as well. I have updated the code accordingly.

Status: Needs Review

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch 2 times, most recently from 4a73df7 to 3317b72 Compare December 4, 2022 17:56
@fabpot
Copy link
Member
fabpot commented Feb 4, 2023

Is this one ready for another round of reviews?

@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch 2 times, most recently from 949df05 to ec03721 Compare February 4, 2023 10:32
@xabbuh
Copy link
Member Author
xabbuh commented Feb 4, 2023

Yes, the PR is ready to be reviewed again.

nicolas-grekas
nicolas-grekas reviewed Feb 24, 2023
View reviewed changes
src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php Outdated
}

if ($date->getTimezone()->getName() !== $options['model_timezone']) {
trigger_deprecation('symfony/form', '6.3', sprintf('Using a "%s" instance with a timezone ("%s") not matching the configured model timezone "%s" is deprecated.', $date::class, $date->getTimezone()->getName(), $options['model_timezone']));
Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing some inline comment to know what to do with this in 7.0.
Are we going to throw something? If yes, can we add the anticipated line as a comment already?
Same for tests: how should they be changed in 7.0? Let's add some commented expectException?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas
Copy link
Member

friendly ping @xabbuh :)

@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch 6 times, most recently from 7b782eb to 1c78bae Compare June 16, 2023 20:11
@xabbuh xabbuh force-pushed the form-time-type-model-timezone-mismatch branch from 1c78bae to b78bfbc Compare June 16, 2023 20:31
@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 261a8ed into symfony:6.4 Jun 28, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 28, 2023
Closed
@xabbuh xabbuh deleted the form-time-type-model-timezone-mismatch branch June 28, 2023 19:59
This was referenced Oct 21, 2023
Merged
Merged
@fmarchalemisys
Copy link
Contributor

Hi guys,

This change is breaking our app because DateTime comes from various sources (php, doctrine, external modules). The timezone can either be utc or the server timezone depending on what created the php DateTime.

Having to normalize the timezone of a hundred of DateTime before passing them to nearly as many FormType would requires a substantial effort.

Can you elaborate on the issues that prompted this PR? In short, is throwing an exception to the face of the user the right approach?

@xabbuh
Copy link
Member Author
xabbuh commented Oct 28, 2024

@fmarchalemisys Please open a new issue. Comments on merged PRs are likely to get lost.

@fmarchalemisys fmarchalemisys mentioned this pull request Oct 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeahDude HeahDude HeahDude left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@yceruto yceruto yceruto approved these changes

Assignees
No one assigned
Labels
Deprecation Form Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
8 participants
@xabbuh @HeahDude @nicolas-grekas @stof @fabpot @fmarchalemisys @yceruto @carsonbot
0