Hmm what I don't understand with this PR is: Doesn't DATE_SUB do a subtraction while dateAdd does an addition, and there is no minus in your new code in file components/com_content/models/articles.php? Did you really test if it works the same way as before?

https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-add

I have tested this item ? unsuccessfully on a69e062

Tested with patch tester on MySQL.

Before patch I had to enter a positive number (900), after patch a negative number (-900) to have the same result.

For me this change of filter behavior would be ok as it is, but then 1. the testing instructions of this PR, and 2. the documentation for the relative date filtering would have to be changed to reflect this change of behavior of the filter.

The reason for the behavior change is line 464 of file components/com_content/models/articles.php, where it should be $query->dateAdd($nowDate, '-' . $relativeDate, 'DAY') because a DATE_SUB is replaced, which does a subtraction, and not a DATE_ADD, which does an addition.

Hint for other testers: Had to refresh cache because I use module caching and page caching.

The other thing is what happens if someone choses a negative value. My proposed solution '-' . $relativeDate might not work when $relativeDate is negative.
So maybe this PR does it in the right way.
But it would be a bc break and also would need change of documentation. So maybe do this for 4.0 because of bc break?

P.S.: I tested with MySQL.

there is no b/c imho , before this pr, it was simply wrong and not cross-database, even don't using the API
but yes true no one cares about different db's so closing

@alikon Sorry, I did not want to discourage you from a useful change. I just followed the testing instructions which said everything should work like before on MySQL.

no problem @richard67, please forgive me for this istinctive reaction....

maybe should i've to write everything should work as "expected" ?

A bit more work but really cool would be to add a function dateSub to the api, and let both dateAdd and dateSub accept negative values so dateAdd for a negative value would be a subtraction, and dateSub for a negative value would be an addition? Then the api functions would work the same like DATE_ADD and DATE_SUB, and then every appearance of DATE_ADD and DATE_SUB could be replaced by the corresponding api function without any change of behavior.

I would understand if you won't go this way because it would be more work.

To me it feels wrong to specify a negative value in the UI, because it does not make sense to search for future intervals, so for me is clear that "30" means "from now on until 30 days in past", so for me the current behavior (on MySQL) without this PR is ok.

If you want i can make a PR against your branch so you have less work with it.

add(1, -1) should be the same as add(1, 1) if we are talking with relative integer number
it's not a matter of a more work even if i'm quite much more lazy than others....
but i'm open .... so PR are warmly welcommed

@alikon Am just testing my changes, then will make PR against your branch.

@alikon I've just made a PR against the branch of this PR. I tested with success with MySQL that behavior is like before. Can you test for PostgreSQL?
And I have not added unit tests for the new function dateSub(). Can you do that? Or shall I try?

@alikon I also noticed that the @since tag in the new function's documentation needs to be changed, but I don't remember the constant text to be added, which will trhen be replaced with the Joomla! version when being released. Do you know that constant text for the @since tag?

I have not tested this item.

@richard67 That constant for @since tag is __DEPLOY_VERSION__

@richard67 correct quote makes no sense there

@HLeithner Sorry but I don't understand.

@alikon I've made a new PR against your branch, the other one I closed. The new one is simple and just contains the -1 * $relativeDate as proposed by @ggppdk .
I also have set up a test environment with postgresql and have tested your PR plus this change on both MySQL and PostgreSQL with success.
So if you just merge my new PR I can set the test result here as success.

@richard67 it was my fault. Everything is ok.

@richard67 merged thanks

I have tested this item ✅ successfully on 65f3169

Tested with MySQL and with PostgreSQL.

With mySQL it works like before, with PostgreSQL it does not work before and now works like with MySQL.

@alikon I made a PR against your repo to make @csthomas happy. Please check and if ok merge, then I can test again.

@richard67 merged thanks

I have tested this item ✅ successfully on 99190b1

Tested with success on both MySQL and PostgreSQL.
On MySQL the filter works as well as before (no idea why it worked before this PR because date parameter was quoted 2 times).
On PostgreSQL it work with this PR in the same way as with MySQL. Without this PR the filter does not work on PostgreSQL, i.e. no article links are shown.

I have tested this item ✅ successfully on 99190b1

Tested with success on both MySQL and PostgreSQL.
On MySQL the filter works as well as before (no idea why it worked before this PR because date parameter was quoted 2 times).
On PostgreSQL it work with this PR in the same way as with MySQL. Without this PR the filter does not work, i.e. no article links are shown.

It seems there are code style errors reported by Drone, maybe my fault:
FILE: ...ub.com/joomla/joomla-cms/libraries/joomla/database/query/postgresql.php
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
691 | ERROR | Expected 2 spaces after the longest type

no idea why it worked before this PR because date parameter was quoted 2 times

in the core before this PR there was no code using dateAdd() ?
but it will be used in the 3.9 for example in the privacy-framework or #20800

I have tested this item ✅ successfully on a09b53b

Tested with success on both MySQL and PostgreSQL.
On MySQL the filter works as well as before (no idea why it worked before this PR because date parameter was quoted 2 times).
On PostgreSQL it work with this PR in the same way as with MySQL. Without this PR the filter does not work on PostgreSQL, i.e. no article links are shown.

I have tested this item ✅ successfully on a09b53b

Tested with success on both MySQL and PostgreSQL.
On MySQL the filter works as well as before (no idea why it worked before this PR because date parameter was quoted 2 times).
On PostgreSQL it work with this PR in the same way as with MySQL. Without this PR the filter does not work on PostgreSQL, i.e. no article links are shown.

No idea why my test result appears 2 times in the issue tracker.

No idea why my test result appears 2 times.

I have tested this item ✅ successfully on 8436f31

Tested with success on both MySQL and PostgreSQL.
On MySQL the filter works as well as before (no idea why it worked before this PR because date parameter was quoted 2 times).
On PostgreSQL it work with this PR in the same way as with MySQL. Without this PR the filter does not work on PostgreSQL, i.e. no article links are shown.

I have tested this item ✅ successfully on 8436f31

RTC

RTC

This should be merged before 3.8.12 is released because it fixes a bug for PostgreSQL.

Looking at this, I think I'd rather have the dateAdd method quote date strings versus relying on the downstream caller to quote them (just because the current use in core is quoting them doesn't mean everyone's going to). If the expectation is that it should produce a string with quoted values, we should do the quoting here.

This should be merged before 3.8.12 is released because it fixes a bug for PostgreSQL.

Unfortunately that's not going to happen without delaying the release as we're already in the RC phase, personally I don't see this being urgent enough to do that.

Also, someone needs to ensure the appropriate changes are ported to https://github.com/joomla-framework/database

Looking at this, I think I'd rather have the dateAdd method quote date strings versus relying on the downstream caller to quote them ...

The problem may be to determine how to quote this parameter.

Which use case is valid?

• $query->dateAdd('publish_up', ...)
• $query->dateAdd('CURRENT_TIMESTAMP()', ...) - probably not an important case
• $query->dateAdd('2018-08-27 00:00:00', ...)

Exactly that. It may be a date or timestamp literal value with string quoting, or a column name with name quoting, or a function like "CURRENT_TIMESTAMP which should not be quoted at all.

Can't say I like it any, but if that's what it is then so be it. This kind of hits the same problem that joomla-framework/database#136 vaguely tries to mask/fix in that it still leaves too much of the decision on whether a value should be quoted to whomever is implementing the method. To me, a query building API should either be ensuring values get quoted consistently or not dealing with it at all (outside of explicitly named quoting methods) and I feel like when we start introducing value objects or leaving this type of implementation detail to the API consumer it's just setting us up for trouble down the line.

(Granted, these days I personally don't write code that makes liberal use of quoting like the Joomla API does (I leave it only to reserved keywords basically, as even parameterized values shouldn't be quoted), but to each their own I suppose)

The parameter in $query-> dateAdd() is problematic.

This is just my first idea, 3 use cases:

• $query->dateAdd(new Column('publish_up'), ...)
• $query->dateAdd(new Expression('CURRENT_TIMESTAMP()'), ...)
• $query->dateAdd('2018-08-27 00:00:00', ...) - if string then use auto quoted

or split $query->dateAdd() into separate methods.

I'd rather just leave it as it is right now instead of trying to add a bunch of complexity with a mixed argument. TBH there's a lot of the API that needs re-thinking, especially with forced parameterized query support at the interface level.

so before to port this change to https://github.com/joomla-framework/database

what's the final decision:
a) is up to the caller to quote the date like now
or
b) must be done inside the dateAdd() method

iirc no other database function in jdatabasequery escapes the $value on its own.

It's already complex to create a database query without magically escaping. IMHO we should focus on a the prepared statement driver.

$query->dateAdd(new Column('publish_up'), ...)
$query->dateAdd(new Expression('CURRENT_TIMESTAMP()'), ...)
$query->dateAdd('2018-08-27 00:00:00', ...)

Do we get any gain from the new? classes, adding $this->quoteName() results in the same output with less overhead.

If this is changed for dateAdd it has to be changed for all functions.

Whatever you will change (if so), please keep in mind that the parameter for calling dateAdd() is already quoted here, and this has not been changed by this PR:

And the quoted string variable $nowDate is then used at a few other places, not only for the call to dateAdd(). So there might be more changes necessary than just changing the call to dateAdd when the dateAdd function will be changed.

I think this is out of scope of this PR, and I think we should leave it as it is for now, as @mbabker suggested in his last comment, but that's just my personal opinion as a contributor, it is not my PR.

even for me is better option a), but to avoid to make more work than needed (ie BEFORE port this to the database framework) i've asked to the contributors....(don't forget i'm lazy ?)

Only my thoughts for J4.

use \Joomla\Database\Function\Date;
use \Joomla\Database\Expression;

/* Date::add(string|expression $date, int $interval, enum $datePart, array $params = []) */

// For prepared statement
Date::add(':date', -1, Date::DAY, ['date': '2018-01-01 00:00:00']);
Date::add('?', -1, Date::DAY, ['2018-01-01 00:00:00']);

// Call $db->quoteName() inside the method
Date::add('publish_up', -1, Date::DAY);

// Clearly ask for trouble if you do not know what you are doing
Date::add(new Expression('CURRENT_TIMESTAMP()'), -1, Date::DAY);

Just leave this as is before we start overengineering our API.

changes have been ported to joomla-framework/database#138

thanks all