Add format string for DateTime with timezone on SQLite

[Roman3349]

Q	A
Type	improvement
Fixed issues	#5929

Summary

Saves the timezone information for DateTimeTz type on SQLite3.

Information from the SQLite3 documentation

Formats 2 through 10 may be optionally followed by a timezone indicator of the form "[+-]HH:MM" or just "Z". The date and time functions use UTC or "zulu" time internally, and so the "Z" suffix is a no-op. Any non-zero "HH:MM" suffix is subtracted from the indicated date and time in order to compute zulu time. For example, all of the following time-values are equivalent:

2013-10-07 08:23:19.120
2013-10-07T08:23:19.120Z
2013-10-07 04:23:19.120-04:00

[morozov]

@Roman3349, thank you for the patch. Please add tests. See Testing Guidelines for reference.

[Roman3349]

@morozov I've added the tests. I'm sorry it took me so long - I did it right after I received your message, but then I had to deal with other things and forgot to commit the changes.

[derrabus]

Your test verifies that PHP parses a given date string correctly. It's good to know that PHP's date extension behaves as expected, but that's not the point of our test suite.

We need a test that actually hits SQLite and makes sure that after your change, SQLite properly stores a date with timezone information and that the timezone is properly restored if we load a DateTimeImmutable object from the database.

[Roman3349]

@derrabus For now, the Oracle tests will fail due to #6469/#2332. I should fix this at the platform level https://github.com/doctrine/dbal/blob/4.4.x/src/Platforms/OraclePlatform.php#L760-L763, where I would specify the format string Y-m-d H:i:s P? Or at the driver level https://github.com/doctrine/dbal/blob/4.4.x/src/Driver/OCI8/Middleware/InitializeSession.php#L32, where I would specify the format string YYYY-MM-DD HH24:MI:SSTZH:TZM?

[derrabus]

@derrabus For now, the Oracle tests will fail due to #6469/#2332.

Skip the test on Oracle then. Your goal is to fix the issue on SQLite, so let's focus on that first. 🙂

If you really want to fix the Oracle issue as well (which would be great!), let's do that in a separate PR.

[derrabus]

Thank you for the test. I've done some research because I found it hard to believe that we haven't covered this type already in our testsuite.

There is actually Doctrine\DBAL\Tests\Functional\TypeConversionTest::testIdempotentConversionToDateTime(). This test has a strange if block that basically skips most of the test for Types::DATETIMETZ_MUTABLE. I think, we should either fix that test or remove the Types::DATETIMETZ_MUTABLE case from it entirely. Also, Types::DATETIMETZ_IMMUTABLE apparently is not covered at all. We should probably fix that, too.

[derrabus]

Suggested change

	/** @return DateTimeInterface[][] */
	/** @return list<array{DateTime}> */

[derrabus]

Suggested change

	public function testInsertAndRetrieveDateTimeTz(DateTimeInterface $expected): void
	public function testInsertAndRetrieveDateTimeTz(DateTime $expected): void

[derrabus]

Suggested change

	$value = Type::getType(Types::DATETIMETZ_MUTABLE)->convertToPHPValue(
	$this->connection->fetchOne('SELECT val FROM datetimetz_table'),
	$platform,
	);
	$value = $this->connection->convertToPHPValue(
	$this->connection->fetchOne('SELECT val FROM datetimetz_table'),
	Types::DATETIMETZ_MUTABLE,
	);

[derrabus]

Given that the timezone is actually relevant here, we should not rely on PHP's default timezone. Let's add an explicit timezone here.

[derrabus]

One more thing we have to think about. We're about to change the behavior of two types for SQLite. The old behavior might not have been the intended one, but still there might be apps out there that have configured that type on SQLite. What would happen if those apps upgraded to the new DBAL version with the new behavior? Would they still be able to read databases that were created with the old behavior?

[Roman3349]

@derrabus Thank you for your review, your suggestions should now be addressed. For the existing stored values, I've added a fallback for the SQLite platform to parse values from the database, I couldn't come up with a better solution than do it in the type. The branch is now rebased to latest commit on 4.5.x.

[Roman3349]

I'm just not sure whether to leave the tests in their respective classes or put them in tests/Functional/TypeConversionTest.php, because there's a lot of duplicate code there.

[derrabus]

Never do instanceof checks on the platform. This code belongs into the platform class.

[Roman3349]

So should I add a public constant or getter to AbstractPlatform to indicate that datetimetz can store a datetime? And this constant/getter will be set to true for SQLitePlatform.
Because otherwise, I can’t think of how to do this in the platform code, since the only relevant thing in the platform code is the format used. Everything else related to converting values from the DB to PHP happens in the class of the given type and in Doctrine\DBAL\Connection.

[derrabus]

The platforms are a collection of template methods that we call whenever we need a behavior to be different between database platforms.

So, when you find yourself doing something like this…

if ($platform instanceof FooPlatform) {
    // behavior for the foo database
} else {
    // default behavior
}

… then you should always be able to refactor this block into a method on AbstractPlatform which is overridden on FooPlatform, so the whole if block collapses into…

$platform->someBehavior();

Does that make sense?