Why is period '1 DAY' and not '1' DAY ? could we fix this?

See link in OP. That looks like correct syntax though, according to documentation https://www.postgresql.org/docs/9.0/functions-datetime.html.

timestamp '2001-09-28 23:00' - interval '23 hours'

Maybe different for prepared statements? I only read https://stackoverflow.com/questions/24499425/php-postgres-query-using-timestamp-interval

is it only the interval parameter? I have no pgsql at the moment to test this, sorry... I would like to avoid removing the prepared syntax if possible...

I would like to avoid removing the prepared syntax if possible...

This is a case where the use of a bound parameter is unnecessary. You are dealing with 100% server generated code, not something with user input. So while being able to use the prepared statement features consistently is a good thing, it is not necessary 100% of the time every single time.

With that said, if Joomla is really going down the path of "everything must be bound and manually $db->quote() anything is discouraged", try this:

$tagDate = $query->dateAdd($nowDate, '-1', strtoupper($timeframe));

$query->where($db->quoteName('tag_date') . ' > :tagDate');
$query->bind(':tagDate', $tagDate);

@mbabker that won't work. The entire result of dateAdd() should not be quoted. And it already adds quotes inside.

Well, short of removing quoting from dateAdd that's the only idea I had. And I wouldn't remove quoting there.

Hmm, why does the pgsql driver add a quote and the mysql driver doesn't, that sounds like a possible SQLi.

Also no other function except concatenate does quoteing without request. So this function shouldn't quote the interval so it has the same behavior like the other database drivers.

The entirety of the PostgreSQL code has been rather interesting to deal with in relation to anything MySQL. I stopped asking why things were inconsistent years ago. Clearly that code didn't go through much as much review when it was being written as it should have otherwise some of the inconsistencies would've been identified and addressed ages ago.

Considering the use case of dateAdd(), personally I would say the current behavior is right. PostgreSQL requires the interval segment to be quoted, which our API is doing, and I would say that being a query builder API it would be incorrect for the query object to return the statement without quoting the required segment. It'd be easier to identify it was purposeful by using $this->quote() instead of trying to manually do it, but ¯\_(ツ)_/¯.

i can remember the original discussion #21844 (comment)

That discussion had to do with quoting the $date argument. The issue here is the PostgreSQL query builder adds quotes to part of the string it generates (which, based on the docs, is required).

Take this string, which is the expected output from $query->dateAdd($query->quote('2019-10-13'), '1', 'DAY') with PostgreSQL:

"timestamp '2019-10-13' + interval '1 DAY'"

The "interval '1 DAY'" part gets quoted by the query builder because that's the PostgreSQL requirement. The query builder is not quoting the $date argument. The problem is the required quoting there is not compatible with the use of bound parameters.

This issue doesn't exist with other drivers because the resulting string is something akin to "DATE_ADD('2019-10-13', INTERVAL 1 DAY)" or "DATE_ADD(:testDate, INTERVAL 1 DAY)" if you used a parameterized date.

I tried to make a better replacement function for dateAdd but didn't came up with a better solution.

So to get this a bit further, would it be suitable to change the interval parameter to integer (in J5) and and force it to be an integer in j4?

Something like this?

	public function dateAdd($date, $interval, $datePart)
	{
                $interval = (int) $interval;
		if ($interval >= 0)
		{
			return 'timestamp ' . $date . " + interval '" . $interval . ' ' . $datePart . "'";
		}
		return 'timestamp ' . $date . " - interval '" . ($interval * -1) . ' ' . $datePart . "'";
	}

Or is there a reason why interval has to be a string? Could it be a column or an expression?

MySQL docs say it's expression and is evaluated as a string https://dev.mysql.com/doc/refman/5.5/en/date-and-time-functions.html#function_date-add.

Here a comparison of MySQL and PostgreSQL regarding date and time and timestamp calculations. PosgreSQL does not have functions date_add or date_sub.

As you can see, in MySQL the value part of the interval expression is only quoted like a string when it is a combination of values, like in INTERVAL '1:1' MINUTE_SECOND, INTERVAL '1 1:1:1' DAY_SECOND, INTERVAL '-1 10' DAY_HOUR or INTERVAL '1.999999' SECOND_MICROSECOND. In "normal" periods with integer value, the value is not quoted, see INTERVAL 1 DAY, but INTERVAL '1' DAY could work, too, as MySQL would cast the '1' to an integer, as far as I know its behavior.

PosgreSQL does not have functions date_add or date_sub. It uses arithmetic operators and date or time or timestamp or interval expressions like date '2001-09-28', time '01:00', timestamp '2001-09-29 03:00', interval '3 hours', and value expressions like double precision '3.5'.

So a mySQL INTERVAL '1.999999' SECOND_MICROSECOND would be a double precision '1.999999' * interval '1 second' in PostgreSQL. Ok, that seems to be easy. But then as soon as more than 1, the intervals change, so you have interval '1 second' and interval '2 seconds' ans so on. Ok, this alone is also not a problem. But as soon as we want to find an equivalent to MySQL INTERVAL '1 1:1:1' DAY_SECOND, where the name DAY_SECOND does not mean "days and seconds" but "from days down to seconds, i.e. days, hours, minutes and seconds", it starts to become funny.

So I think a db library function which supports all possible use cases of MySQL's DATE_ADD and DATE_SUB in PostgreSQL will be crazy spaghetti code or something with regexes which is hard to read.

What we currently have in the 2.0-dev branch of the db framework is:

We don't have a dateSub.

Regarding bind variables: In MySQL, INTERVAL :d DAY should use an integer variable, but maybe INTERVAL ':d' DAY would work, too, i.e. bind as string, and e.g. INTERVAL ':x' SECOND_MICROSECOND would also use a string. In PostgreSQL it could be e.g. timestamp ':t' - interval ':i'with :t = e.g. '2001-09-28 23:00' and :i = e.g. '1 hour' or '23 hours'.

No idea yet if we can do that for all which is possible with MySQL's DATE_ADD and DATE_SUB or if we have to limit it to a subset of that so we can use same functionality on PostgreSQL.

This all here as a kind of summary of the problem for everybody who is interested in DB stuff.

But I agree with @mbabker comment above: We use dateAdd with pre-defined values and not user-entered values. All use cases which I've found were either date time range filters in the list views, or the expiration date calculations for privacy requests or user activation. Nowhere was used any user input for the number of days to go in past. So no need for prepared statements/bind variables.

Maybe the easiest thing would be we do the date time calculation in PHP, add or substract so and so many days from current date in most cases, and then use the resulting datetime value as start or end date in our queries, and not use dateAdd anymore, and give up the idea to have a function mapping functionalities for datetime calculations of several database kinds into 1 common syntax, with support for bind variables.

So regarding to the analysis by richard shouldn't we reduce our implementation what we can support without getting crazy? The database framework supports more then mysql and pgsql.

My suggestions for changing the interval to be only an integer is still valid. Additional the $datePart parameter could be a list of constants.

@richard67 what happens in pgsql if you use '1 DAYS' or '20 DAY'?

My only concern is that we don't mix quoted and non quoted input values for different databases drivers. If prepared statements aren't possible then it's but then we should be consistent with the quoting. (In my opinion the database driver shouldn't quote anything)

@richard67 what happens in pgsql if you use '1 DAYS' or '20 DAY'?

SELECT CURRENT_TIMESTAMP + INTERVAL '1 DAYS'; => OK.
SELECT CURRENT_TIMESTAMP + INTERVAL '10 DAY'; => OK.
SELECT CURRENT_TIMESTAMP + INTERVAL '1 HOURS'; => OK.
SELECT CURRENT_TIMESTAMP + INTERVAL '10 HOUR'; => OK.

Not sure if we should rely on that.

My suggestions for changing the interval to be only an integer is still valid. Additional the $datePart parameter could be a list of constants.

That sounds reasonable to me.

I think we don't need to support MySQL expressions like INTERVAL '1 1:1:1' DAY_SECOND because these can be combined by several date_add calls using the "base units".

So what do we need? We need the base intervals YEAR, MONTH, DAY, HOUR, MINUTE, SECOND and maybe MICROSECOND. This is already supported by what we have now. To make it nice we could use plural when necessary on PostgreSQL, i.e. if integer value is not +1 or -1 we use e.g. DAYS and not DAY.

Quoting should be done in the right way by the query routines we have now, not by the calling functions, that's the only way we can support any kind of db, because in the calling function you don't know which kind of db you have, and as you can see the quoting is different for MySQL and PostgreSQL.

Then all it needs to finally make it safe is to either check the $datePart parameter to have only values supported by all kinds of db, i.e. those restricted base intervals listed above, or to make it be an enum (list of constants).

Now about bind:

If we use integers for the intervals' values we can use a trick in PostgreSQL: We can use e.g. ? + ? * INTERVAL '1 DAY', with ? being the bind variables' places, first one being the timestamp and second one the integer value of the interval. That means we use the integer as a multiplier for an interval with value 1. See discussion about this on stackoverflow.

On MySQL the dynamic SQL would be e.g. DATE_ADD(?, INTERVAL ? DAY), see discussion here on stackoverflow.

P.S.: Of course it should also be possible to bind the 1st parameter, the datetime/timestamp part.

Only with binding the type of the interval, DAY or MONTH and so on, that seems to be not really possible.

On PostgreSQL it could work this way: ? + (? || ' ' || ?)::INTERVAL, with 1st ? being the timestamp, 2nd ? being the integer value of the interval and 3rd ? being the type of the period as a string, i.e. the expression within the brackets ( and ) is type-casted to an interval. If using 1 parameter $datePart for the value and type of interval together, like it is now, then this would simply be ? + ?::INTERVAL, as long as we could make sure that the 2nd bind var is always something valid like n DAY, with n being an integer.

But on MySQL I have no idea how to bind the type of the interval.

Merging this as we don't seem to be able to come up with anything better