Fix SQLite date arithmetic for dynamic intervals

[Russ-ctl]

Q	A
Type	improvement
BC Break	no
Fixed issues	#4426, #4462

Summary

Changes SqlitePlatform::getDateArithmeticIntervalExpression to make it consistently support dynamic intervals for any supported DateIntervalUnit. Also adds a test similar to DataAccessTest::testDateArithmetics that tests all of the getDate(Add|Sub)(Unit)Expression with a named parameter instead of a static integer.

To support weeks and quarters with dynamic intervals, I had to put the multiplication into the generated SQL. If this feels like maybe too much, I could instead have those cases throw if the value !is_numeric; that's still BC (since it doesn't work today) while providing a much clearer error message. (I only need seconds to work, for my project!)

First time contributing to a Doctrine project, so let me know if I've based the PR on the right branch!

[Russ-ctl]

phpcs issues should be fixed.

PHPStan/Psalm are complaining that is_numeric($interval) is redundant since the type annotation for AbstractPlatform::getDateArithmeticIntervalExpression is int. I'm not very familiar with dbal internals, but given #3017 it seems like the type annotation should possibly change, since at least some platforms do support expressions for $interval, rather than only integers.

Looking through the test failures --

• MySQL, Maria, SQLite all pass
• Postgres, Oracle both stop at quarters
  • It looks like the respective platforms do math in PHP for these cases (Oracle, Postgres)
  • Curiously, the Oracle implementation appears to do math in PHP for quarters or years and math in SQL for most other intervals
• MS SQL has lots of errors, a bit opaque for me; I can dig in more if someone can provide a bit of overall direction on how I should proceed with this PR
• DB2 stops at weeks
  • Similar to Oracle and Postgres, it does a multiply in PHP for weeks and quarters, specifically

I'd like to see dynamic intervals supported going forward, ideally across all backends (and I believe that is reasonably straightforward; most backends seem like they could be reworked to move the multiplication out of PHP and into SQL).

I'm also open to scoping the tests to just SQLite/MySQL/MariaDB, since those are the platforms I use and which work (with this PR, at least). I'm not sure what to do about the type annotation in this case, though.

[morozov]

@Russ-ctl, thanks for the contribution. As far as I can tell, this is not a bug, and the Platform API up until ~4.0.0 expects the interval to be a number rather than an SQL expression. For instance,

dbal/src/Platforms/AbstractPlatform.php

Lines 1263 to 1273 in e88ce6d

	/**
	* Returns the SQL to add the number of given seconds to a date.
	*
	* @param string $date
	* @param int $seconds
	*
	* @return string
	*
	* @throws Exception If not supported on this platform.
	*/
	public function getDateAddSecondsExpression($date, $seconds)

This issue may have been partially or fully resolved in 4.0.x (#3498):

As a side effect of the above, with a slight rework in the implementation, the $interval argument of the getDate(Add|Sub)*Expression() methods can now accept an SQL expression which makes it possible to define the interval as a numeric literal (former behavior), prepared statement parameter, column name or technically any valid SQL expression 🔥

It is fine if you want to get it addressed in a version sooner than 4.0.0 but the fix should be consistent with what we already have. Additionally, I don't believe doing it in 2.x is a good idea. We might do it in 3.3.x instead.

I have a few comments about the tests and the implementation but we need to address the above question first.

[Russ-ctl]

As far as I can tell, this is not a bug, and the Platform API up until ~4.0.0 expects the interval to be a number rather than an SQL expression.

Yes, this was confusing to me, since #3019 was accepted despite that API contract; I figured since that was accepted a fix for the other intervals might also be :)

This issue may have been partially or fully resolved in 4.0.x (#3498):

🎉

Aha, I had not checked the 4.x branch to see if the issue was fixed there, just checked 2.x and 3.x. Skimming the 4.x branch now, it looks like the approach there is much more robust, as are the tests, which I'm glad to see.

Unfortunately, my team is still on PHP 7 and we're still using doctrine/dbal 2.10 at the moment, so it will likely be a while before we are able to get up to 4.0.

How much work do you expect would be involved in putting a fix together for 3.x, given the API contract expects the interval to be an int?

[morozov]

Unfortunately, my team is still on PHP 7 and we're still using doctrine/dbal 2.10 at the moment, so it will likely be a while before we are able to get up to 4.0.

There's no plan to release 4.0 yet, so you'll have to upgrade at least to 3.2.x. What exactly PHP 7 are you on? DBAL 3 requires PHP 7.3 which is quite permissive.

How much work do you expect would be involved in putting a fix together for 3.x, given the API contract expects the interval to be an int?

No idea. Code-wise, it should be straightforward but guarding against potential BC breaks may require extra caution.

[Russ-ctl]

There's no plan to release 4.0 yet, so you'll have to upgrade at least to 3.2.x. What exactly PHP 7 are you on? DBAL 3 requires PHP 7.3 which is quite permissive.

We're still on 7.2 unfortunately.

No idea. Code-wise, it should be straightforward but guarding against potential BC breaks may require extra caution.

Alright, so it sounds like it's not completely trivial. We're using a workaround in our own codebase, so we're not blocked for now, and it seems like it's probably fixed already in the 4.x branch. So, I'm good with closing this if you are.

[morozov]

So, I'm good with closing this if you are.

I am. Closing it.