:bug: bugfix/#6261 - Allow (also for DATETIME) dynamic intervals in DATE_ADD & DATE_SUB for SQLite

[DaedalusDev]

• :bug: Fix code divergence
• :white_check_mark: Add related tests

Q	A
Type	bug
Fixed issues	#6261

Summary

Code inspired from a sibling Pull Request

Question

1. Documentation currently don't match :

<?php
/**
* [...]
* @param int|numeric-string $interval The interval [...]
*/

May i update documentation ?

2. Note that the existing code don't work and will produce a warning with WEEK and QUARTER with a $interval as non-numeric value. Probably an unknown issue, so maybe the code would throw an error for these specific cases. https://github.com/doctrine/dbal/blob/6a793fb72948c9dec844de57de8cdbc16ff75bec/src/Platforms/SqlitePlatform.php#L160C3-L160C3

TypeError : Unsupported operand types: string * int

In doctrine 4.x.x, getDateArithmeticIntervalExpression is redesigned and :

• use DATETIME() in all cases;
• use the dedicated getConcatExpression and quoteStringLiteral ;

For WEEK and QUARTER, a method called multiplyInterval is called and may prevent this issue.

[derrabus]

Please add a functional test that covers your change. Are you sure that this issue applies to SQLite only?

[DaedalusDev]

Please add a functional test that covers your change. Are you sure that this issue applies to SQLite only?

I added a related functional test. I haven't test if the issue is present in other platforms but my fix apply only for SQLite because date/datetime management is quiet different than other SQL languages. Check https://www.sqlite.org/lang_datefunc.html for more details.

[derrabus]

I haven't test if the issue is present in other platforms

Do you have time to do that? It would be wierd to fix this problem for SQLite only. If we remove the SQL assertion from your functional test, it more or less looks like a test that should pass on any platform.

[DaedalusDev]

I haven't test if the issue is present in other platforms

Do you have time to do that? It would be wierd to fix this problem for SQLite only. If we remove the SQL assertion from your functional test, it more or less looks like a test that should pass on any platform.

Here, my pure code analyse, i didn't test functionaly all platforms : getDateArithmeticIntervalExpression is currently overrides by 6 of 17 supported platforms.

:white_check_mark: AbstractMySQLPlatform (including all Mysql/Mariadb Platforms)

Use the dedicated DATE_ADD and DATE_SUB function. The produced result implement INTERVAL and will natively support interpolation. So this platform work as expected and don't require an adjustment. Ex : getDateArithmeticIntervalExpression('NOW()', '+', 's.utc_offset', 'MINUTE') will produce DATE_ADD(NOW(), INTERVAL + s.utc_offset MINUTE).

:warning: DB2

The code will produce the same TypeError : Unsupported operand types: string * int for WEEK and QUARTER operator with a string interval. The produced result natively support interpolation so, i don't think an adjustment is required. Ex: getDateArithmeticIntervalExpression('current', '+', 's.utc_offset', 'MINUTE') will produce current + s.utc_offset MINUTE.

:x: Oracle

The code will produce the same TypeError : Unsupported operand types: string * int for QUARTER and YEAR operator with a string interval. The produced result will change for MONTH, QUARTER, YEAR because it use a dedicated function ADD_MONTHS. Unfortunately, according to documentation, this function only support integer value so maybe and interpolation can fix this issue but i suggest another thinks : Oracle is supposed to support INTERVAL keyword so i think it is possible to refacto the diverging code branches. I currently don't use oracle as database so i don't have feedback about performances considerations or any information about version support.

:white_check_mark: PostgresSQL

The code will produce the same TypeError : Unsupported operand types: string * int for QUARTER operator with a string interval. The code use a dedicated syntax. The produced result implement ::interval is write with an interpolation. So this platform work as expected and don't require an adjustment. Ex : getDateArithmeticIntervalExpression('NOW()', '+', 's.utc_offset', 'MINUTE') will produce (NOW() + (s.utc_offset || 'MINUTE')::interval) which mean a valid syntax.

:white_check_mark: SQLite

⏫ ⏫ :-)

:white_check_mark: SQLServer

Use the dedicated DATEADD function. Ex : getDateArithmeticIntervalExpression('@Date', '+', 's.utc_offset', 'MINUTE') will produce DATEADD(MINUTE, s.utc_offset, @Date) which mean a valid syntax.

We can create additionnal functionnal tests to ensure all plaform work as expected.

[github-actions[bot]]

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. If you want to continue working on it, please leave a comment.

[github-actions[bot]]

This pull request was closed due to inactivity.