Redirect provides functionality that creates a redirect to a system path when the alias is deleted. For example: there is an alias /about-our-amazing-company to node/123. When we delete that alias, the Redirect module creates a redirect to node/123 from /about-our-amazing-company so search engines still refer to the correct page.

However, the code that creates the redirect is commented in redirect_path_delete(). I have no idea why.

/**
 * Implements hook_path_delete().
 */
function redirect_path_delete($path) {
  if (!variable_get('redirect_auto_redirect', TRUE)) {
    return;
  }
  elseif (isset($path['redirect']) && !$path['redirect']) {
    return;
  }
  elseif (empty($path)) {
    // @todo Remove this condition and allow $path to use an array type hint
    // when http://drupal.org/node/1025904 is fixed.
    return;
  }

  // Redirect from a deleted alias to the system path.
  // if (!redirect_load_by_source($path['alias'], $path['language'])) {
    // $redirect = new stdClass();
    // redirect_object_prepare($redirect);
    // $redirect->source = $path['alias'];
    // $redirect->redirect = $path['source'];
    // $redirect->language = $path['language'];
    // redirect_save($redirect);
  // }
}
CommentFileSizeAuthor
#16 redirect-deleted-paths-1763436-16.patch2.67 KBsylvainm
#15 interdiff-redirect-1763436-14-15.txt595 byteshanoii
#15 redirect-deleted-paths-1763436-15.patch2.67 KBhanoii
#14 interdiff-1763436-13-14.txt1.11 KBhanoii
#14 redirect-deleted-paths-1763436-14.patch2.8 KBhanoii
#13 redirect-deleted-paths-1763436-13.patch2.3 KBhanoii
#11 1763436-redirect-deleted-paths-11.patch1.99 KBcgmonroe
#10 1763436-redirect-deleted-paths-10.patch931 bytestorotil
#1 redirect_hook-path-delete-commented_1763436_1.patch893 bytesadognameddog

Comments

adognameddog’s picture

adognameddog commented
Status: Active » Needs review
StatusFileSize
new redirect_hook-path-delete-commented_1763436_1.patch893 bytes

Here is the patch removing the comments.

torotil’s picture

torotil commented
Status: Needs review » Reviewed & tested by the community

Patch looks fine to me and the requested feature seems reasonable. Setting to RTBC.

shaundychko’s picture

shaundychko
Location Vancouver, Canada
commented

Works for me too, and this patch is pretty important.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

The biggest problem I have is the most typical use case for a path alias being deleted is when the node itself is also being deleted, in which case the redirect would still lead to nothing. I'm not in favor of bringing back this functionality, especially enabled by default.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Postponed
torotil’s picture

torotil commented

What about first checking if the system-path still exists? (ie. via drupal_valid_path())

shaundychko’s picture

shaundychko
Location Vancouver, Canada
commented

Another, minor, problem I've noticed is that if a node has it's title change, then changed back to what it was originally (and having the node alias generated from the title), there will be "infinite loop" errors reported in the db log. Everything seems to work OK nevertheless, so I suppose these can be ignored...

shaundychko’s picture

shaundychko
Location Vancouver, Canada
commented

Actually, this "infinite loop" business is annoying, so I guess best practice is to set pathauto to leave the old alias intact, despite the SEO duplicate content penalty?

torotil’s picture

torotil commented

Hi shaun,

#1796596: Fix and prevent circular redirects is dealing with redirect loops. This is about creating a redirect if a path alias is deleted.

torotil’s picture

torotil commented
Status: Postponed » Needs review
StatusFileSize
new 1763436-redirect-deleted-paths-10.patch931 bytes

Here is a patch that checks whether the system path is still valid before creating the redirect.

cgmonroe’s picture

cgmonroe commented
StatusFileSize
new 1763436-redirect-deleted-paths-11.patch1.99 KB

Needed this functionality but thought that it needed to be optional and not change existing behaviour.

Attached is a modified version of torotil's patch that adds an setting option to turn this on or off (off is default so current behaviour stays the same). This option of overridden by the existing "Automatically create redirects.." option.

pere orga’s picture

pere orga
Primary language Catalan
commented
Priority: Major » Normal
Status: Needs review » Needs work

The patch looks good to me, apart from not deleting the new redirect_auto_redirect variable on hook_uninstall().

Changing the priority to Normal.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Status: Needs work » Needs review
StatusFileSize
new redirect-deleted-paths-1763436-13.patch2.3 KB

Well, I was looking for this and looks like a nice patch.

Just a re-roll to the latest dev and also simplify a bit the logic having the settings separate instead of one overriding the other one.

Adding the variable to hook_uninstall() or rather redirect_variables() in this case as per #12

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
StatusFileSize
new redirect-deleted-paths-1763436-14.patch2.8 KB
new interdiff-1763436-13-14.txt1.11 KB

I found a but in the previous patches (all of them) in which in a rare case (but possible) that the redirect already exists, this gives out a key constraint fatal error.

Attached is the same logic but with the same approach as redirct_path_update().

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
StatusFileSize
new redirect-deleted-paths-1763436-15.patch2.67 KB
new interdiff-redirect-1763436-14-15.txt595 bytes

Hmm, no need to disable all aliases, carried over from _update(), but not really necessary.

sylvainm’s picture

sylvainm commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new redirect-deleted-paths-1763436-16.patch2.67 KB

It looks good for me, thx !
I tested it and it works as expected.

Just a little coding standard I saw :

elseif (property_exists($existing, 'status') && $existing->status == 0)  {

There are 2 spaces instead of 1 between ) and {
Attached patch solves that

vasike’s picture

vasike
Location Ramnicu Valcea
commented

i confirm the issue and the solution (latest patch #16)
Thanks

pifagor’s picture

pifagor commented
pifagor’s picture

pifagor commented

RTBC

alex_optim’s picture

alex_optim
Location Tallinn
commented

Looks good.

pifagor’s picture

pifagor commented

  • pifagor committed a6e5c93 on 7.x-2.x authored by SylvainM 
    Issue #1763436 by hanoii, torotil, adognameddog, cgmonroe, SylvainM,...

  • pifagor committed 84ca068 on 7.x-1.x 
    Issue #1763436 by hanoii, torotil, adognameddog, cgmonroe, SylvainM,...
pifagor’s picture

pifagor commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

donquixote’s picture

donquixote commented
Related issues: +#1025904: path_delete() cannot handle multiple aliases

I should mention this will not work reliable with the current version of Drupal core.
#1025904: path_delete() cannot handle multiple aliases

path_delete() can delete multiple paths at once, but only for the first of them the hook_path_delete() is invoked.
So anyone who wants to use this should patch core.

EDIT: I just notice this is already mentioned in a code comment in the function.