Alternate behavior for path redirect interaction:
When generating a new url alias, if the exact alias exists in path_redirect (alias and src), instead of generated a new url alias with '-0' at the end, the url is deleted from path_redirect table.

CommentFileSizeAuthor
#9 261615_pathauto_9_redirect_delete.patch1.1 KBgreggles
#8 redirect_delete-261615-8.d6.patch1.64 KBFreso
redirect_delete.patch1010 bytesaufumy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aufumy’s picture

aufumy CreditAttribution: aufumy commented
Status: Active » Needs review

changing status to patch (code needs review)

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Can you describe some steps to reproduce the problem to test this patch?

aufumy’s picture

aufumy CreditAttribution: aufumy commented

Hi Greggles

1. Enable pathauto and path_redirect modules. Notice that the default pattern for nodes is '[title-raw]' in "/admin/settings/pathauto"

2. Create a story "/node/add/story" , e.g. title = 'My Nodes', url is automatically generated in url_alias table

+-----+--------+----------+
| pid | src    | dst      |
+-----+--------+----------+
|   1 | node/1 | my-nodes | 
+-----+--------+----------+

3. Add a url redirect "/admin/build/path-redirect/add", to make sure an old url of an html page works and points to the story created.
From: docs/my-nodes.html
To: node/1

+-----+--------------------+----------+-------+----------+------+
| rid | path               | redirect | query | fragment | type |
+-----+--------------------+----------+-------+----------+------+
|   1 | docs/my-nodes.html | node/1   |       |          |  301 | 
+-----+--------------------+----------+-------+----------+------+

Now there are 2 urls pointing to 'node/1' : 'my-nodes' and 'docs/my-nodes.html'

4. Decision is made to regenerate pathauto following as closely as possible the old url pattern

  • In "/admin/settings/pathauto" set the pattern for all story paths to be "docs/[title-raw].html"
  • Delete aliases in "/admin/build/path/delete_bulk" or using sql
  • Don't delete the url redirect in "/admin/build/path-redirect", as the pathauto/token generated url may not necessarily match the old url.
  • Edit and save the node, to regenerate the url, or use the bulk regeneration function.

5. However, in the case when the pathauto/token generated url would exactly match, when pathauto runs, if there was no entry in the url redirect table for the story it would normally create the url alias "docs/my-nodes.html", however since there is a matching entry it creates "docs/my-nodes.html-0". So now there are 2 urls which point to the story.

I suggest instead when pathauto runs, if it finds exactly the same matching url pointing to the same node, it deletes it from path redirect, and adds the one url as "docs/my-nodes.html"

Therefore when the primary url desired for 'node/1' is exactly the same as an existing url in path_redirect, instead of changing the primary url, by adding '-0', the secondary urls are removed from path_redirect.

Thanks
Audrey

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs review » Reviewed & tested by the community

Audrey - thanks for the detailed steps to repeat the bug and for the patch. A slightly easier way (and perhaps more common case) is:

1. Create a node with the title "my title" when your pathauto setting is [title-raw]
2. Edit the node and change the title to "my title 2"
3. Edit the node and change the title back to "my title"

The problem manifests itself under this simplified scenario as well ;)

In my opinion, this appears ready to go. I'd like to see if Freso has any feeling on it.

One weakness I found is that if there is an existing feed alias then that isn't handled quite properly but I think that is a problem with the way that Pathauto makes assumptions about feed aliases and not really with this specific bug.

Freso’s picture

Freso CreditAttribution: Freso commented

My first impression (from just reading the patch) was that this ought to be fixed by path_redirect, but after actually reading the comments, I agree. At least with the scenario in #3. I think the test case provided in #4 should be handled properly by path_redirect... but then I realised we're injecting the redirect directly into the database. Doesn't path_redirect have a function that'll do this? If not, I'd say we should go and file a feature request! :)

To summarise:
The patch looks good as a temporary fix for 5.x-2.x/6.x-1.x. For 6.x-2.x, we should work towards using a function in path_redirect. (Now I'm side tracking again, but with the API function we could use function_exists instead of db_table_exists - thus saving a hit to the db, no? =))

Freso’s picture

Freso CreditAttribution: Freso commented

Actually, after posting this, I realised it's a SELECT there, and no INSERTing or UPDATE'ing is going on, so I'm not sure the DELETE is in the proper place... ? Where do we set up the path_redirect? I know I'm making the issue a bit more complicated now, but perhaps db_query_range("SELECT rid FROM {path_redirect} WHERE path = '%s'", $alias, 0, 1) should become db_query_range("SELECT rid FROM {path_redirect} WHERE path = '%s' AND redirect = '%s'", $alias, $src, 0, 1) instead (I'm not familiar with db_query_range() syntax, so that may not be fully kosher), and have the deletion happen at the insertion point?

Freso’s picture

Freso CreditAttribution: Freso commented

Eh. db_query_range("SELECT rid FROM {path_redirect} WHERE path = '%s'", $alias, 0, 1) should become db_query_range("SELECT rid FROM {path_redirect} WHERE path = '%s' AND redirect <> '%s'", $alias, $src, 0, 1), not what I wrote in the previous comment.

Freso’s picture

Freso CreditAttribution: Freso commented
Status: Reviewed & tested by the community » Needs review
FileSize
redirect_delete-261615-8.d6.patch1.64 KB

And here's my idea in a basic patch form. I'm not sure about which variables to pass to the SQL (of $old_alias, $dest, $src), and I'm not sure of exactly where the deletion should happen. Let alone, whether placing it here makes other things go wonky... (This issue should be added to the list of functionality test cases!)

I do think the best solution to this whole ordeal is for path_redirect_save() to handle this, but I also realise this can't be expected to happen in time for 5.x-2.3/6.x-1.1.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
FileSize
261615_pathauto_9_redirect_delete.patch1.1 KB

After discussion offline, Freso and I agree that this is a problem for Pathauto to clean up. He also encouraged me to use the real API instead of just a db query.

The attached patch makes the assumption that if path_redirect_save exists then the path_redirect database table exists. That seems like a reasonable assumption to me.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

..and...

@aufumy - can you test this patch and see if it works for you?

Freso’s picture

Freso CreditAttribution: Freso commented
Version: 5.x-2.x-dev » 6.x-1.x-dev
Status: Needs review » Reviewed & tested by the community

Patch looks good and works as expected (tested on 6.x-1.x). RTBC.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Title: path redirect existing url, duplicates in url_alias with "-0" » If path redirect has an existing duplicate redirect, new alias generated with -0
Status: Reviewed & tested by the community » Fixed

Thanks Freso and aufumy - this is now fixed in both 6.x and 5.x.

Woohoo - one step closer on http://groups.drupal.org/node/11551 :)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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