-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Deprecate passing timezone information to methods where it is ignored #6020
Conversation
@greg0ire, do you think something like this can prevent those foot shots? |
84e812e
to
0cdd7a4
Compare
There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
@phansys sorry for ignoring you, I must have get swamped. Yes, I think that would help. |
694728b
to
f4767aa
Compare
Don't worry! no problem. we all have our times and own occupations. |
bcd0866
to
5260030
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests. doctrine/deprecations
comes with helpers for catching deprecations.
src/Types/DateImmutableType.php
Outdated
if ($offset !== $defaultOffset) { | ||
Deprecation::triggerIfCalledFromOutside( | ||
'doctrine/dbal', | ||
'https://github.com/doctrine/dbal/pull/xxxx', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'https://github.com/doctrine/dbal/pull/xxxx', | |
'https://github.com/doctrine/dbal/pull/6020', |
src/Types/DateImmutableType.php
Outdated
if ($offset !== $defaultOffset) { | ||
Deprecation::triggerIfCalledFromOutside( | ||
'doctrine/dbal', | ||
'https://github.com/doctrine/dbal/pull/xxxx', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'https://github.com/doctrine/dbal/pull/xxxx', | |
'https://github.com/doctrine/dbal/pull/6020', |
ed2abda
to
b10717c
Compare
Ah I need to merge up. Hang on. |
That worked. Please address the remaining CS issues. |
@@ -117,4 +130,96 @@ public function testRequiresSQLCommentHint(): void | |||
{ | |||
self::assertTrue($this->type->requiresSQLCommentHint($this->platform)); | |||
} | |||
|
|||
/** @dataProvider provideDateTimeInstancesWithTimezone */ | |||
public function testTimezoneDeprecationFromConvertsToDatabaseValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function testTimezoneDeprecationFromConvertsToDatabaseValue( | |
public function testTimezoneDeprecationFromConvertToDatabaseValue( |
$defaultOffset = (new DateTimeImmutable())->format('O'); | ||
|
||
if ($offset !== $defaultOffset) { | ||
Deprecation::triggerIfCalledFromOutside( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a realistic situation where this deprecation might be triggered? In what case could the DBAL provide timezone information to this method? If it is lost when saving to the database, this seems unlikely, but maybe there are other situations I did not think of? If the timezone of the database server and the application server differ, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, I must review the flow in order to answer.
There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
This pull request was closed due to inactivity. |
Summary
The offset comparison is made against the default timezone instead of UTC because the user may be using a specific default timezone on purpose.
Related to #6014 and #6017 (comment).