Duplicate timestamp placeholder in statistics query

@Beakerboy: I think that the original code is right. The value of :timestamp get compared with the timestamp value in the database. When the value in the database is higher then use the value from the database and when that is not the case then use the value of :timestamp. The value of :timestamp should be the same, therfor the original is to me right. To your defence, in most queries the 2 placeholders should have a different name. Only this is one of those rare exceptions.

@beakerboy Can you add a comment to the patch with why this change is necessary. The stuff with when EMULATE_PREPARES is set to FALSE. Now we need a test that checks that the query uses the placeholders :timestamp1 and :timestamp2. We want to make sure that this change does not get reverted in the future.

@daffie I'm not sure that testing this is correct. To test properly - i.e. that we don't do this again in the future is extremely expensive. I think in this instance it is okay to accept the fix and move on.

With the post from @alexpott in comment #8, is this the patch for me RTBC.

+++ b/core/modules/statistics/src/Plugin/migrate/destination/NodeCounter.php
@@ -96,7 +96,7 @@ public function import(Row $row, array $old_destination_id_values = []) {
-      ->expression('timestamp', 'CASE WHEN timestamp > :timestamp THEN timestamp ELSE :timestamp END', [':timestamp' => $timestamp])
+      ->expression('timestamp', 'CASE WHEN timestamp > :timestamp1 THEN timestamp ELSE :timestamp2 END', [':timestamp1' => $timestamp, ':timestamp2' => $timestamp])

The one thing that might help to remember to not incorrectly "optimise" this in the future is a code comment. I didn't know the

A query may have any number of placeholders, but all must have unique names even if they have the same value.

rule so this would help future me remember that.

Comment add, back to RTBC.

This looks pretty good. I went back and forth about test coverage but can agree with no test. I also agree with the inline comment.

However, right now, the inline comment is making a claim that doesn't seem to be documented anywhere else. Can we link the reference in the comment? https://www.drupal.org/docs/8/api/database-api/static-queries

Thanks!

Committed and pushed 07dd3e2ce7 to 9.1.x and 5d0d3ed093 to 9.0.x and 1b0cbeea4c to 8.9.x and 154038f140 to 8.8.x. Thanks!

Backported to 8.8.x as a very low risk bug fix that helps alternate db drivers.