Redirects deleted too early

I think the issue here might be when redirects you've entered haven't been accessed yet ({redirect}.access field is 0). This would certainly be the case with redirects you've just created although also could be redirects you created months ago that just happen to not have been ever hit.

The query that purges inactive redirects simply looks for redirect rows where the access field is less than the current time minus the auto deletion interval

$query->condition('access', REQUEST_TIME - $interval, '<');

Because we can't distinguish between newly created redirects (with access of 0) and old redirects that just happen to never have been hit (access of 0) I think the solution should be to set the access field to the current timestamp when a redirect is first added. Let me try rolling a patch.

I think that just adding another condition to the query to pull inactive redirects to check that the count field is greater than 0 will fix this. A newly created redirect as noted above will have an access time of 0 and so will get caught up in the query noted above. Adding a condition that the count of accesses of the redirect be greater than 0 should address this.

I've had this problem too. I created a load of redirects, which were deleted by cron before they were used. I've temporarily fixed by disabling the auto-delete setting.

Instead of checking that the redirect has been accessed though, which could mean that redirects are never deleted if not accessed, why not simply set an access when the redirect is first created? That will effectively give the redirect 6 months (or whatever has been set) to be used.

Here is a patch that makes the access time default to the current timestamp instead of 0.

This patch also changes the previously failing test-assumption (it was assuming $redirect->access = 0).

#6 works for me. Thanks @torotil !

I marked #1747500: Delete never accessed redirects after the set purge period (like in D6) as a duplicate of this one.

I think the patch should use REQUEST_TIME instead of time().

Closed #2025525: Redirect Expiration Feature Deletes New Redirects as a duplicate of this one

We could use solution in #1853144: Store created date/time if gets merged

What about existing redirects? Should this patch fix those in an update hook?

For sites which expire redirects after a set time, trying to delete a massive number of redirects at once could cause a problem. emsearcy suggested the following which would space the deletions across 180 days:

UPDATE redirect SET access=UNIX_TIMESTAMP()+FLOOR(RAND()*15552000) WHERE access=0;

Here is solution based on the patch from #1853144: Store created date/time. The date that a redirect is created is now also taken into account when purging redirects.

Explanation of the patches:

• redirect-purge-1396446-12-tests-only.patch
  For the testbot, to demonstrate the problem.
• redirect-purge-1396446-12
  The fix based on #1853144: Store created date/time. Should not been evaluated by the testbot while #1853144: Store created date/time has not been committed yet.
• redirect-purge-1396446-12-for-testbot.patch
  For the testbot, to demonstrate that the change fixes the issue. This includes the changes from #1853144: Store created date/time.

I modified @MegaChriz's patch to work with the current rc3 release

As mentioned above the solution in #18 does not permit expiring redirects which were never accessed. I still feel this patch should do that, and include a fix for existing redirects in an update hook.

Upgrading to Major as unexpected data deletion is kinda sorta a big deal :)

I also took the "created" date approach. I used @MegaChriz's patch on https://www.drupal.org/node/1853144, then @frank.schalkwijk's #18 above. This combination adds a "created" column in the redirect table and a hook_update sets the value as "0" for existing redirects.

• I added an additional filter to the purge query which excluded redirects with zero "created" values from being deleted on Cron.
• I added a Drush command which allows for setting the created value on redirects with that value set to 0. This command has two options:
  • Date: which allows the user to specify the date the value will be set to (defaults to Now)
  • Limit: which allows the user to set a specific number of values (defaults to all)

I realize I added code from another existing ticket but, in my opinion, that solution would be the answer to this problem even if the other ticket didn't exist.

Setting to needs review after adding patch.

My patch was against 7.x-1.0-rc3. Should it have been against 7.x-1.x-dev?

@Tschet yes, please post a patch against the latest HEAD of 7.x-1.x-dev.

@Tschet patch for #21 appears to resolve the issue for us when applied to 7.x-1.0-rc3, but I'm reluctant to apply a patch in prod for 4+ year old issue without some feedback from the project maintainers. I've attempted to refactor the patch so it can apply to 7.x-1.x-dev to keep this conversation moving forward.

I think the existing test of the deletion of the redirect when cron was run is actually confirming the bug and is now failing because the redirect isn't being deleted. Please correct me if I'm wrong, but when redirect is configured to delete unused redirect that are more than a week old in http://cgit.drupalcode.org/redirect/tree/redirect.test?h=7.x-1.x#n191 and then a redirect is created http://cgit.drupalcode.org/redirect/tree/redirect.test?h=7.x-1.x#n197, it shouldn't have been deleted in http://cgit.drupalcode.org/redirect/tree/redirect.test?h=7.x-1.x#n220 because it wasn't > 1 week old.

I think the test is only failing because it was written in a way that exploited this bug.

Also, the comment on the new function redirect_created_zero_find() says "Find baby name nodes based on supplied criteria.", which I'm guessing was copied from some sample code. This should be updated.

I believe this incorporates all of the important changes from # 21, without the errors. It should work on the 7.x-1.x-dev.

Fatal error: Cannot redeclare drush_redirect_set_created() (previously declared in /var/www/html/sites/all/modules/redirect/redirect.drush.inc:51) in /var/www/html/sites/all/modules/redirect/redirect.drush.inc on line 104

This function was added to redirect.drush.inc twice in the patch from #33.

Same issue with redirect_created_zero_find(). Really hoping the tests to run now.

I've fixed @Tschet's patch so the tests run again, but this fails the same test as it did #28. Like I said in #31, I believe that test is now wrong.

I suppose it would be easier if #1853144: Store created date/time is done first. Now are we trying here to fix two issues at once, which I think makes it harder to review. It seems to me that the feature for the Drush command should be a seperate issue.

So please review and improve #1853144: Store created date/time and get that one committed. Then return back to this issue.

I updated the issue summary and set the status of this issue to "postponed".

This patch is a modification of @Kreynen's. I've removed all of the duplicate code from #1853144: Store created date/time. I've also removed the Drush integration so that it can be handled in its own case.

There were some use cases not covered in original purge query modifications, that I've added. This patch assumes that that redirects where access and created = 0 should be kept. This is to be compatible with any existing redirects that have been created. (Such as a scenario where someone has turned off purging until these issues are resolved.)

This patch is dependent on the patch from #1853144: Store created date/time as indicated in the issue status

Just realized the patch from #47 is made relative to drupal root. Oops.

This is critical because it causes data corruption that is not recoverable without manually recreating that data.

I created a combined patch from two patches: issues #1396446: Redirects deleted too early + #1853144: Store created date/time, but without tests.
Just apply the patch and execute drush updb -y.

It seems the patch in #50 was created with respect to the user's Drupal root, rather than within the Redirect directory. I've updated the file paths to make it apply properly.

The patch from #50 caused testPageCache to fail. I've updated that test to accommodate this new feature, and added another test to make sure these cron purges are behaving correctly.

This passes manual testing and looks great to me.

Hello,

I tested the patch #54 and it works for me.
I think it is ready to be merged.

I am just wondering how we deal this with #1853144: Store created date/time?

#1853144: Store created date/time has been RTBC with no action from maintainers for years, but never merged. I'm really hoping that because this patch now fixes the bug in the testing used to confirm the functionality in previous releases that contributed to data loss, it will actually get reviewed by the maintainers and merged.

I don't really see why this issue would get committed if the other one didn't. But anything to make it easier for this to be committed.

I think Berdir hangs out in slack so it might be possible to ping him there and see if anyone can look at this.

Berdir is plenty busy with about a million other things and has no intention of getting involved in the 7.x-1.x version of this project, sorry :) I only maintain the 8.x-1.x version.

@Berdir:

Haha, no worries.

That leaves Dave Reid. The relevant master issue, which he opened previously, is #2514278: Evaluate adding a comaintainer

Here's a new patch that should fix the failed test due Cannot redeclare redirect_update_7103()

If and when this gets committed, we should make sure to also credit anyone who worked on #1853144: Store created date/time which at one point was a separate issue, but then was rolled into this one.

@Delphine Lepers what problems did you find that resulted in moving the status from RTBC->Need Work? Adding the interdiff to make it easier to view the changes between the patches.

Hello!
Just tested the patch in #67: newly created redirects, with count = 0 are being retained during then cron run. I believe it is good to go.

I should begin by saying that I initially didn't know the redirect module had this feature. I had simply written something to put in my own hook_cron, using my own query. Only when I started to dig into the redirect module looking for redirect_delete did I stumble upon redirect_purge_inactive_redirects. When I looked at the code I saw what it would do to new items, which brought me to this issue. I wrote this without any knowledge of this issue, the existing patches, proposed created column, etc...

This patch is in no way attempting to detract from adding a created column to the redirect table. That is a column I would very much like to see added.

Having said that,

This patch doesn't make any schema changes, nor does it require retroactively assigning values to existing records. The basic premise is that in addition to purging any records where access is not zero and access is less than the interval limit, it will also purge records where access is zero and rid is less than that of the most recent accessed stale record. The idea being that if the rid is a lower number it must be older.

The only issue I can think of, should somebody, within the interval window, edit a record that is older than the interval and has not yet been accessed, that record will be purged. Maybe that problem has been solved already, don't know, didn't look, but at the moment I can't think of how a created column would fix that with out the addition of an updated column.

Even though I intend to use this patch on my site in the coming days, I wouldn't recommend others do the same. It suits my needs, it may not suit yours.

Anyways, submitted for your consideration...

Used patch #67, can confirm redirects are not being deleted too early now (access = 0) and newly created redirects are getting the current time in the created field

Updated patch #67 to the latest code and added a check if the created column does not exists yet before adding it

Tried patch #73 with Redirect 7.x-1.0-rc4. It seemed to have fixed the problem at first. It passed multiple tests on different environments including prod, but then it happened again.

In my redirect settings, I have it to Delete redirects that have not been accessed for 1 yr and delete a max of 10 redirects per cron run.

The patch was applied, cleared cache, updated the db, which applied the db updates (Redirect 7105 Adds the {redirect}.created field.).

When I ran cron manually via drush, the redirects with count = 0 stayed.

When the cron ran automatically as scheduled, the redirects with count = 0 are gone again.

Not sure if the patch was meant for 7.x-1.x-dev?

We always roll patches against dev. to make sure it safely can be applied and works with the latest changes.

I applied the patch in #73 on 7.x-1.x-dev some days ago and the issue seems to be solved, thanks!

Closing this as Outdated as Drupal 7 is EOL.