Skip to content
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

Deprecate passing timezone information to methods where it is ignored #6020

Closed
wants to merge 1 commit into from
Closed

Deprecate passing timezone information to methods where it is ignored #6020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/Types/DateImmutableType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,22 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform)
}

if ($value instanceof DateTimeImmutable) {
$offset = $value->format('O');
$defaultOffset = (new DateTimeImmutable())->format('O');

if ($offset !== $defaultOffset) {
Deprecation::triggerIfCalledFromOutside(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6020',
'Passing a timezone offset (%s) different than the default one (%s) is deprecated'
. ' as it will be lost, use %s::%s() instead.',
$offset,
$defaultOffset,
DateTimeTzImmutableType::class,
__FUNCTION__,
);
}

return $value->format($platform->getDateFormatString());
}

Expand All @@ -56,7 +72,27 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform)
*/
public function convertToPHPValue($value, AbstractPlatform $platform)
{
if ($value === null || $value instanceof DateTimeImmutable) {
if ($value === null) {
return null;
}

if ($value instanceof DateTimeImmutable) {
$offset = $value->format('O');
$defaultOffset = (new DateTimeImmutable())->format('O');

if ($offset !== $defaultOffset) {
Deprecation::triggerIfCalledFromOutside(
Copy link
Member

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?

Copy link
Contributor Author

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.

'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6020',
'Passing a timezone offset (%s) different than the default one (%s) is deprecated'
. ' as it may be lost, use %s::%s() instead.',
$offset,
$defaultOffset,
DateTimeTzImmutableType::class,
__FUNCTION__,
);
}

return $value;
}

Expand Down
111 changes: 108 additions & 3 deletions tests/Types/DateImmutableTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,22 @@

use DateTime;
use DateTimeImmutable;
use DateTimeZone;
use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\ConversionException;
use Doctrine\DBAL\Types\DateImmutableType;
use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
use InvalidArgumentException;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

use function get_class;

class DateImmutableTypeTest extends TestCase
{
use VerifyDeprecations;

/** @var AbstractPlatform&MockObject */
private AbstractPlatform $platform;

Expand Down Expand Up @@ -48,10 +53,18 @@ public function testConvertsDateTimeImmutableInstanceToDatabaseValue(): void
$this->platform->expects(self::once())
->method('getDateFormatString')
->willReturn('Y-m-d');
$date->expects(self::once())
$date->expects(self::exactly(2))
->method('format')
->with('Y-m-d')
->willReturn('2016-01-01');
->willReturnCallback(static function (string $format): string {
switch ($format) {
case 'O':
return 'UTC';
case 'Y-m-d':
return '2016-01-01';
default:
throw new InvalidArgumentException();
}
});

self::assertSame(
'2016-01-01',
Expand Down Expand Up @@ -117,4 +130,96 @@ public function testRequiresSQLCommentHint(): void
{
self::assertTrue($this->type->requiresSQLCommentHint($this->platform));
}

/** @dataProvider provideDateTimeInstancesWithTimezone */
public function testTimezoneDeprecationFromConvertsToDatabaseValue(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function testTimezoneDeprecationFromConvertsToDatabaseValue(
public function testTimezoneDeprecationFromConvertToDatabaseValue(

string $defaultTimeZone,
DateTimeImmutable $date
): void {
$this->iniSet('date.timezone', $defaultTimeZone);

$defaultOffset = (new DateTimeImmutable())->format('O');

self::assertFalse($defaultOffset === $date->format('O'));

$this->platform->expects(self::once())
->method('getDateFormatString')
->willReturn('Y-m-d');

$this->expectDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/6020');

$this->type->convertToDatabaseValue($date, $this->platform);
}

/** @dataProvider provideDateTimeInstancesWithTimezone */
public function testTimezoneDeprecationFromConvertToPHPValue(string $defaultTimeZone, DateTimeImmutable $date): void
{
$this->iniSet('date.timezone', $defaultTimeZone);

$defaultOffset = (new DateTimeImmutable())->format('O');

self::assertFalse($defaultOffset === $date->format('O'));

$this->expectDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/6020');

$this->type->convertToPHPValue($date, $this->platform);
}

/** @psalm-return iterable<string, array{0: string, 1: DateTimeImmutable}> */
public function provideDateTimeInstancesWithTimezone(): iterable
{
yield 'timezone_utc' => [
'UTC',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('America/Argentina/Buenos_Aires')),
];

yield 'timezone_amsterdam' => [
'Europe/Amsterdam',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('America/Argentina/Buenos_Aires')),
];

yield 'offset_utc' => [
'UTC',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('-0300')),
];
}

/** @dataProvider provideDateTimeInstancesWithNoTimezoneDiff */
public function testNoTimezoneInValueConversion(string $defaultTimeZone, DateTimeImmutable $date): void
{
$this->iniSet('date.timezone', $defaultTimeZone);

$this->platform->expects(self::once())
->method('getDateFormatString')
->willReturn('Y-m-d');

$this->expectNoDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/6020');

$this->type->convertToDatabaseValue($date, $this->platform);
$this->type->convertToPHPValue($date, $this->platform);
}

/** @psalm-return iterable<string, array{0: string, 1: DateTimeImmutable}> */
public function provideDateTimeInstancesWithNoTimezoneDiff(): iterable
{
yield 'timezone_utc' => [
'UTC',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('UTC')),
];

yield 'timezone_buenos_aires' => [
'America/Argentina/Buenos_Aires',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('America/Argentina/Buenos_Aires')),
];

yield 'same_offset_with_different_timezones' => [
'America/Sao_Paulo',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('America/Argentina/Buenos_Aires')),
];

yield 'offset_-0300' => [
'America/Argentina/Buenos_Aires',
(new DateTimeImmutable())->setTimezone(new DateTimeZone('-0300')),
];
}
}