Hi,
we've chosen "Create a new alias. Leave the existing alias functioning" as update action. When we create a page, it gets its alias. Now we manually add an additional alias for this page because the page should have it's normal (pathauto created) alias and a shortcut for references.

When we edit the page again, pathauto only checks for *one* existing alias in _pathauto_existing_alias_data and finds the latest one, i.e. the manually added one. As this differs from the pathauto-generated one, pathauto tries to create a new alias and add it on every save of the node.

But as the pathauto-generated alias still exists, we get a error message "user warning: Duplicate entry..." on every save of the node.

I think that pathauto should fetch *all* existing aliases for a node in _pathauto_existing_alias_data and in _pathauto_set_alias() it should be sth. like

  // Skip replacing the current alias with an identical alias
  if (!in_array($dst, $old_alias)) {...

Every other use of $old_alias would have to be changed to $old_alias[0].

What do you think about this? I could write a patch if you agree on this.

CommentFileSizeAuthor
#24 pathauto-nodebulkupdate-for-pgsql.patch1.09 KBJosh Waihi
#21 373841-test.patch2.64 KBDeFr
#14 pathauto-2.x_multiple_aliases_v4.patch8.55 KBFrank Steiner
#14 pathauto_multiple_aliases_v4.patch8.15 KBFrank Steiner
#8 pathauto_multiple_aliases_v3.patch8.18 KBFrank Steiner
#7 pathauto_multiple_aliases_v2.patch8.1 KBFrank Steiner
#3 pathauto_multiple_aliases.patch8.08 KBFrank Steiner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

Is nobody else getting this error?

greggles’s picture

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

I suggest

1) Include the full error (and search for it). I've seen mentions of that error a lot. Inserting it here will help other people find this issue.

2) Provide a patch. If you have an idea of how to fix it a patch is always helpful.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented
FileSize
pathauto_multiple_aliases.patch8.08 KB

Ok, let me try...

When

  • the pathauto setting is "Create a new alias. Leave the existing alias functioning."
  • and an additional alias has been created for a pathauto-aliased node
  • and this alias is found first when fetching existing aliases from the DB

then pathauto tries to recreate the formerly generated alias on every node save because it only compares it desired alias against the first existing alias it finds in the DB.
This leads to the error message

user warning: Duplicate entry 'test-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/746', 'test', '') in /usr/share/drupal/modules/path/path.module on line 113.

(here "test" is the pathauto alias).

Additionally, pathauto would only delete or redirect the first alias it finds in the DB and leave other existing aliases for a node unchanged.

The attached patch

  • makes _pathauto_existing_alias_data collect all existing aliases (and their pids)
  • makes _pathauto_set_alias compares the new alias against all existing aliases
  • causes the deletion of *all*, not only the first existing alias if the setting is "Create a new alias. Delete the old alias." (except an alias that matches the one that pathauto wants to create)
  • causes the deletion and redirection of all aliases if the setting is "Create a new alias. Redirect from old alias." (except an alias that matches the one that pathauto wants to create)

To avoid breaking things I changed _pathauto_set_alias so that it can take either a single pid and single old_alias or arrays of both. Some of the patch is only for renaming $pid to $pids and $old_alias to $old_aliases.

- hen creating an additional alias for pathauto-aliased node with settings "Create a new alias. Leave the existing alias functioning."

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented
Status: Active » Needs review
nicholas.alipaz’s picture

nicholas.alipaz CreditAttribution: nicholas.alipaz commented

testing, I am getting this error in my logs on all submissions:
array_search() [<a href='function.array-search'>function.array-search</a>]: Wrong datatype for second argument in /home/alipaz3/public_html/cbbeta/sites/all/modules/pathauto/pathauto.inc on line 402.

Also, does not seem to fix any of the issues.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

What version are you testing against exactly (-dev 1.x from which day)?

Also, what do you mean by "any of the issues"? It should fix only one issue, i.e., preventing the re-recreation of an existing alias.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented
FileSize
pathauto_multiple_aliases_v2.patch8.1 KB

Got the datatype errors, that was due to pre-initializing with NULL instead of array(). Thanks for spotting this!

Could you try the new patch?

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented
FileSize
pathauto_multiple_aliases_v3.patch8.18 KB

Updated to include the fix against unintended alias removals, see http://drupal.org/node/388626

Since the multiple_alias patch changes the case distinctions when to call path_set_alias with which parameters, the anti-removal fix looks a bit different here, so I included it. As deletion and redirection is done within a different if-case now, we just need to prevent calling path_set_alias if $dst is empty.

liquidcms’s picture

liquidcms CreditAttribution: liquidcms commented

not sure if related.. possibly not, but i get same error on every node save, but my settings are: Create a new alias. Delete the old alias.

and error is:


user warning: Duplicate entry 'journal/roto-wisi-' for key 2 query: UPDATE url_alias SET src = 'node/44', dst = 'journal/roto-wisi', language = '' WHERE pid = 114 in C:\Inetpub\websites\8to13\modules\path\path.module on line 100.

although, just noticed, possibly related to items with common titles and the -n at end of url - these were auto-created test nodes from the devel module

115	node/44	        journal/roto-wisi
116	node/44/feed	journal/roto-wisi/feed
117	node/45	        node-45-journal
118	node/45	        journal/roto-wisi-0
119	node/45/feed	journal/roto-wisi-0/feed
120	node/46	        node-46-journal
121	node/46	        journal/roto-wisi-1
122	node/46/feed	journal/roto-wisi-1/feed
123	node/47	        node-47-journal
124	node/47	        journal/roto-wisi-2
Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

It is related. pathauto will only delete *one arbitrary* old alias. So if you have two aliases (like for node/45 etc. in your case) and pathauto does not delete the one it tries to create, you will exactly run into this situation.

The patch should help here. Could you test it and report how it works for you?

garbanzito’s picture

garbanzito CreditAttribution: garbanzito commented

applied pathauto_multiple_aliases_v3.patch and it worked well for me when dealing with a situation much as described in comment # 3 -- thanks!

greggles’s picture

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

@Frank Steiner - thanks so much for your work troubleshooting and fixing this. I had never run into it before and now I see what the problem is. It won't show up for most people, but for those where it does...boy is it annoying.

Does this issue affect 5.x as well? If so, can you roll a patch for 5.x as well? If you don't use 5.x then that's OK, I can try to do it.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

I've never used Drupal 5.x so I guess I'm not a big help here :-) But I will at least try to port the patch for the -2.x branch, too, ok?

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
FileSize
pathauto_multiple_aliases_v4.patch8.15 KB
pathauto-2.x_multiple_aliases_v4.patch8.55 KB

Here are versions for the latests -dev releases for -1.x and -2.x, including the patch from http://drupal.org/node/388626

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

Hmm, is there anything else I can do to get these patches applied to the -dev versions?

greggles’s picture

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

Hi Frank - if you could write simpletests for it that would absolutely help. There is a basic simpletest file in Pathauto now which should make it easier to learn.

Otherwise, I just need some time to sit down and review it.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

Ok, I've no idea what simpletests are yet, but I will learn and try to do it :-)

DeFr’s picture

DeFr CreditAttribution: DeFr commented

I've applied the patch pathauto_multiple_aliases_v4.patch from #14 in a situation similar to the one described in #9, and it works great ! Thanks !

@Frank Steiner: Do you want/need help for the testing part of this patch ?

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

Would be nice, yes, because I haven't yet looked into the simpletest stuff, so I still don't even know what I would have to do :-D I simply don't have enough time at the moment as we are upgrading our whole chair from SLES 10 to SLES 11. So whatever you can/want to do get this started will be appreciated! You can also contact me directly.

DeFr’s picture

DeFr CreditAttribution: DeFr commented
FileSize
373841-test.patch2.64 KB

Ok, first stab at a patch for adding a test for this issue. It's only the test part, which highlights the fact that there's one test fail as is, and that it disapears when the patch from #14 is applied.

While a bit contrived, the test in the example is one of the most reliable ways to introduce the inconsistency which leads to the warning this bug deals with.

Michael-IDA’s picture

Michael-IDA CreditAttribution: Michael-IDA commented

@ #12 greggles

Does this issue affect 5.x as well? If so, can you roll a patch for 5.x as well? If you don't use 5.x then that's OK, I can try to do it.

Hi greggles,

No this isn't an issue in 5.x (specifically Pathauto 5.x-2.0-beta2). One of the sites we do is currently stuck on 5.x due to other module upgrade problems, it has hundreds of nodes with multiple pathauto aliases, with the odd node having 5+ aliases. (Client started out with static html files, converted to ??, converted to D4, ...)

We can confirm Pathauto 6.x-1.2 (D6.14) is deleting all other (even manually added) aliases upon a node edit as identified by Frank. I'll apply the patch(es) to a test site and see if I can break 'em.

Will take a couple weeks,

Sam

PS: Somewhat curious why we can't just rip the 5.x code in the offending section? Guess I'll look at that too

PS2: Congrats on a great module, where beta-2 is still in production (no don't ask, clients are clients)

Martin.Schwenke’s picture

Martin.Schwenke CreditAttribution: Martin.Schwenke commented

subscribing

Josh Waihi’s picture

Josh Waihi CreditAttribution: Josh Waihi commented
FileSize
pathauto-nodebulkupdate-for-pgsql.patch1.09 KB

I found this to be the same issue on PostgreSQL - this was due to the way the CAST comparison was done in the database. Basically, CHAR means somthing else to MySQL than it does to PostgreSQL. Please submit this patch at least (or make it awesomer) so bulk updates work on PostgreSQL

greggles’s picture

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

$cast = ($db_type = 'pgsql') ? 'TEXT' : 'CHAR';

$cast = ($db_type == 'pgsql') ? 'TEXT' : 'CHAR';

Did you mean to use the assignment operator (=) or the comparison operator (==)

I'd rather not do a database specific test if possible, but I think given the performance benefits of the cast it's probably worthwhile.

Josh Waihi’s picture

Josh Waihi CreditAttribution: Josh Waihi commented

Yeah I understand what your saying, its a gotcha I learn't building the PostgreSQL layer for D7 - Ideally we'd stay away from CASTs. yeah sorry, that need to be a comparison operater rather than an assignment one ;)

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

@Josh Waihi: See #319070: Provide a wrapper for cross-db and cross-field-type concatenation (PosgreSQL) to fix the PostgreSQL incompatibilities.

Josh Waihi’s picture

Josh Waihi CreditAttribution: Josh Waihi commented
Status: Needs review » Closed (duplicate)

@Dave Reid, lets resume this issue over there.

DeFr’s picture

DeFr CreditAttribution: DeFr commented
Status: Closed (duplicate) » Needs review

@Josh Waihi: Before your patch in #24, the patches in this issue wasn't PostgreSQL specific at all, and not related to bulk update either. Thus, back to need review for the patch in #14 and its test in #21.

I guess an improved issue title might be in order to prevent confusion, but it could get long pretty quickly. Something like "PathAuto only checks current alias when deciding to create a new one" would describe the patch in #14 quite well.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

Hi DeFr,
thanks a lot for your help! I tried to understand the test you wrote and wanted to start writing another test for testing the "unintended alias removal" bug (see http://drupal.org/node/388626) which I migrated into this patch earlier.

Unfortunately, I fail to setup simpletest for our system. Although I followed all installation instructions, simpletest even fails to run its own tests. Maybe it's a problem with drupal 6.15 because there is a bug report of someone else having problem with 6.15 and simpletest failing on its own tests.

I'll try to get it solved somehow so that I can test and extend your simpletest.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

The current code fetches the URL alias that is considered the 'current' alias for that path and language and will be used when linked in pages. I do see the point of wanting to handle mulitple existing aliases if the 'delete old alias(es)' or 'redirect from old alias(es)' update actions are selected.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Title: pathauto tries to recreate existing alias » Use update action for all existing aliases, not just the current existing alias

Changing title to reflect the current direction of the issue.

Frank Steiner’s picture

Frank Steiner CreditAttribution: Frank Steiner commented

> I do see the point of wanting to handle mulitple existing aliases if the 'delete old alias(es)' or 'redirect from old alias(es)' update actions are selected.

Not only that, but you also don't want to recreate existing aliases. We upgraded to pathauto-6.x-1.5.tar.gz after a long time and the behaviour has changed in that we do no longer get an error message but just the same alias again in a situation like this:
- Setting is "Create a new alias. Leave the existing alias functioning."
- node pattern is [menupath-raw]
Now create a new node with menu title "test1". Alias will be "/test1". Re-edit the node and change the menu title to "test2". The new primary alias shown after saving and when clicking on the node in the menu will be "/test2". However, "test1" still exists due to the setting "...leave the existing alias functioning".
Now re-edit again and change the menu title back to "test1", and after saving we have three aliases "test1", "test2" and "test1".

Happens also when we add an additional alias manually which becomes the first in the list. On the next save ("Automatic alias" is always checked) the existing automatically created alias is re-created again. Now only once because it succeeds instead of failing over and over again. But still not the correct solution. Unfortunately I'm not using drupal 7, so I cannot provide a port from the #14 patch for D7 :-(

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Category: bug » feature

Re-classifying as a feature request.