Use dynamic query instead of SQL Query for the count

Charchil Khandelwal → made their first commit to this issue’s fork.

Created MR!7 for the issue mentioned, please review.
Thank you.

@LRWebks could you please review this and try if both queries have the exactly same result?

Removed the deprecated command and replaced it with an injected Drupal service call. To review it is!

Someone else has to resolve the thread though, as I'm not maintainer

The benefit of the used DI is that it allows easier testing, for example injecting a different time. So I think that would be the perfect, but also most complex solution. I really like it, but it adds complexity.

The more simple one might be:

$query->condition('w.timestamp', strtotime('-1 hour'), '>');

which uses global system time. So I think the taken approach is even better.

I also looked if there's another way using the prior SQL:
BETWEEN UNIX_TIMESTAMP(DATE_ADD(NOW(), INTERVAL -1 HOUR)) AND UNIX_TIMESTAMP() as parameter for ->condition() to keep the code, but that doesn't seem to be allowed.

So my vote is RTBC for this, even if it looks complex, but for testing it makes sense to also inject time!

Documentation: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Date...

I'd like to have this signed-off by @nevergone.

The current solution is definitely wrong. UNIX_TIMESTAMP() a MySQL-related (MariaDB, Percona) function, It does not work in other database systems (SQLite, PostgreSQL).

I think that the chosen solution (getRequestTime()) works properly, is easy to understand and helps with testing.

Perfect, thank you!

I think we should then merge both RTBC'd issues, but before tagging a new release, give it some time and testing. Quite a lot of changes recently. But nice!! :)

Merging this RTBC'd issue, thanks all!

Sorry I didn't see this needs a local rebase!

Rabsed the MR, it's ready for review now.
Thanks!

Thanks @ankithashetty well done!

Automatically closed - issue fixed for 2 weeks with no activity.