Set last_access->value to 0 or NULL for new redirects

Ok so this is simple enough.

We can just set the $redirect->last_access->value to 0 for new redirects:

/**
 * Implements hook_ENTITY_TYPE_presave().
 */
function redirect_metrics_redirect_presave(Redirect $redirect) {
  // Initialise the default metrics when a new redirect is created.
  if ($redirect->isNew()) {
    // Set the access count if it's empty.
    if ($redirect->access_count->isEmpty()) {
      $redirect->access_count->value = 0;
    }
    // Set the access time if it's empty.
    if ($redirect->last_access->isEmpty()) {
      $redirect->last_access->value = 0;
    }
  }
}

Then use rewrite results of Redirect: Last accessed in the view so that Drupal doesn't try to put a date when the value is 0.

{% if last_access__value == "0"%}
   Never
{% else %}
   {{ last_access }}
{% endif %}

Here is a patch built on the patch from this issue

You will need to run drush cim --partial --source=modules/contrib/redirect_metrics/config/install to pull in the changes on the view.

I thought about also changing ->setInitialValue(\Drupal::time()->getRequestTime()) to ->setInitialValue(0) in the for last_access in RedirectMetricsFieldDefinitions.php as well as I don't think time stamping a redirect as accessed, before it has been accessed is a good idea.

I can easily add that to the patch as well.

We will need to fix the tests. And maybe provide an update hook to set redirects last access that have an access count of 0 to zero.

I think in the test we need to change this line:

$this->assertEquals($test_request_time, $redirect->last_access->value);

to:
$this->assertEquals(0, $redirect->last_access->value);

Curious if this fixes any of the broken tests...

If I am looking at this right, $test_request_time = \Drupal::time()->getRequestTime(); is not needed in the tests if we are setting new redirects to 0.

looks like there is a problem with the tests?

There was 1 error:

1) Drupal\Tests\redirect_metrics\Functional\RedirectReportsTest::testRedirectReports
Behat\Mink\Exception\ResponseTextException: The text "foo" appears in the text of this page, but it should not.

And:

Unknown
fail: [run-tests.sh check] Line 0 of :
FATAL Drupal\Tests\redirect_metrics\Functional\RedirectReportsTest: test runner returned a non-zero error code (2).

Setting this to a bug report because it is a confusing UI/UX to mark a redirect that has never been accessed with a timestamp of having been accessed. This also causes issues in migrations, as I mentioned above.

We should write an update hook to set all redirects with a count of 0 to a last access of 0. The created date can be retained for when the redirect was created of course.

Fixed the tests, and deprecated FilterResponseEvent. Added a view page for never accessed do distinguish them from stale.

here is one without the deprecation changes.

Thanks! Can you provide an interdiff? I am not seeing what changed.

By the way, how is the patch working for you? It would be nice to get the maintainer to take a look at this issue.

Also might want an update hook to set redirects that have an access count of 0, to have a last access of 0 as well.

Maybe NULL is better than 0. Either way, stale redirects can be calculated off the created date, so last access timestamp should only be populated when the redirect is actually accessed.

NULL would probably be better than zero so that we can use the default views output for empty instead of rewriting the field.

Also, an update script would be good to set last_access values with an access_count of 0 to NULL.

Here is a patch with the function.

Here are the basic tests added with NULL value.

The maintainer should decide what way this should be implemented in views so that those can be reworked if needed and so that tests can be rewritten.

Whoops. here you go.

2.x branch needs dev release and test parameters.

+ // Initial field values should be 0. Redirects should be considered

0 should be changed to NULL

This is better for the reports test: (lines 51-57)

    // The popular report will show all aliases, ordered by the most used one.
    $this->drupalGet('admin/config/search/redirect/popular');
    $this->assertSession()->elementContains('css', 'tbody tr:first-child td:nth-child(2)', 'bar');
    $this->assertSession()->elementContains('css', 'tbody tr:first-child td:nth-child(6)', '5');
    $this->assertSession()->elementContains('css', 'tbody tr:last-child td:nth-child(2)', 'foo');
    $this->assertSession()->elementContains('css', 'tbody tr:last-child td:nth-child(5)', 'Never');
    $this->assertSession()->elementContains('css', 'tbody tr:last-child td:nth-child(6)', '0');

Thanks for the review!
Yeah, I should write an update hook.

Bumping back to NR for the update hook.

Should probably have a couple of people test it.

You have to install the module without the patch, then apply the patch, then run drush updb etc.
All redirects that have an access count of 0 will have the last_access timestamp set to NULL.

Here is a patch for those who prefer patches.