Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@xabbuh
Copy link
Member

@xabbuhxabbuh commentedMay 21, 2022•
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?no
Deprecations?yes
Tickets
LicenseMIT
Doc PR

@xabbuhxabbuh added this to the6.2 milestoneMay 21, 2022
@xabbuhxabbuh requested a review fromyceruto as acode ownerMay 21, 2022 14:59
@carsonbotcarsonbot 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 timezonesMay 21, 2022
@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch 2 times, most recently from97a0564 to95e119bCompareMay 22, 2022 11:14
@xabbuhxabbuh 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 timezonesMay 22, 2022
@xabbuh
Copy link
MemberAuthor

Status: Needs Review

@HeahDude
Copy link
Contributor

HeahDude commentedMay 22, 2022•
edited
Loading

What about otherDate* 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 otherDate* types as well?

@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch 3 times, most recently frombc464f2 to9baadc8CompareMay 27, 2022 06:33
@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch from9baadc8 tobf7222aCompareMay 27, 2022 06:52
@xabbuh
Copy link
MemberAuthor

Should we create a dedicated listener that can be used in otherDate* 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-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch 2 times, most recently from4a73df7 to3317b72CompareDecember 4, 2022 17:56
@fabpot
Copy link
Member

Is this one ready for another round of reviews?

@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch 2 times, most recently from949df05 toec03721CompareFebruary 4, 2023 10:32
@xabbuh
Copy link
MemberAuthor

Yes, the PR is ready to be reviewed again.

}

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']));

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
MemberAuthor

Choose a reason for hiding this comment

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

done

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

friendly ping@xabbuh :)

@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch 6 times, most recently from7b782eb to1c78baeCompareJune 16, 2023 20:11
@xabbuhxabbuhforce-pushed theform-time-type-model-timezone-mismatch branch from1c78bae tob78bfbcCompareJune 16, 2023 20:31
@nicolas-grekas
Copy link
Member

Thank you@xabbuh.

@nicolas-grekasnicolas-grekas merged commit261a8ed intosymfony:6.4Jun 28, 2023
@xabbuhxabbuh deleted the form-time-type-model-timezone-mismatch branchJune 28, 2023 19:59
This was referencedOct 21, 2023
@fmarchalemisys
Copy link
Contributor

Hi guys,

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

Having to normalize the timezone of a hundred ofDateTime before passing them to nearly as manyFormType 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
MemberAuthor

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

fmarchalemisys reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@ycerutoycerutoyceruto approved these changes

+1 more reviewer

@HeahDudeHeahDudeHeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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
[8]ページ先頭
©2009-2025 Movatter.jp