Replace relevancy column with sorting on log(count)

Function UNIX_TIMESTAMP does not exist in PostgreSQL, only with MySQL. Do check this link.
Quick fix until a proper patch is released is to edit /var/www/mysite/web/modules/contrib/redirect/modules/redirect_404/src/SqlRedirectNotFoundStorage.php and replace 3 occurrences of "UNIX_TIMESTAMP(NOW())" with "EXTRACT(EPOCH FROM NOW())".

Thanks. I think we just need to move the generation of the current timestamp to PHP and pass that in as an argument.

Done as @Berdir suggested at comment #3.

Replaced IFNULL() with COALESCE()

I still don't really understand why we needed to introduce all that complexity, a simply query to delete the records that were not updated for the longest would have been *so* much simpler.

Now have to debug fun things like this. Apparently $cut_off is *really* small: 2.98101277755881e-59.

And apparently sqlite doesn't support comparing values with such a high precision and just deletes everything. Switched to fetching the actual relevancy and deleting based on that. That passes but of course makes the whole thing pointless as we are then basically back to just deleting everything that is higher than a certain relevancy ad we don't need the whole calculation anymore :p

I also like how the test sets the row limit to 5 but asserts that we have 6 records ;)

halp?

Discussed this a while back, we agreed on using log() to sort on the count. Then we still get a sort based on usage, but it's a lot less complicated than this.

Tried to improve the delete query using log() as discussed (supported by MySQL and PostgreSQL). Still need to define the condition on timestamp for the SQLite database driver case and fix the tests. Will continue tomorrow.

Improved purgeOldRequests() and fixed the test, now it passes locally.
I didn't use log() anymore because I get incorrect values, i.e. "count => sort_condition result":
1 -> 0,
5 -> 0,
12 -> 1,
300 -> 1,
315 -> 1,
1557 -> 1.

In the test I've used this count example and the timestamp as @Berdir and I have discussed yesterday.

1. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -102,34 +102,30 @@
          // Define sort_condition with the appropriated function. It is used to
          // compute the record relevancy based on its count (amount of visits).
   -      $count_length = 'LENGTH(count)';
   -      if ($this->database->driver() == 'pgsql') {
   -        $count_length = 'CHARACTER_LENGTH(TO_CHAR(count))';
   -      }
   -      $sort_condition = 'floor(log(10, ' . $count_length . '))';
   +      $count_length = ($this->database->driver() == 'pgsql') ? 'CHARACTER_LENGTH(TO_CHAR(r404.count))' : 'LENGTH(r404.count)';
    
   

   both before and after is way too complicated.

   The discussion was length XOR log. not both.

   You can simplify this to $count_log = 'log(10, count)'. Nothing else. No driver logic (yet).

2. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -102,34 +102,30 @@
   +      $query->addExpression('ceil(' . $count_length . ')', 'sort_condition');
          $query->groupBy('sort_condition');
   +      $query->groupBy('count');
   +      $query->groupBy('timestamp');
          $query->orderBy('sort_condition', 'DESC');
   

   Why groupBy()? We don't want to group, just orderBy('count_log')->orderBy('timestamp')

Ok, discussed and checked with @Berdir, now purgeOldRequests() should have the expected implementation. Fixed the test for the SQLite case as well.

1. +++ b/modules/redirect_404/redirect_404.install
   @@ -48,12 +48,6 @@ function redirect_404_schema() {
          ],
   -      'relevancy' => [
   -        'description' => 'A float number that defines the relevancy of a record.',
   -        'type' => 'float',
   -        'not null' => TRUE,
   -        'default' => 1.00,
   -      ],
        ],
        'primary key' => ['path', 'langcode'],
   

   We need an update function to remove this column for existing sites.

2. +++ b/modules/redirect_404/redirect_404.services.yml
   @@ -6,6 +6,6 @@ services:
          - { name: event_subscriber }
      redirect.not_found_storage:
        class: Drupal\redirect_404\SqlRedirectNotFoundStorage
   -    arguments: ['@database', '@config.factory']
   +    arguments: ['@database', '@config.factory', '@datetime.time']
   

   This service only exists in 8.3, we can't add that yet.

3. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -75,16 +73,15 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
          ->fields([
   -        'timestamp' => REQUEST_TIME,
   +        'timestamp' => $this->dateTime->getCurrentTime(),
   

   yes, stick to REQUEST_TIME for now.

4. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -105,20 +102,42 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
    
   -    // Determine cutoff level to get the current min relevancy we want to keep.
   +    // Settle cutoff to get [count_log and] timestamp for the delete condition.
   

   Why Determine => Settle, I actually like the old verb more I think.

5. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -105,20 +102,42 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
   +    if ($this->database->driver() != 'sqlite') {
   +      // This expression is used to compute the relevancy based on 'count'.
   +      $query->addExpression('floor(log(10, count))', 'count_log');
   

   make an explicit check for mysql and postgresql. no idea if other databases support this. And add a comment that sqlite does not support log() functions, so we only consider the timestamp.

6. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -105,20 +102,42 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
   -    // Delete records below cutoff value, if given. Otherwise skip the cleanup.
   +    // Delete records having older timestamp and less visits than cutoff.
   +    // Otherwise skip the cleanup.
   

   and less visits (on a logarithmic scale) ...

7. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -105,20 +102,42 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
   +        // Delete rows with same count_log AND older timestamp than cutoff ...
   

   no need for ..., just .

8. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
   @@ -19,16 +20,27 @@ class Fix404RedirectCronJobTest extends KernelTestBase {
        $this->installSchema('redirect_404', 'redirect_404');
   +    /** @var \Symfony\Component\HttpFoundation\RequestStack|\PHPUnit_Framework_MockObject_MockObject $request_stack */
   +    $request_stack = $this->getMock('Symfony\Component\HttpFoundation\RequestStack');
   +    $time = new Time($request_stack);
   +    $this->dateTime = $time->getCurrentTime();
   

   is this left-over? I don't think we need this?

9. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
   @@ -19,16 +20,27 @@ class Fix404RedirectCronJobTest extends KernelTestBase {
   -    // Set the limit to 5 just for the test.
   +    // Set the limit to 3 just for the test.
        \Drupal::configFactory()
          ->getEditable('redirect_404.settings')
   -      ->set('row_limit', 5)
   +      ->set('row_limit', 3)
          ->save();
   

   I would suggest to to two test runs with the same data. Once keep 3 records, once keep 4 or 5, with different asserts.

10. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
    @@ -76,20 +89,26 @@ class Fix404RedirectCronJobTest extends KernelTestBase {
    +   *   (optional) The timestamp of the last visited request. Based on the
    +   *   current system time, see the following to set a dynamic timestamp:
    +   *     - 0: today
    +   *     - 86400: 1 day ago (yesterday)
    +   *     - 604800: 1 week ago
    +   *     - 2629743: 1 month ago
    

    instead of documenting this and calculting yourself, you could simply use strtotime('-1 week'), then you get a timestamp and it is self-documenting.

Done as suggested above.

A nitpik:

+++ b/modules/redirect_404/redirect_404.install
@@ -48,14 +48,24 @@ function redirect_404_schema() {
+    $database->update('redirect_404')
+      ->fields(['relevancy' => NULL])
+      ->execute();
+  }

We don't need to specifically delete the entries. It should be automatically done when we drop the field.

Thank you :)
removed the unnecessary lines in the hook_update() function.

Looks pretty ok now, below are some notes to me for some cleanup before commit.

1. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -105,20 +91,42 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
   +    if ($this->database->driver() == 'mysql' || $this->database->driver() == 'pgsql') {
   +      // This expression is used to compute the relevancy based on 'count'.
   +      $query->addExpression('floor(log(10, count))', 'count_log');
   +      $query->orderBy('count_log', 'DESC');
   +    }
   +    // SQLite does not support log() function, so we only consider 'timestamp'.
   

   restructure comments a bit here.

2. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -105,20 +91,42 @@ class SqlRedirectNotFoundStorage implements RedirectNotFoundStorageInterface {
   +        // Or delete all the rows with count_log less than the cutoff.
   +        $condition = $delete_query->orConditionGroup()
   +          ->where('floor(log(10, count)) < :count_log1', [':count_log1' => $cutoff['count_log']])
   +          ->condition($and_condition);
   

   also here, it is technically and OR, but in human-speak, we're not doing an OR but an AND really (delete records matching condition A and delete records matching B).

3. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
   @@ -24,51 +24,96 @@ class Fix404RedirectCronJobTest extends KernelTestBase {
    
   -    // Set the limit to 5 just for the test.
   +  /**
   +   * Tests adding and deleting rows from redirect_404 table.
   +   */
   +  function testRedirect404CronJob() {
   +    // Set the limit to 3 just for the test.
        \Drupal::configFactory()
   

   check if we can avoid duplicated code.

Committed.

Argh, forgot to actually run the sqlite tests, which didn't pass. This fixes them and does a bit more cleanup.

Committed.