text:
Join as a member. There's a certificate with your name on it.
Notes: This is not an issue for D7 any longer (see post #299 for more info). A -perhaps- related issue for pathauto 7.x is #1236030: interface language change the path of the node.
The original problem description mentioned the error "user warning: Duplicate entry '...' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES (...)". This warning has disappeared since Drupal 6.16**. Comments up to #52 are not valid any more.
The cause of this warning, outlined in #53 along with the first patch, also caused aliases to disappear in other nodes (with same path, but different language) -- as outlined in #103. This is what the issue is mainly about, at the moment.
After various incarnations of patches:
- pfournier posted a new patch in #188 which was closely examined (by various people, a.o. roderik).
- roderik expanded on his patch according to some new info I found, which resulted in the patch in #275 after close scrutiny by pfournier.
(copied from #302)
We can consider the patch well-researched.
The only 'unresolved' discussion is what the goal of the patch should be:
- only fixing the bug while taking care that as much behavior as possible is retained <- roderik's stance; patch in #275
- doing away with some illogical behavior at the same time. <- pfournier's stance; would need a little change to #275, basically scrapping an UPDATE statement.
This needs comment from the D6 core maintainer.
The difference is only in 'theoretical' edge cases; it doesn't have any effect on code in Drupal core or pathauto (or any known/reported contrib module).
(The mentioned "illogical behavior" was specified at some time, but the spec is clearly outdated.)
(** read #1106030: Node edit form risks deleting another node's path alias for the reason why the original error has disappeared.)
Comment #1greggles CreditAttribution: greggles commented
|» Postponed (maintainer needs more info)
Do you have any steps for how to repeat this?
Comment #3greggles CreditAttribution: greggles commented
Well, I've never seen this and nobody else has reported it. For now it seems to be specific to your site. It seems quite likely that it's a bug in Pathauto, but I don't know how to repeat it so I can't debug/fix it...
Comment #4Freso CreditAttribution: Freso commented
@Steve Dondley: What other modules are you using?
Comment #5xanga2008 CreditAttribution: xanga2008 commented
Im also facing the similar issue.
it started after i have installed the latest category module.
user warning: Duplicate entry 'node/-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('taxonomy/term/30', 'node/', '')
Is this is a critical bug, should i uninstall category module?
Comment #6greggles CreditAttribution: greggles commented
|» Category
|» 6.x-2.0-alpha1
|Postponed (maintainer needs more info)
|» Active
it started after i have installed the latest category module.
That explains it a bit...
Comment #7keesje CreditAttribution: keesje commented
|» Drupal core
|» 6.3
|» path.module
I have the same issue (I believe).
user warning: Duplicate entry 'title/feed-nl' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/7/feed', 'title/feed', 'nl') in absolutepath/modules/path/path.module on line 112.
In my opinion it is a combination of pathauto and the in core content translation modules.
I don't have categories module installed.
In my case the system tries to generate duplicate aliasses for content submissions (and feeds) with different languages and the same node title.
In pathauto I do not have language specific settings.
At this point I am not sure, it could be 'old' aliasses not deleted from nodes that have a language change, title change or deleted content aliases.
I've seen 3 or 4 of these messages, I'l try to report here if this happens more often.
Comment #8keesje CreditAttribution: keesje commented
|» translation.module
If someone has a better clue wich project/component is responsible, please shift.
Comment #9John.Mulder CreditAttribution: John.Mulder commented
I'm having exactly the same issue here. Working local with the taxonomy module. The error occurs when I save an edit of an entry in a vocabulary. Despite the warning, the entry is updated and saved however. Below te warning I get:
user warning: Duplicate entry 'taxonomy/term/7-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('taxonomy/term/', 'taxonomy/term/7', '') in G:\Server\xampp\htdocs\private\drupal\modules\path\path.module on line 112.
Comment #10greggles CreditAttribution: greggles commented
|Drupal core
|» Category
|» 6.x-2.0-alpha3
|» Wrapper modules
@qrios, @john.mulder - do you use the Category module?
This is an issue with the Category module, afaik.
Comment #11John.Mulder CreditAttribution: John.Mulder commented
No, I'm not using the Category module. My feel is that there's a flaw with the check on whether an entry already exists, as the path.module obviously tries to enter something in the db that already is there.
Tnx for your contribution.
Comment #12greggles CreditAttribution: greggles commented
|» Pathauto
|» 6.x-1.x-dev
|Wrapper modules
|» Code
|» Postponed (maintainer needs more info)
Great - thanks for the quick response. So...let's bring this back to Pathauto then ;)
@John.Mulder - Which version of Pathauto are you using and what are the steps you take to get this error to happen?
Comment #14John.Mulder CreditAttribution: John.Mulder commented
Hi Greggles,
I'm using the latest version: 6.3. I just discovered that the warning does NOT occur if I uncheck the 'Automatic alias' checkbox which I find quite near the bottom of any page for editing a node. It is definitely a path module issue. I have just made some changes in a form created using the latest version of the webform module. It is exactly the same error as with the previously mentioned Taxonomy edit.
So basically this is my situation: I have url rewrite enabled and thus the path module so as to have nicely readable url's; they make me happy ;)
For any content I am editing (except when editing a blog entry or a story entry) I get the mentioned error/warning upon saving/submitting the edit. The only way to avoid it, is by unchecking the checkbox 'Automatic alias' and then submitting the changes. @steve: can you confirm this?
Tnx for your help.
Comment #17greggles CreditAttribution: greggles commented
Ok - thanks for the additional information. I still haven't seen this bug and, as Steve Dondley says, it's not consistent across his sites. If you do find more information on what is causing this please let me know.
One other question (though it may not matter) are you sure you've you're running the latest version of token and that run update.php since updating Token/Pathauto?
Comment #18keesje CreditAttribution: keesje commented
@qrios, @john.mulder - do you use the Category module?
No, Im not.
Comment #20John.Mulder CreditAttribution: John.Mulder commented
Everything up to date here also.
Token 6.x-1.10
Webform 6.x-2.1.2
And again: not using the category module.
I may have found part of the cause. In the path module under Automated Alias Settings, there's a text area filled with small words (like in, with, and etc.) that are supposed to be filtered from a url-alias.
When editing a post, one seems to be able to fill in any of those words again, by unchecking the earlier mentioned Automatic alias checkbox . In my case: /getting-in-touch would automatically become /getting-touch when left to the automatic aliasing mechanism. This is somewhat awkward, so I changed it back to /getting-in-touch manually. At that point there seems to be a conflict with the Automated Alias Settings of the path module, as soon as one edits a node again and submitting the changes with the Automatic alias checkbox checked.
Hmm, quite a story huh ;) Hope this still makes some sense. Again: it is definitely a bug, but not very major. My guess is it's a flaw in the logic behind the path module: user and system are allowed to perform contradictory actions. That would confuse me too ;)
Comment #22nonsie CreditAttribution: nonsie commented
I haven't encountered this error until the current site I'm working on.
One thing I've noticed is that when a translation is created and the alias is exactly the same as for the original node the alias for translated node is not changed to alias-0. So after translating the node original node and translated node will end up having the same alias triggering an error each time original or translated node is updated. I'm pretty sure the problem lies in the combination of content translation and pathauto but not quite sure where.
Comment #23greggles CreditAttribution: greggles commented
Is everyone else having this problem using content translation module?
Comment #25Freso CreditAttribution: Freso commented
@Steve Dondley (and everyone else answering #23):
How about the Locale module?
Comment #27keesje CreditAttribution: keesje commented
"Is everyone else havig this problem using content translation module?"
Yes I am. See post #7, I think it's related to translated content with same node title, since that's gonna be my URL alias. E.g. "Contact" is a common node title for the contact page in many languages. But looking at the other posts, this is not the only scenario triggering this error.
More specific:
I use locale.module and parts of i18n.module for block- and string translation only.
Comment #28nonsie CreditAttribution: nonsie commented
I think I'm starting to get it - looking at query log and it's core path module that's causing the issue.
I have pathauto set up to use titles for aliases, I've created a node titled foobar in English and translated it into Spanish. I can create both nodes without any errors.
However I'm getting an error on node update:
UPDATE url_alias SET src = 'node/359', dst = 'foobar', language = 'en' WHERE dst = 'foobar'
Looking at path_set_alias() function it does not account for same alias in different languages, dst is not an unique field - if both dst and src are set the following part of the function is executed:
if ($alias == drupal_get_path_alias($path, $language)) {
// There is already such an alias, neutral or in this language.
// Update the alias based on alias; setting the language if not yet done.
db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
In my case I have two aliases named 'foobar' in two different languages causing the update to error.
Any ideas how to fix this?
Comment #29Freso CreditAttribution: Freso commented
|mysql errors when saving a page
|» path_set_alias() doesn't account for same alias in different languages
|» Drupal core
|» 6.x-dev
|» path.module
|Postponed (maintainer needs more info)
|» Active
path_set_alias() obviously needs to take account of the language when setting the alias. And this needs to happen in core, as the problem (if I understand correctly from the above mentions) should be reproducible on non-Pathauto setups as well. (Do move back if that is not the case though.)
I'm thinking that y'all might not be having the same issue, even if the symptom is the same. If core's path.module can be fixed (if it is indeed broken) and make one of the issues go away, we'll hopefully be better able to "treat" the rest. :)
Comment #30nonsie CreditAttribution: nonsie commented
|» Needs review
Attached patch fixes part of the problem - setting correct language alias for nodes. Still looking into taxonomy duplicate alias issue.
Comment #31scottrigby CreditAttribution: scottrigby commented
I'm seeing the same error in a D6.4 installation:
user warning: Duplicate entry 'adverts/faq-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/6', 'adverts/faq', '') in /home/archiveo/public_html/modules/path/path.module on line 112.
My pathauto's Automated Alias Settings
* General Settings > Update Actions: 'Create a new alias. Leave the existing alias functioning.'
* Node Path Settings > Pattern for all Image paths: 'adverts/[title-raw]'
What's happening in my case
* On certain nodes of that type (in this case the type is image) I had already set the path alias to be different (this is because there are a few images on our site that are not 'adverts' but the rest are... hence adding that path logic mentioned above.
* But based on the error, it seems that when I edit these nodes, pathauto adds a new alias based on the logic, while also keeping the old one.
Module info:
* I don't have Category module installed, or core Taxonomy module enabled.
* using pathauto-6.x-1.1
* token-6.x-1.11
* cck-6.x-2.0-rc4
* globalredirect-6.x-1.0
* stringoverrides-6.x-1.6
* xmlsitemap-6.x-0.x-dev
(I have other mods installed like image, etc, but i'm not sure how many of these are relevant)
Hope this provides more info?
:) Scott
Comment #32nonsie CreditAttribution: nonsie commented
does the patch in #30 remove the error for nodes?
Comment #33scottrigby CreditAttribution: scottrigby commented
hi nonsie,
I tried to apply the patch, but it failed. So I looked to see if the core path module changed between D6.3 and 6.4. – and apparently it did...
Comment #34Freso CreditAttribution: Freso commented
Edit: Err, wait. Hang on. Sorry. That was for another issue. Please disregard me. :$ Is it an okay excuse that it's 10 to 1 AM? >_>
Comment #35nonsie CreditAttribution: nonsie commented
hmm, I don't think there were any changes in path.module in 6.4 (at least they are not in the patch file)
Comment #36scottrigby CreditAttribution: scottrigby commented
omigosh. Ok, apparently I must have been hallucinating from sleep dep...
nonsie you are so right about that (the mod did NOT change between 6.3 & 6.4...) – what I did was a diff on a D5 installation by mistake!
Ok, so I applied the patch (which DID successfully patch), and I replaced the core path module with the patched one...
However, I still get the errors, like:
* user warning: Duplicate entry 'reviews/put-your-crazy-pants-lets-party-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/28', 'reviews/put-your-crazy-pants-lets-party', '') in /home/archiveo/public_html/modules/path/path.module on line 117.
* user warning: Duplicate entry 'reviews/put-your-crazy-pants-lets-party/feed-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/28/feed', 'reviews/put-your-crazy-pants-lets-party/feed', '') in /home/archiveo/public_html/modules/path/path.module on line 117.
Do you think it's because there are already dupes in the table? If so, how should I go about getting rid of these?
Otherwise I'm not sure how to test, because I still get errors even after the patch...
One other thought... should I run the update script now that the module has been modified?
Comment #37wdrupal100 CreditAttribution: wdrupal100 commented
the solution for me was to look inside the url_alias table. i found the duplicate src entries and verified they werent being listed in the admin > site building > url aliases page. i removed the invalid aliases from the table manually. For me this is only an issue on some pages so I have just been doing these steps as the errors occur.
also i had previously enabled language support but then disabled it. i had to then go and clear all the language settings from the url_alias table
Comment #38keesje CreditAttribution: keesje commented
"One other thought... should I run the update script now that the module has been modified?"
Cant hurt. Basically it executes update code in the install script of an updated module if available, and clears cache.
Since the patch doesn't effect the install script it should not be necessary.
Comment #39Alauddin CreditAttribution: Alauddin commented
I just changed my url alias setting to get rid to this error.
Admin > Site building > Url Alias > Automated Alias Setting > General Setting
under 'update action'
select : Do nothing. Leave the old alias intact.
The only reason I had it on - Create a new alias. Leave the existing alias functioning - was because I am working on a new site and wanted to have the urls update.
In general I think it is BEST to leave setting at : Do nothing, leave the old alias intact
for these 2 MAIN reasons
1) no broken links. If your page was indexed and you now 'updated' the url - users get 404 error on the old url indexed by SE
2) all your hard work to 'optimize' pages and have them indexed by SE's goes to waste if you just 'update' the url.
ps: if you do must update urls - then consider '.htaccess' 302 redirects of old urls to new to keep from having 404 errors
Comment #40nonsie CreditAttribution: nonsie commented
Setting update action to "Do nothing" doesn't solve the issue itself - you still won't be able to have same alias in multiple languages.
Comment #41earnie CreditAttribution: earnie commented
|Needs review
|» Needs work
Wouldn't the correct fix for
Duplicate entry 'reviews/put-your-crazy-pants-lets-party-' for key 2 query
be an addition to the dst key of the language column? But system_update_6005() is supposed to do that already. Did your update add create a new dst unique index with the language column like it should have?
Also, I notice that
VALUES ('node/28', 'reviews/put-your-crazy-pants-lets-party', '')
from #36 has an empty language column.
I agree that the correct action for pathauto users is as Alauddin explains in #39.
The patch needs work because it only needs to exchange the update with
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $alias, $language);
It doesn't need to test if $language is set.
You probably should roll the patch for D7 first and then it can be back ported to D6
Comment #42nonsie CreditAttribution: nonsie commented
system_update_6005 did create new unique index - UNIQUE KEY `dst_language` (`dst`,`language`).
The patch also needs to check if $language exists. For example let's say a user created a node in English and saved it. Later, the user went back and changed the node language setting to no language - in that case your update would fail because $language is not set yet in the database it's set to en.
Comment #43earnie CreditAttribution: earnie commented
The patch also needs to check if $language exists. For example let's say a user created a node in English and saved it. Later, the user went back and changed the node language setting to no language - in that case your update would fail because $language is not set yet in the database it's set to en.
The example you give should never exist. The alias doesn't exist so an insert would happen. If this code is executed without a record being available to update; then we have a bug somewhere else. There is no need to filter the update based on $language.
Comment #44nonsie CreditAttribution: nonsie commented
Earnie, you are right - an insert would need to happen rather than update however at the moment it doesn't.
I'm not quite sure what's the policy re keeping old aliases eg in the example I provided you would have one alias with language set to en and one set to blank rather than updating the old en language alias?
Comment #45earnie CreditAttribution: earnie commented
IMO, an alias of $language = '' and one of $language = 'en' are different aliases even though the result is the same. One might argue that $language = '' is assuming a default and it should be set to the specified default before saving to the db.
if (empty($language)) {
$language = variable_get('default_language', 'en');
Comment #46nonsie CreditAttribution: nonsie commented
No language is not the same as default language - if it was it would require major rewrite of all modules that implement that.
So we have established that path module should create a new alias record if alias/language is changed, which means that the problem is in drupal_get_path_alias
Comment #47earnie CreditAttribution: earnie commented
I don't think so. If a user creates an alias with a language set and then set no language the appropriate action is a warning that the language dependent data will not be active. It is then up to the user to update the nodes for which she wants aliases for the no language setting. A contrib module in the development package could be added to translate the language specific items to no language items. It shouldn't be up to Drupal core to clean up the mess.
However, your patch points out a defect in the update because the language key should be specified in the WHERE clause.
Comment #48Manuel Garcia CreditAttribution: Manuel Garcia commented
Also having this issue, subscribing
Comment #49kesmeby CreditAttribution: kesmeby commented
This is the error I'm getting after saving content with Path Auto enabled:
user warning: Duplicate entry '/feed-' for key 2 query: UPDATE url_alias SET src = 'node/1/feed', dst = '/feed', language = '' WHERE pid = 5 in /home/kyle/public_html/chsfsalpha/modules/path/path.module on line 100.
For some reason PathAuto is creating a duplicate of each node with "/feed" appended to the end. I'm not using any language features or trying to publish RSS of the site.
Comment #50greggles CreditAttribution: greggles commented
@ksemeby - the symptom is similar, but the underlying problem you have is different. Please create a new issue in the Pathauto issue queue about your problem.
Comment #51Maciej Lukianski CreditAttribution: Maciej Lukianski commented
Some additional info on the bug:
I have 'Automated alias settings' on my site enabled and 'default Taxonomy term path settings' set as:[catpath-raw]
I get the same 'Duplicate entry..' message when I change order of terms in Content Management>Taxonomy by draging them up and down.
I also fund other occurence of the same bug in a differend thread - this time after upadating simplenews.module: http://drupal.org/node/228792#comment-1119871
Hope this might give someone some help in sorting this out
Comment #52davexd CreditAttribution: davexd commented
@kesmeby - I'm not sure if if its a different issue or not, but I had EXACTLY the same as you (anyone feel free to direct me somewhere else)
How I fixed it: go to your site: admin/build/path/pathauto
Under: Node Path Settings make sure the "Internal feed alias text (leave blank to disable):" field is blank.
It fixed it for me :)
Comment #53neochief CreditAttribution: neochief commented
Finally, I have found the reason of this bug and fixed it. Here's my explanation.
In latest D6.8 there's a key in url_alias table called dst_language on (dst, language) columns. Let's imagine that we have pathauto set to "Create a new alias. Leave the existing alias functioning." Also, imagine that we already have such data in url_alias:
pid src dst lng
123 node/10 abc ru
124 node/10 bcd ru
Let's try now to set alias "bcd" again to node #10 [I draw a little schema]:
path.module, line 103
So, to kill this bug, we should actualy remove chances to such situation and delete possible dst/language pair by
db_query("DELETE FROM {url_alias} WHERE dst = '%s' AND language = '%s'", $alias, $language);
just before that INSERT.
Also, included nonsie's patch, because it fixes same issues with UPDATEs.
Comment #54Jo Wouters CreditAttribution: Jo Wouters commented
|Needs work
|» Needs review
I can confirm the issue; and was able to repeat it.
I'm using this patch now, and it solved the issue.
Comment #55earnie CreditAttribution: earnie commented
|Needs review
|» Reviewed & tested by the community
per #54 this is tested and the patch code looks good.
Comment #56Damien Tournoud CreditAttribution: Damien Tournoud commented
|Reviewed & tested by the community
|» Needs work
if ($alias == drupal_get_path_alias($path, $language)) {
// There is already such an alias, neutral or in this language.
// Update the alias based on alias; setting the language if not yet done.
- db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ if($language) {
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $alias, $language);
+ }
+ else {
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ }
Hum, so if $language is empty or "0", we empty the language of all aliases *and* change all aliases to have a source of $path? That sounds like non-sense AND could create duplicate key issues :)
I believe the correct fix is just to add a
AND language = '%s' condition to the existing UPDATE query.
else {
+ // Ensure that there is no such aliases created before
+ db_query("DELETE FROM {url_alias} WHERE dst = '%s' AND language = '%s'", $alias, $language);
// A new alias. Add it to the database.
db_query("INSERT INTO {url_alias} (src, dst, language) VALUES ('%s', '%s', '%s')", $path, $alias, $language);
This change makes zero sense to me, even after reading the explanation in #53.
Comment #57Jo Wouters CreditAttribution: Jo Wouters commented
Updating $language on all $dst when $language is empty is indeed not a good idea.
The code is working in my tests; but it might indeed overwrite too much data.
So you propose to only change:
if ($alias == drupal_get_path_alias($path, $language)) {
// There is already such an alias, neutral or in this language.
// Update the alias based on alias; setting the language if not yet done.
- db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ if($language) {
+ // Update only the alias defined for this language
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $alias, $language);
+ }
+ else {
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ }
Comment #58Damien Tournoud CreditAttribution: Damien Tournoud commented
@Jo Wouters: the
if ($language) check is useless. The only thing missing is a condition on 'language' in the update query.
Comment #59Jo Wouters CreditAttribution: Jo Wouters commented
@Damien Tournoud
In the code comments I read:
... setting the language if not yet done.
If we always do:
db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s' AND language = '%s' ", $path, $alias, $language, $alias, $language);
then we'll never set the language :-)
I don't know this part of core good enough to know when the alias is set without the language being set, but looking at the comments I think we still need the $language test :-)
Comment #60nonsie CreditAttribution: nonsie commented
I agree with comment in #59.
I've been using my own patch from #30 since I posted it here without any problems.
Comment #61Damien Tournoud CreditAttribution: Damien Tournoud commented
@Jo Wouters: I think the comment is a left over of something. Because there can only be one entry per (dst, language), it makes strictly no sense to match only on the "dst" column.
Think about it one second: what does this do (from the original patch in #30):
// There is already such an alias, neutral or in this language.
// Update the alias based on alias; setting the language if not yet done.
- db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ if($language) {
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $alias, $language);
+ }
+ else {
+ db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ }
- If $language is set, we only modify the alias for that language (so far so good).
- If $language is not set, we *remove* the language for all aliases (?!?).
For those reasons, I'm for the change in #57 (please remove the redundant $alias and $language: we don't need to update them if we matched on them), plus removing the comment about the language.
Comment #62Jo Wouters CreditAttribution: Jo Wouters commented
|Needs work
|» Needs review
|755 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path_module_269877.patch. View
Thanks Damien !
So it looks like this is enough, indeed:
// Check for existing aliases.
if ($alias == drupal_get_path_alias($path, $language)) {
// There is already such an alias, neutral or in this language.
- // Update the alias based on alias; setting the language if not yet done.
- db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ db_query("UPDATE {url_alias} SET src = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $language);
else {
// A new alias. Add it to the database.
Patch can be found in attachment.
Comment #63neochief CreditAttribution: neochief commented
How about my changes in #53?
Comment #64Damien Tournoud CreditAttribution: Damien Tournoud commented
@Jo Wouters: this is starting to looks great.
@neochief: it's unclear to me why your change can ever be needed. Please elaborate.
Comment #65neochief CreditAttribution: neochief commented
Because in latest Drupal 6.8 with latest pathauto there is no possibility to use "Create a new alias. Leave the existing alias functioning." feature without my patch. Please, take a look at my explanation one more time.
Comment #66Damien Tournoud CreditAttribution: Damien Tournoud commented
@neochief, may be that's a bug in pathauto, then :) Please elaborate on why the change is needed.
Comment #67neochief CreditAttribution: neochief commented
Damian, how could I be more accurate with explanation? I've already provided a full use-case to reproduce this bug. Here's one more time. I'm talking about duplicates error in this code. Jo's solution is not full. It's not pathauto's bug, it's a bug of core path_set_alias().
Comment #68Damien Tournoud CreditAttribution: Damien Tournoud commented
|Needs review
|» Needs work
@neochief: ok you are right, this is still not quite right yet.
The check
if ($alias == drupal_get_path_alias($path, $language)) is in fact the culprit. This should be
if (drupal_lookup_path('source', $alias, $language)), because what we need to check is that there is not dst/language couple already existing.
Please roll a patch for this.
Comment #69neochief CreditAttribution: neochief commented
|960 bytes
After digging deeper into the problem, I found that this is not everything we should change. But I moved that part into a separate thread (#358315: drupal_lookup_path() not respects alias' order). Please, take a look my explanation there.
If we'll fix that issue, your
if (drupal_lookup_path('source', $alias, $language)) will not allow to set old aliases active. For example, you have such url_alias:
pid src dst lng
123 node/10 abc ru <- oldest alias (this will be active in Drupal 6.8 and older)
124 node/10 bcd ru <- old alias
125 node/10 efg ru <- active alias [with my fix]
and trying to save a node with "bcd" alias. With your solution, we will only update old alias and will not make it active. With my solution (#53) we will have such url_alias on exit:
pid src dst lng
123 node/10 abc ru
125 node/10 efg ru
126 node/10 bcd ru <- active alias
Anyway, I'm attaching both version of patches — mine and Damien's (both with Jo's changes included).
Comment #70two2the8 CreditAttribution: two2the8 commented
Hey there,
I see the same user warning whenever I edit & save an existing taxonomy term or node:
user warning: Duplicate entry 'blog/username/truncated-title-of-post' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/30157', 'blog/username/full-title-of-post', '') in /var/www/DRUPAL-6.8/modules/path/path.module on line 112.
Taxonomy Terms:
user warning: Duplicate entry 'taxonomy/term/4910-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('taxonomy/term/', 'taxonomy/term/4910', '') in /var/www/DRUPAL-6.8/modules/path/path.module on line 112.
I'm using pathauto with the settings that neochief describes above.
I'm eager to fix the problem, so I'll try one of the patches mentioned above, though I confess I'm not clear on the differences between the two patches in #69. The approach described by neochief in #69 seems to be most comprehensive, but I'm a bit confused about whether it depends on another problem (http://drupal.org/node/358315) being solved elsewhere before it will work. Can anyone clarify?
Also, this thread deals with a seemingly similar problem being discussed here: http://drupal.org/node/228792. I'm not smart enough to tell if it's the same issue, but it might be worth someone taking a look.
Comment #71neochief CreditAttribution: neochief commented
Your duplicate errors will gone with any of (#69) patches. As for my patches in [#358315: drupal_lookup_path() not respects alias' order], they are fixing another bugs, please see that issue for more info.
Comment #72neochief CreditAttribution: neochief commented
|Needs work
|» Needs review
Comment #73two2the8 CreditAttribution: two2the8 commented
Thanks for clearing that up, neochief, and for your work on this issue.
I applied your latest patch, and so far it seems to eliminate the error for me.
Comment #74solutiondrop CreditAttribution: solutiondrop commented
Thanks neochief. The patch(s) cleared up the problem for me. I have noticed anything wrong in the last day.
Comment #75Encarte CreditAttribution: Encarte commented
Comment #76momper CreditAttribution: momper commented
i still get errors like this in drupal 6.9 - isn't it fixed with an update - or will it be fixed in drupal 6.xx?
Comment #77earnie CreditAttribution: earnie commented
Those of you testing the patches need to change the status if you are satisfied with the patch from ``patch (code needs review)'' to ``patch (reviewed & tested by community)''. Until that happens then this will continue to be an issue. Also, you need to say which of the patches in #69 you've tested and want committed.
Comment #78neochief CreditAttribution: neochief commented
|» 7.x-dev
As the progress on #358315: drupal_lookup_path() not respects alias' order is moving on, we should stop on my solution from #69. Here's patches for D6-7. (Patch for 6.x is working fine for me about a couple of month so far)
Comment #80neochief CreditAttribution: neochief commented
Damn, I hate Testbed and it seems that it hates me too :) Patch is applying cleanly to D7 HEAD, any ideas why it failed?
Comment #81ainigma32 CreditAttribution: ainigma32 commented
any ideas why it failed
Have you tried creating diff using Drupal's root as the diff's root?
- Arie
Comment #82neochief CreditAttribution: neochief commented
|Needs work
|» Needs review
Failed: Invalid PHP syntax in path.module . View
Ah, yes, Arie, your totally right! Here are new ones.
Comment #84greggles CreditAttribution: greggles commented
I'm not sure what parts of the patch are causing the PHP syntax errors.
As a quick nit-pick review -
// Ensure that there is no such aliases created before lacks a period on the end and doesn't seem natural to me. How about "// Ensure that no similar aliases exist."
Also, perhaps use -D6.patch instead of _D6.patch
Comment #85neochief CreditAttribution: neochief commented
|Needs work
|» Needs review
|935 bytes
Invalid patch format in path-module-D7.patch. View
|937 bytes
Comment #86neochief CreditAttribution: neochief commented
|Needs review
|» Reviewed & tested by the community
Great! I think it can be pushed to RTBC now, doesn't it?
Comment #87neochief CreditAttribution: neochief commented
|» Critical
Comment #88webchick CreditAttribution: webchick commented
|Reviewed & tested by the community
|» Needs work
Sorry, but given that it took 80 replies to track down this bug and around 40 to come up with a set of steps to reproduce it, we really need to write some tests to ensure this doesn't bite us again.
+ // Ensure that no similar aliases exist.
+ db_query("DELETE FROM {url_alias} WHERE dst = '%s' AND language = '%s'", $alias, $language);
Can you explain in this comment why such a condition might ever happen?
Comment #89neochief CreditAttribution: neochief commented
Comment #90webchick CreditAttribution: webchick commented
Yes. I want it in the source code, not in the issue queue. :)
Comment #91neochief CreditAttribution: neochief commented
|Needs work
|» Needs review
|path-module-D7_with tests.patch
FAILED: [[SimpleTest]]: [MySQL] Fetch test file: failed to retrieve [path-module-D7_with tests.patch] from project client. View
Here's a new patch. I also fixed wrong double-alias validation in URL administration (yes, spreading the context, but this directly related to the current issue). Tests for everything included. Without patching the module, new test fails, and passing with it, so I guess verything is fine :)
Comment #92neochief CreditAttribution: neochief commented
Hey, could comeone from these 90 comments finally review this patch and mark it as RTBC? :)
Comment #93Damien Tournoud CreditAttribution: Damien Tournoud commented
|Needs review
|» Needs work
I need to look at this a little bit more, but is this chunk of code really doing anything?
if ($alias == drupal_get_path_alias($path, $language)) {
// There is already such an alias, neutral or in this language.
db_query("UPDATE {url_alias} SET src = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $language);
If $alias == drupal_get_path_alias($path, $language), it means that we have a ($path, $alias, $language) entry already, so the UPDATE is probably a no-operation.
Comment #94earnie CreditAttribution: earnie commented
Re: #93:
But the dst column is changing. Meaning we have an alias for an old node and now we have a new node with the same alias.
Comment #95Damien Tournoud CreditAttribution: Damien Tournoud commented
But the dst column is changing. Meaning we have an alias for an old node and now we have a new node with the same alias.
@earnie: dst is $alias, src is $path and we do:
if ($alias == drupal_get_path_alias($path, $language)) {
Am I missing something?
Comment #96earnie CreditAttribution: earnie commented
Sorry, I meant the src column is changing to update an existing dst for a new src.
Comment #97telemako CreditAttribution: telemako commented
Hello, I have a problem related to this issue too.
I posted this:
Look at it for some other detaisl
Anyway I get the warning:
user warning: Duplicate entry 'content/rr16-it' for key 'dst_language' query: UPDATE url_alias SET src = 'node/70', dst = 'content/rr16', language = 'it' WHERE dst = 'content/rr16' in C:\wamp\www\tcwebsite\modules\path\path.module on line 108.
when I translate a node that has taxonomy filed, It doesn't happen on translation of content of other type.
I think is related to pathauto, taxonomy.
Let me know if is there a solution.
Comment #98telemako CreditAttribution: telemako commented
Hmmm... it seems like on translate from italian to english the language is not passed and the module think it should create another alias for the italian language and not for the english one.
Comment #99neochief CreditAttribution: neochief commented
telemako, did you tried #85?
Comment #100neochief CreditAttribution: neochief commented
@Jo Wouters, as you're the author of stucking line from the patch, could you please explain to Damien what is it for? I would be very appreciated if we push the progress up, cause everything is almost done.
Comment #101telemako CreditAttribution: telemako commented
hmm.. seems interesting.
Only one problem, I'm newbe enough to ask how to apply the patch. ^^'
Where can find the correct way? Or some tips here?
Comment #102neochief CreditAttribution: neochief commented
Comment #103GiorgosK CreditAttribution: GiorgosK commented
tested #85 for drupal 6 and its not solving the following case
create a language neutral page with url "abcd"
create another language specific page with url "abcd"
the two pages now have the same exact URL and
only the second page can be accessed at the url "abcd"
isn't this part of the same problem ?
Comment #104hargobind CreditAttribution: hargobind commented
Thought you'd want to know that I applied patch #85 for 6.11 and it fixed the errors I was experiencing. Thank you all!
Here was my scenario:
Drupal 6.11
Automated Alias Settings, Update Action is set to "Create a new alias. Leave the existing alias functioning."
I created a node and initially unchecked "Automatic Alias" and specified my own path. Then when I went back and edited the node, "Automatic Alias" checked itself and I left it that way, and then clicking Save gave me the error that was initially reported.
Comment #105nonsie CreditAttribution: nonsie commented
Patch in #85 on 6.11 fixed my errors as well. I also had to fix updating an existing alias. I changed it from
db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE pid = %d", $path, $alias, $language, $pid);
db_query("UPDATE {url_alias} SET src = '%s', dst = '%s' WHERE pid = '%s' AND language = '%s'", $path, $alias, $pid, $language);
which seems to do the trick
Comment #106mathieu CreditAttribution: mathieu commented
Subscribing - having similar issue.
Comment #107neochief CreditAttribution: neochief commented
I can state that the patch for D7 isn't working now, as database layer got major changes from that time.
Comment #108mvc CreditAttribution: mvc commented
Patch in #85 on 6.11 fixed my problem (without the modification mentioned in #105). Thanks, neochief!
Comment #109asb CreditAttribution: asb commented
Comment #110nonsie CreditAttribution: nonsie commented
Could we possibly get patch from #85 rolled into the next D6 version? I end up using this patch every single time I set up a multilingual site.
Comment #111RikiB CreditAttribution: RikiB commented
#85 fixed me up :)
Comment #112Lars Pohlmann CreditAttribution: Lars Pohlmann commented
We just came across this bug. The Bug (and the fix) is known for more than a year. To be honest, I don't understand why this hasn't been resolved yet in Drupal 6...
Comment #113jct CreditAttribution: jct commented
|Needs work
|» Reviewed & tested by the community
#85 worked for me as well in D 6.13
Not sure if this is the right thing to do, but the patch appears to have been "reviewed and tested," no?
Comment #115neochief CreditAttribution: neochief commented
this is 7.x issue queue. After it will be fixed in D7, we can backport and commit it to D6
Comment #116asb CreditAttribution: asb commented
> this is 7.x issue queue. After it will be fixed in D7, we can backport and commit it to D6
About one year ago, in #29, this became an D6 issue [#http://drupal.org/node/269877#comment-959710], and you changed it a few months ago (in #78) to an D7 issue. I don't know much about internal Drupal coding and bug fixing politics, but as a user I'd simply appreciate a pragmatic solution for a nasty bug in the software I'm using - which is D5 and D6, and definitely not D7.
Thanks & greetings, -asb
Comment #117neochief CreditAttribution: neochief commented
Hey, I'm just telling you the commit politics. Unless someone will update my patch from 91 and commit it to 7x, patch will not be accepted to 6x and older.
Comment #118asb CreditAttribution: asb commented
I understand, damn! Please don't give up on this issue, and thank you very much for your hard work (this explains why we didn't get #444228: Optimize CSS option causes php cgi to segfault in pcre function "match" into core). Greetings, -asb
Comment #119rmmcclay CreditAttribution: rmmcclay commented
You're right, Steve. I encountered this error and it was easily repeated: create a node (page), first click on Preview and then Save. Would happen every time unless I unchecked the Automatic Alias checkbox.
This is a completely fresh install, modules and all. 5 weeks old.
Comment #120RikiB CreditAttribution: RikiB commented
Well, I upgraded to 6.14 and now its broke again. So I went back to #85 to manually fix it again and it doesn't seem to remove the errors anymore. Can anyone confirm a fix for Drupal 6.14? This is an extremely annoying bug.
Comment #121logicalpat CreditAttribution: logicalpat commented
I've been experiencing this same issue for a while now in Drupal 6.x and am updated to 6.14 and still experience it.
Comment #122GiorgosK CreditAttribution: GiorgosK commented
if you are looking for drupal 6 patch
test the d6 patch from comment #85
and report back (I check that it applies against 6.14 but have not tested)
Comment #123jseffel CreditAttribution: jseffel commented
The patch for comment #85 works with Drupal 6.14. Just applied the patch and tested it on a "problem node" and the error message is gone.
Comment #124RikiB CreditAttribution: RikiB commented
hmm, I just tripple checked and mine seems patched but Im still seeing the error. Strange.
Comment #125miro_dietiker CreditAttribution: miro_dietiker commented
Ran in to this very same issue... greetings to the issue tracker.
Comment #126harrrrrrr CreditAttribution: harrrrrrr commented
worked for me
Comment #127DamienMcKenna CreditAttribution: DamienMcKenna commented
The D6 patch from #85 resolved the issue for us too, we were seeing it in D6.13 when used in combination with Content Profile and PathAuto when someone was saving their profile.
Comment #128hanoii CreditAttribution: hanoii commented
subscribing, also actually looked at #85 and it seems fine, will test it soon with one site that's throwing the error each time I save a node with a translation.
Comment #129DamienMcKenna CreditAttribution: DamienMcKenna commented
What would it take to get this patch rolled into a 6.x release? What further testing is required?
Comment #130mathieu CreditAttribution: mathieu commented
|» 6.x-dev
I wonder how the "recent" commit of #332333: Add a real API to path.module changes this issue. There is no way that the last patch will apply to D7 without major modifications... and this is still an annoying issue in D6. Any chance this gets in as a bugfix? I believe this is fixed in D7 following the addition of the "real path api", so I'm taking the chance to mark this as a D6 issue.
Comment #131hanoii CreditAttribution: hanoii commented
I actually tried #85, and it worked, currently using it fine so far.
Comment #132GiorgosK CreditAttribution: GiorgosK commented
|Needs work
|» Reviewed & tested by the community
patch from #85 applies to drupal 6.14 and seems to get rid of the warning
should this be marked as "reviewed and tested by the community" ?
Comment #133NiklasBr CreditAttribution: NiklasBr commented
Comment #135mathieu CreditAttribution: mathieu commented
|Needs review
|» Reviewed & tested by the community
Re-submit of same file as in #85, to prevent testbot from changing status (is this how it works?) and hopefully moving this forward.
Comment #136jct CreditAttribution: jct commented
|Reviewed & tested by the community
|» Needs review
From what I can tell, the Status needs to be changed to "needs review."
I find it interesting that we're all figuring things out as we go here and hope that more experienced members will see it for what it is--communal learning :)
Comment #137jct CreditAttribution: jct commented
One more try w/ attachment
Comment #138mathieu CreditAttribution: mathieu commented
|Needs review
|» Reviewed & tested by the community
Looking at how the bot reacted after your comment in #113, it looks like the testbot reacts to RTBC.. but anyway, with this one being set to D6, the testbot shouldn't do much now.
@Gábor: are there any problems left preventing this from getting in?
Comment #139Gábor Hojtsy CreditAttribution: Gábor Hojtsy commented
|» 7.x-dev
|Reviewed & tested by the community
|» Needs work
Either we fix this problem for Drupal 7 in this issue or you point me to an issue where this was fixed for Drupal 7 and THEN we can proceed adding this fix in Drupal 7. Reasons include avoiding regressions in Drupal 7, getting advantage of test coverage in Drupal 7 for stuff getting into Drupal 6, and so on.
Moving back to Drupal 7 needs work for the first option I've outlined.
Comment #140sun.core CreditAttribution: sun.core commented
I don't recall such a change either, so we need someone to test whether this bug still exists in D7. If it does, we need to re-roll #91 against HEAD. In either case, we still want to commit a test to prevent us from breaking this again.
Critical as long as this bug could still exist.
Comment #141marcoBauli CreditAttribution: marcoBauli commented
same issue on different sites (6.x)
Comment #142dafeder CreditAttribution: dafeder commented
I had this issue in D6.14 - came up randomly, in my case apparently for just one node type. #85 has fixed it with no issues so far.
Comment #143catch CreditAttribution: catch commented
path_set_alias() no longer exists in D7. It's now http://api.drupal.org/api/function/path_save/7
It looks to me like drupal_write_record() ought to just handle this, so I'm moving this back to D6.
Comment #144nwe_44 CreditAttribution: nwe_44 commented
Comment #145catch CreditAttribution: catch commented
|» 6.x-dev
Really moving version this time. I checked through the calls to path_save() in core, and they either specify language, specify pid, or both.
Comment #146mathieu CreditAttribution: mathieu commented
Here's the same patch as in #85 (which is the same as later ones in #135 and #137, except for changes in file name), with some more comments, following on webchick's comments in #88 and #90.
If this is D6 only, are tests needed?
Comment #147klonos CreditAttribution: klonos commented
Comment #148DarrellDuane CreditAttribution: DarrellDuane commented
Comment #149mvc CreditAttribution: mvc commented
|Needs review
|» Reviewed & tested by the community
works for me. thanks!
Comment #150Mitson CreditAttribution: Mitson commented
The patch in #146 works for me (and for now).
This bug (description in top of page) is appeared after fresh install of D6.15 with some extra modules.
Many thanks!
Comment #151klonos CreditAttribution: klonos commented
Combination of the patch in #146 and the ones in #358315: drupal_lookup_path() not respects alias' order #77 solved my issues and saved my life!
Hope to see them two committed to the next 6.16 ;)
Comment #152Gábor Hojtsy CreditAttribution: Gábor Hojtsy commented
|Reviewed & tested by the community
|» Needs work
Looking at the first part of the patch, I'm seeing that drupal_get_path_alias() might return an alias which was set to be language neutral. So there might not be an existing alias you can update for this language. My understanding is that the alias will not be saved in that case. Am I missing something?
Comment #153doublewhopper CreditAttribution: doublewhopper commented
Thank you very much!
Comment #154ddanier CreditAttribution: ddanier commented
Comment #155b3liev3 CreditAttribution: b3liev3 commented
when you have the same alias for different languages you get a conflict.
I just write a query to get the pid of the alias I want to "update" and I put it in the path_set_aias sentence.
$pid = db_result(db_query("SELECT pid FROM {url_alias} WHERE dst = '%s' AND language = '%s'",$alias, 'en'));
path_set_alias('node/4/20', $path, $pid, 'en');
path_set_alias('node/4/20', $path, null, 'en');
Comment #156klonos CreditAttribution: klonos commented
@b3liev3: I use the same alias for different locales and DON'T get a conflict.
Do you use D6.16 that has the patch from post #77 in #358315: drupal_lookup_path() not respects alias' order merged?
Did you try applying the patch from #146?
Finally, what is your language negotiation method? (Prefix only or Prefix with fallback)
Comment #157b3liev3 CreditAttribution: b3liev3 commented
You are right @klonos the patches work and it's fixed in D6.16
The snippet was useful if you didn't use the patches though.
Well, thanks a lot for your time n attention :)
Comment #158klonos CreditAttribution: klonos commented
You're welcome ;)
Now, all there needs to be done I see this make it to the next D6.17 and the issue can then be marked fixed... with a note suggesting to upgrade to the latest core of course.
Comment #159blueblade CreditAttribution: blueblade commented
Comment #160klonos CreditAttribution: klonos commented
@mathieu: can you please take a look at what Gábor said in #152?
@Gábor: I am not sure of how what you mention in #152 can possibly have an effect. Can you please provide a use scenario we can test?
This works for me just fine in two websites + a few others report their issues resolved by using it. Given the fact that #358315: drupal_lookup_path() not respects alias' order is fixed and included in D6.16, it will be a shame for this to miss the next D6.17
thank you.
Comment #161mathieu CreditAttribution: mathieu commented
@klonos - I lost my test setup, so I couldn't reproduce anymore with 6.16. I'll have more time next week and so will @mvc - he told me he'd give a hand when he could.
Comment #162MyXelf CreditAttribution: MyXelf commented
Comment #163SHAAAAAA CreditAttribution: SHAAAAAA commented
|Needs work
|» Needs review
Comment #164marcushenningsen CreditAttribution: marcushenningsen commented
|Needs review
|» Reviewed & tested by the community
The patch works for me. Thanks a lot.
Comment #165miro_dietiker CreditAttribution: miro_dietiker commented
|Reviewed & tested by the community
|» Needs review
Concerns of Gabor in #152 (and others?) need to be clearly considered and answered. Due to complexity all possible cases need to be checked to set this rtbc.
You can't set this to rtbc just because "it works".
Comment #166klonos CreditAttribution: klonos commented
I agree with Miro, this is simply tested by the community ( TBC :P ) but not thoroughly reviewed by someone that understands the code and every thing that changing it implies.
Once it is tested + reviewed we can set it to RTBC.
Comment #167earnie CreditAttribution: earnie commented
|Needs review
|» Reviewed & tested by the community
Looking at the first part of the patch, I'm seeing that drupal_get_path_alias() might return an alias which was set to be language neutral. So there might not be an existing alias you can update for this language. My understanding is that the alias will not be saved in that case. Am I missing something?
// There is already such an alias, neutral or in this language.
- // Update the alias based on alias; setting the language if not yet done.
- db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE dst = '%s'", $path, $alias, $language, $alias);
+ db_query("UPDATE {url_alias} SET src = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $language);
I don't see how leaving the UPDATE as is resolves the problem Gabor brings to the table in #152, that problem is a totally different issue. The patch has been tested to prove that an existing problem with the UPDATE is resolved.
Comment #168Kasper Souren CreditAttribution: Kasper Souren commented
I'm converting a site to Drupal where most paths are the same across languages. This has been biting me hard for a long time now.
This patch works very well for me.
Comment #169ufku CreditAttribution: ufku commented
the patch works!
Comment #170klonos CreditAttribution: klonos commented
Just upgraded to D6.17 and the patch from #146 still works as expected. Sad it didn't make it in core thought. Hope to see it in D6.18 perhaps then (?)
Comment #171NiklasBr CreditAttribution: NiklasBr commented
Thanks, patch seems to be working on the one installation I tested as well. Let us hope for inclusion in D6.18 then.
Comment #172Marko B CreditAttribution: Marko B commented
Tested #62 works fine. Bulk alias still is a mess and doesnt work, but i guess this is different issue.
Comment #173klonos CreditAttribution: klonos commented
@deepM: as already suggested, you need to use the latest patch from #146. If that doesn't work for you, please report any issues so it can be fixed.
PS: ...I guess what I really mean to ask is: Do you mean that patch in #62 works while the one in #146 doesn't?
Comment #174Marko B CreditAttribution: Marko B commented
Yes i ment that :-) confirming its not working ok, recomend #62.
Comment #175klonos CreditAttribution: klonos commented
I've got a working live multilingual site using D6.17 with the patch from #146 working just fine. The site started with D6.15 and was eventually upgraded to 6.16 and recently to 6.17. All these drupal versions working 100% ok with #146.
So, I'll have to ask again... did you try #146 and had certain issues? Or is it simply that you heard/think it doesn't work? If you did try it as you say, what were the issues you had?
The reason I insist is that we were waiting for this to get committed to D6.17 (unfortunately it did not) and if it does have unresolved issues, then people should report them so they can eventually be fixed.
Comment #176Marko B CreditAttribution: Marko B commented
Nice that you were persistant, in fact just for sake of debuging this i went again patching and realised that this patch in fact didnt do nothing for me, i used TortoiseDiff for it and when i try to patch nothing happens (maybe this index 5c66e06..668d6b0 100644 in header confuses it or something) anyway patches from other post (http://drupal.org/node/357185) #47 and one before doesnt work (got a bit confused from all the posts patches, versions etc). So this one i dont know, #62 is working, guessing this is just similar to that one, it should work. Sorry for confusion :)
Comment #177klonos CreditAttribution: klonos commented
Thanx for taking the time to reply marko.
I have sent you a personal mail concerning this, since I was afraid I might go off-topic here. Please see my answer there and check if you need to manually apply the patch. If you find out that it doesn't apply cleanly, then please report it so it can be rerolled.
Comment #178Marko B CreditAttribution: Marko B commented
Thanx on effort, its working, but still dont know why 146# wont apply with tortoise patching, doesnt throw any errors, something is not right with why patch is done.
Comment #179klonos CreditAttribution: klonos commented
Thank you marko. Now we are back on track with the patch in #146.
I always apply my patches manually though and I am not sure if it needs a reroll or not. Anyone care to check?
Comment #180Marko B CreditAttribution: Marko B commented
Well i would like that everyone puts ALSO patched filed instead of only patch, just for the sake of making everything quicker for those who dont want to manualla apply and try to figure how to do it etc, would make life easier for many people and its this what is this all about or not? :-)
Comment #181klonos CreditAttribution: klonos commented
I was wondering about the same thing as a newcomer to drupal. I guess there are pretty solid reasons for not doing so. On top of my head I would say storage/space issues.
Comment #182klonos CreditAttribution: klonos commented
... + what about those cases where you need to apply more than one patch(es) on the same module. How would you 'merge' the changes from two pre-patched versions then?
Anyways, I think we are getting off-topic here ;)
Comment #183pfournier CreditAttribution: pfournier commented
FAILED: [[SimpleTest]]: [MySQL] Fetch test file: failed to retrieve [path_module_269877#183.patch] from project client. View
I had a look at patch #146 and it does not seems right (although it may fix the bug reported in this thread).
Looking at how path_set_alias() is used in the path and the path_auto modules, I see these cases:
- we want to create a new alias; in this case $pid = NULL, and both $path and $alias are set;
- we want to update a specific alias; in this case $pid is set to the alias pid to update, and both $path and $alias are set;
- we want to delete a specific alias; in this case $pid is set to the alias pid, and at least one of $path or $alias is not set;
- we want to delete all alias for some source or destination; in this case, $pid is not set, and at least one of $path or $alias is not set.
The first part of the function (if ($pid) {...}) does a good job, except that it fails to properly check for duplicate aliases, as it is claimed in the function header comment.
The second part of the function (else if ($path && $alias) {...}) puzzles me. First, we use drupal_get_path_alias() to check for duplicates; the function does a poor job at telling us if an alias exists for a given language. Then, there is the UPDATE statement which could change the source or the language of an alias. I did not see any use for this in the current Drupal code. If I am missing something here, please point it out.
Finally, the last part of the function (else {...}) is ok.
Here is my patch that make the function behave as I think its callers expect it. I also added a return value, to inform the caller whether the alias was set or not.
After applying the patch, I recommend you delete and rebuild all your aliases to get rid of duplicate aliases.
Comment #184pfournier CreditAttribution: pfournier commented
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path_module_2.patch. View
Well there seems to be some problem with the patch filename. Here is one you'll be able to download.
Comment #185pfournier CreditAttribution: pfournier commented
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path_module_3.patch. View
Argh, here is the right patch file... Sorry for the noise and the extra work, QA Bot.
Comment #186miro_dietiker CreditAttribution: miro_dietiker commented
|Reviewed & tested by the community
|» Needs review
pfournier it's a d6, you should postfix your patch with -D6 to leave testbot as he can't do anything for you (he will fail applying the patch to D7).
Hehe.. great filename you where able to bring in!
So again this needs review.
Comment #187Amarjit CreditAttribution: Amarjit commented
Fantastic, #146 works perfectly for me. I have tested created a new translation, editing the path after creation, deleting paths. All URLs and translations seem intact.
Cheers mathieu.
Comment #188pfournier CreditAttribution: pfournier commented
Thanks Miro, I was not aware of that. Here is the correctly named patch file.
Comment #189ufku CreditAttribution: ufku commented
|Needs review
|» Reviewed & tested by the community
Comment #190pfournier CreditAttribution: pfournier commented
|Reviewed & tested by the community
|» Needs review
Patch #146 may work, but it is not correct.
My setup is as follow: Drupal 6.16 + pathauto 1.3 + i18n 1.4 (i18nsync module enabled).
Pathauto is set to use node titles as alias.
Languages enabled: english, french, with french as default (I dont think default language have an impact on this).
Using the english admin interface (I dont think the admin interface language have an impact on this).
Create a node: language english, title "Foo" (so alias will be "foo").
Then go to admin/build/path/add and add a new alias for the same node:
- All languages
- alias "foo"
Despite the success message, the new alias will not be added to the alias table because drupal_get_path_alias($path, $language) finds an alias. Original code was not working well either: it replaced the english alias by the All languages alias. We should not rely on drupal_get_path_alias() to tell us if we are creating or updating an alias.
Patch #188 works correctly: it adds a new All language alias, while preserving the english alias.
Comment #191klonos CreditAttribution: klonos commented
well... the patch needs to be named path-path_set_alias-269877-188-D6.patch
Comment #192checker CreditAttribution: checker commented
Comment #193bbenone CreditAttribution: bbenone commented
Using the patch from #188, I think that I am still seeing this... but maybe it's something different:
My configuration: Drupal 6.17 + i18n 6.x-1.2, pathauto 6.x-1.3
I'm starting with a plain old page node, in english, with a manually set path of: about/qualifications
I click translate, and I add a spanish translation
On the node creation screen, I uncheck auto alias, and give the new node a path of "about/qualifications" (same as the original node, but should be OK since it's a different language)
Then click save
... two problems:
(1) I receive this error message:
user warning: Duplicate entry 'content-' for key 2 query: INSERT INTO path_redirect (source, redirect, query, fragment, language, type, last_used) VALUES ('content', 'about/qualifications', '', '', '', 301, 1277350622) in /home/worldac2/public_html/includes/common.inc on line 3468.
(note that language is being set to '' in the SQL stmt....)
the new node, spanish translated, has the correct path "es/about/qualifications" (good)
but the old node lost it's path and is now using an automatically generated default alias "content/qualifications" (bad)
Fixing the old node seems to break the new node, and vice-versa...
Comment #194klonos CreditAttribution: klonos commented
@bbenone: what is your language setting in ../admin/settings/language/configure ?
Comment #195bbenone CreditAttribution: bbenone commented
@klonos: I'm set to "Path prefix with language fallback."
also, fwiw, I noticed after my post last night that my i18n module was a little out of date. I've upgraded to 6.x-1.5 but am seeing the same behavior.
Comment #196klonos CreditAttribution: klonos commented
ok then, can you please try the patch from #146 and let us know if that one solves the issue you are having?
Comment #197magnestyuk CreditAttribution: magnestyuk commented
patch in #188 works for me.
Comment #198pfournier CreditAttribution: pfournier commented
@bbenone: Do you have the Path redirect module enabled? If so, could you try to disable it and see if it fixes the errors you get?
Comment #199bbenone CreditAttribution: bbenone commented
Ya, those errors I posted earlier relate to path_redirect, which is unrelated to this conversation. Sorry for the diversion.
Tonight, I tried a clean install, with just i18n + pathauto, and applied the patch. I guess it's working - I don't get any error messages there, but, I still do see the behavior of my custom path being lost. (see issue 2 in comment 193 above). I suppose that is a different bug unrelated to the current thread?
Comment #200mrfelton CreditAttribution: mrfelton commented
@bbenone: with the patch at #188, I see the exact same problem that you described in #193. However, I don't have path_redirect set to do anything when an alias changes, so I didn't get that part of the error.
Comment #201mrfelton CreditAttribution: mrfelton commented
@klonos: patch at 146 makes no diference either.
For clarification...
Drupal 6.17 + i18n 6.x-1.4, Pathauto 6.x-1.x-dev (from today)
Default language, Irish (/)
Additional language, English (/en)
1) Create a node in the default language (Irish), with a manual alias of /test
2) Translate the node, and give it a manual alias of /test
At this point, /admin/build/path/list/test shows two aliases, each set to the correct language and pointing at the correct node. Doing a dsm($node) on nodeapi:view, shows $node->path to be set correctly for both nodes. However, doing a dsm($node) in form_alter on the node edit page show that $node->path is only set on the /en node.
Another strange thing:
ENGLISH NODE (/en/test-en):
click the edit button, takes you to /en/node/4658/edit
$node->path is unset, and the pathauto manual alias textarea is empty.
manually adjust the url, REMOVONG the path prefix so that it is /node/4658/edit, and now $node->path is set, and pathauto has picked up the manual alias or /test-en
IRISH NODE (/test-ie)
click the edit button, takes you to /node/4657/edit
$node->path is unset, and the pathauto manual alias textarea is empty.
manually adjust the url, ADDING a path prefix so that it is en/node/4658/edit, and now $node->path is set, and pathauto has picked up the manual alias of /test-ie
So, why does adjusting the path of the node edit page like this affect weather or not the path is loaded with the node? and why would adding a /en prefix onto the path of the node edit form for an Irish node, cause it to correctly load in the Irish alias for that node?!
Comment #202pfournier CreditAttribution: pfournier commented
@greggles: this may be of interest to you.
I was able to reproduce the problem in #193 (2). This is not a bug in the patch. The behavior was present before.
Here is what is happening when you save the translated node:
1) On node_save(),
- path_nodeapi() is called and saves the path alias you entered for the translated node.
- pathauto_nodeapi() is called, but since you unchecked the Automatic Alias checkbox, nothing is done.
2) i18nsync sync the original node and saves it:
- path_nodeapi() is called for the original node and the alias you entered for the original node is resaved, unchanged.
- pathauto_nodeapi() is called for the original node, but because there is no value for the Automatic Alias checkbox on the original node (it was not presented to you, and pathauto does not save the value of this checkbox), pathauto thinks it should create an automatic alias for the original node. The alias is created based on the settings provided in admin/build/path/pathauto. The effect of this is to reset the original node alias to the automatic alias; in your case, the automatic alias probably evaluates to an empty string (like [menupath-raw], if your page is not inserted in a menu).
This bug is also present without the patch in #188; however, because of the bug in the original path_set_alias(), the effect is slightly different (the translated node looses its alias instead of the original node).
I think the only way to cleanly fix this problem would be to apply patch 188 and modify pathauto to save the state of the Automatic Alias checkbox. This way, it would know whether it needs to generate an Automatic Alias on a node_save(). There are many issues in pathauto about this problem. I think patch #188 is a first step to solve this problem in pathauto.
In the meantime, do not mix custom aliases and automatic aliases.
Comment #203mrfelton CreditAttribution: mrfelton commented
My problem, which is exactly the same as that described in #193, is caused by the patch at #456824: drupal_lookup_path() speedup - cache system paths per page., which I had applied to my Drupal 6.17 installation, and which is included in Pressflow. By reversing that patch, everything is working exactly as it should do once again. I can can translate a node, and manually set the alias of each to be the same. The aliases save correctly, and they are loaded correctly.
I found this out by following up on my comments in #201. drupal_get_path_alias() would only return the alias correctly for one language. Clearly, this has to do with the caching that has been implemented by that patch. If any of you that are experiencing this problem are using Pressflow, or have patched your Drupal install, please try reversing that patch and see it fixes the issue for you.
Comment #204rup3rt CreditAttribution: rup3rt commented
Comment #205spylvas CreditAttribution: spylvas commented
Comment #206pfournier CreditAttribution: pfournier commented
#358722: Node aliases lost/changed when using i18n synchronize translations have a patch for the problem discussed at the end of #202 (I did not tested it yet)
Comment #207mdorrell CreditAttribution: mdorrell commented
I'm using the patch from #188. This is working for us. What is the status of this issue?
Comment #208ayalon CreditAttribution: ayalon commented
To get the patch from 188 work correctly you have to turn of i18n_syncronize or use my updated patch on #358722: Node aliases lost/changed when using i18n synchronize translations.
Comment #209fago CreditAttribution: fago commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_6_path_lang.patch. View
I also ran into the problem together with i18nsync. From what I've seen passing the previous $pid is somehow broken, so there were quite some duplicates. Without a pid, path_set_alias() additionally cares about setting the language of existing entries - which I think happens for the node_save of the synced-node and as effecit is the cause for the loss of the currently edited node's path alias.
Attached patch ensures the right pid is passed, which fixes the problem for me.
Comment #210pfournier CreditAttribution: pfournier commented
@mdorrell, I am using patch #188 and patch from #358722: Node aliases lost/changed when using i18n synchronize translations, and everything seems to work fine (I am using i18n_sync).
Comment #211fmjrey CreditAttribution: fmjrey commented
A patch submitted yesterday in issue #358722: Node aliases lost/changed when using i18n synchronize translations could also fix this one here:
With all these patches around, I'm not sure where to look for a solution.
If anyone as a good handle on this issue, please let us know what the definite patch is.
Comment #212John T. Haller CreditAttribution: John T. Haller commented
As this bug has been open for 2 years, it doesn't look like it will ever be fixed. Perhaps it should be closed and individual bugs created for the individual issues as several seemingly-unrelated bugs are pointing here now, which is completely counterproductive as this bug is not going to get fixed.
I'm tired of applying ( http://drupal.org/files/issues/path-module-D6.patch ) to every single Drupal release. I don't use PathAuto or anything else. I just use the core along with Locale and a few other bits. But, without the patch, whenever I save a page and change the URL, a new entry is created. It's getting OLD.
Comment #213fago CreditAttribution: fago commented
ad #212, I don't know where this patch is from, but drupal_get_path_alias() also retrieves the alias if the path doesn't match, thus for that case the update query would fail. However I agree that path_set_alias() should be fixed to properly work without $pid - or don't support updating without $pid at all.
#209 should fix the problem nevertheless just by passing the $pid.
Comment #214ayalon CreditAttribution: ayalon commented
@fago: your patch works but it does not solve the problem with i18n_sync decribed here:
If you use i18n_sync, you also have to apply the patch here:
Comment #215fago CreditAttribution: fago commented
I don't know about pathauto, but it solves the problem for i18n_sync + the path module.
Comment #216AdrianB CreditAttribution: AdrianB commented
Comment #217ufku CreditAttribution: ufku commented
This is the patch from #188 by pfournier.
This patch ensures that path_set_alias() does not create duplicate aliases or update multiple aliases belonging to different languages.
The bug can easily be reproduced by node_save() where you save one of two nodes that have the same alias in different languages.
Please do not submit patches that are related to/solving issues from other projects.
Let's review and get it committed so we can have a better chance to solve related issues in other projects.
Comment #218fmjrey CreditAttribution: fmjrey commented
Same patch as #217 but in a standard form.
ufku: could you tell us more about your patch?
Comment #219ufku CreditAttribution: ufku commented
@fmjrey: It's pfournier's patch from #188. It's explained in subsequent comments.
To sum up, this patch is all about making path_set_alias() work correctly with duplicate aliases.
Please do not mix this with issues from other projects.
Once this is fixed, it will be easier to fix other path related issues.
Comment #220fmjrey CreditAttribution: fmjrey commented
ufku: sorry, with all these patches I got myself confused on my own install. I thought I already applied the patch you talked about, but in fact it was an older one I applied.
So yes, #218=#217=#188
Comment #221GiorgosK CreditAttribution: GiorgosK commented
patch from #218 solves following problem
create page "abcd" it automatically gets alias "en/abcd"
alias table contains "abcd" > node/1 language en
creating page translation creates a duplicate alias
"abcd" > node/1 language en
using this patch we will get correct alias for page translation
"abcd" > node/2 language el
Comment #222gnindl CreditAttribution: gnindl commented
Also have a look at this patch #790338: All aliases get deleted when translatable nodes with same URL alias are reverted to previous revision which solved this issue in an elegant way.
Comment #223pfournier CreditAttribution: pfournier commented
I did not test patch from #790338: All aliases get deleted when translatable nodes with same URL alias are reverted to previous revision, but it seems to me that whenever an alias is updated in a language, we will still loose the All languages alias (and vice versa).
Comment #224koorneef CreditAttribution: koorneef commented
Just my 2 cents: the patch in #209 by Fago works for me.
I don't use i18n sync anymore.
Thanks !
Comment #225blaiz CreditAttribution: blaiz commented
Comment #226j0nathan CreditAttribution: j0nathan commented
Comment #227akaliel CreditAttribution: akaliel commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path_alias_2_1.patch. View
I tested the path from http://drupal.org/node/790338 and found that it fixed the issue for the me when updating node aliases. It didn't overwrite node aliases for all other languages, just then ones I was trying to.
But it didn't solve the issue when deleting aliases. With that patch it would still delete all node aliases of the same destination of all languages instead of just the current language. So I extended the same logic from that patch for deleting and then added a new patch to that thread. But I will add it here too.
Comment #228akaliel CreditAttribution: akaliel commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path_alias_lang.patch. View
Sorry the last patch was not rolled against the latest Drupal core. This one is!
Comment #229GiorgosK CreditAttribution: GiorgosK commented
did you try patches above #218 ? #187 ? #146 ?
how does your patch relate to those ?
a lot of work has gone into them and we can not ditch them like that !!!!!
please consider evaluating and reporting for one of those before posting your own solution !!
please list your test cases
it seems as though everyone creates a patch for their test case and this issue will never move forward
my testcases
admin/settings/language/configure > path prefix
english language prefix "en"
greek language prefix "el"
english page "page" with path "page"
translation of "page" in greek language with path "page"
pages can be browsed at "en/page" and "el/page"
admin/settings/language/configure > path prefix
english language prefix "" (empty)
greek language prefix "el"
english page "page" with path "page"
translation of "page" in greek language with path "page"
pages can be browsed at "page" and "el/page"
admin/settings/language/configure > path prefix
english language prefix "" (empty)
greek language prefix "el"
neutral language page "page" with path "page"
english page "page" with path "page"
neutral page can not be browsed since both have same path "page" and "page"
please list more of your test cases
1. 2. work ok with drupal 6.19 with no patches !! (can anyone confirm this ?)
3. when pathauto is enabled creates "page-0" as path for english page
but if we edit and force path alias to "page" then we have both paths same "page"
(one page cannot be browsed)
Comment #230aquila CreditAttribution: aquila commented
Subscribing to this quite horrible issue.
Comment #231aquila CreditAttribution: aquila commented
How hard would it be to code real tests for this problem? I'm willing to give it a shot to help solve this bug, but I do not have too much time.
The strategy I consider:
- download a clean drupal 6.19
- enable i18n
- install simpletest
And then?
Comment #232fago CreditAttribution: fago commented
Peope, don't forget the fix at #209, which is fixing the problem at its source. Apart from that path_set_alias() could improve too, yes. But still passing $pid is broken, what #209 fixes and solves the foundational problem appearing as soon as any module does use a programmatic
Comment #233aquila CreditAttribution: aquila commented
My first attempts at a test are described in this forum post: http://drupal.org/node/939594
I can't get the correct alias to appear, at this moment.
Any help would be appreciated, so we can get the test cases described in #229 to be covered automatically and hopefully solve this bug!
Comment #234GiorgosK CreditAttribution: GiorgosK commented
Its very possible that this issue has been resolved from a patch on another similar issue
or from i18n recent fixes ...
please test with a clean 6.19 installation
and i18n6.x-1.7 enabled
all cases are assuming
admin/settings/language/configure > path prefix
english default language
admin/settings/language/edit/el "path prefix"="el"
admin/settings/language/edit/en "path prefix"=""
node "abcd" - en - alias "abcd" - view at "abcd"
node "abcd" - el - alias "abcd" - view at "el/abcd"
node "abcd" - en - alias "abcd" - view at "abcd"
node "abcd" - neutral - alias "abcd" - view at "abcd"
node "abcd" - en - alias "abcd" - view at "abcd"
node "abcd" - el - alias "abcd" - view at "el/abcd"
node "abcd" - neutral - alias "abcd" - view at "abcd"
admin/settings/language/edit/en "path prefix"="en"
node "abcd" - en - alias "abcd" - view at "en/abcd"
node "abcd" - el - alias "abcd" - view at "el/abcd"
node "abcd" - en - alias "abcd" - view at "en/abcd"
node "abcd" - neutral - alias "abcd" - view at "abcd"
node "abcd" - en - alias "abcd" - view at "en/abcd"
node "abcd" - el - alias "abcd" - view at "el/abcd"
node "abcd" - neutral - alias "abcd" - view at "abcd"
my tests conclude
same aliases in different language works OK
this issue has been resolved and should be closed (please confirm)
when "neutral" node gets created (cases 1b 1c 2c) it can't be viewed
(except on case 2b) because alias overlaps with language specific aliases
but this was not directly discussed in this issue and
can be addressed with a different patch/issue
please test without any patch the test cases above
or post your own test cases
Comment #235ayalon CreditAttribution: ayalon commented
GiorgosK, your test case is useless if you did not activate i18n sync to test this issue. One main problem is, that i18n sync overrides aliases...
Comment #236GiorgosK CreditAttribution: GiorgosK commented
I have "translation synchronization" enabled on my tests above
but that would probably be a different issue since #358722: Node aliases lost/changed when using i18n synchronize translations
so above tests can be tried and reported with i18n sync enabled and disabled
Comment #237neochief CreditAttribution: neochief commented
|Needs review
|» Reviewed & tested by the community
As co-maintainer of i18n, I'd say that it's issue of i18n, not Drupal core. Let's ignore it, finally patch the core, and solve #358722: Node aliases lost/changed when using i18n synchronize translations independently. #218 is just perfect.
Comment #238klonos CreditAttribution: klonos commented
+ for moving on with this too and handle the other issue in its own queue.
Comment #239aquila CreditAttribution: aquila commented
I would like to pitch the idea that we should add tests first. The testcases in #229 and #234 are quite clear, and will protect us from having to go to this patch chaos again. I have tried to start testing but to me it is very unclear which of the cases listed above should fail, making it hard to verify the correctness of the tests.
Comment #240kuzyakiev CreditAttribution: kuzyakiev commented
Comment #241Danny_Joris CreditAttribution: Danny_Joris commented
I don't receive these errors as stated in the original issue, but I just have the issue described at #790338: All aliases get deleted when translatable nodes with same URL alias are reverted to previous revision . That patch worked for me, but because some of you said that it was not a complete solution or that it would conflict with other patches, I tried the patch shared in #218 .
This 'works', but it synchronizes the same path alias with all translations, even when the titles are different. My issue is only that when two translated nodes have the same title (and thus should have the same path alias), the second node translation would not get a path alias assigned automatically.
Comment #242Danny_Joris CreditAttribution: Danny_Joris commented
I also tried #228 , which seems to be an improved version of #790338: All aliases get deleted when translatable nodes with same URL alias are reverted to previous revision .
This works good, except whenever I give one of the 2 translations another title, it will correctly update it's alias, but it will also add an exact copy of the alias for the other translation. This works in both ways.
In the example in the attachment you can see that I first cleared all aliases for nodes 148 and 149 and saved them again to assign aliases to them again. Then I updated the alias of the Dutch (Nederlands) translation, resulting a duplicate alias for the English (Engels) node. Then I changed the English node a few times, resulting in correctly updating the English url alias, but making multiple duplicate aliases for the Dutch node.
Comment #243klonos CreditAttribution: klonos commented
Are #218 and #228 meant to be used/tested in combination? If so, can someone please merge them in one?
Comment #245BenMirkhah CreditAttribution: BenMirkhah commented
Comment #246BenMirkhah CreditAttribution: BenMirkhah commented
#209 worked for me, thanks fago
Comment #247ikeigenwijs CreditAttribution: ikeigenwijs commented
Comment #248haffmans CreditAttribution: haffmans commented
|Needs work
|» Reviewed & tested by the community
I've tested the patch in #218 using Drupal 6.20.
Before the patch, only with Synchronize Translations enabled, I would get duplicate entries in the URL aliases list if a translation's path is set to the same as the original node. When disabling Synchronize Translations the issue did not occur. Also note that system_update_6055() changed the unique indexes to add the pid. Hence the error in the original post is less likely to get, yet problems still exist with Synchronize Translations enabled without the patch.
I have not enabled pathauto or the Category modules. I've set the negotiation to Path Prefix only. The Synchronize Translation options for the specific node type has only "Status" checked.
My test case:
1. Create an English node, set a path
2. Create a Dutch node (as translation), set the same path
3a. Unpatched: there would be two duplicate aliases (same src, dst, language - different pid).
3b. With patch: A secondary alias was made as expected (same dst, different src/language/pid)
4. Edit/resave both nodes. Make sure the path is filled in.
5a. Unpatched: editing the translation (Dutch) spawned another duplicate alias. Note that the translation had no alias so the path had to be set again in the form.
5b. Patched: the aliases remained in-tact as expected.
This is fixed by the patch in #218. This patch also seems to solve (very similar/duplicate?) issue #370089: warning shown when the same url alias name is used for different languages.
In combination with Pathauto, however, I also seem to get some odd behavior (with the auto-path depending only on the title):
1. Create an English node
2. Create a translation (Dutch) with a different title
3. Remove the created aliases from the Aliases page
4. Edit one of the nodes.
Now note that regardless of the "Automatic Alias" checkbox on the edit form, at least the aliases of the other translations are recreated. If you check the "Automatic Alias" box, the aliases of both nodes will be created. In fact the path of translations seems to be automatically recreated on every save, regardless of settings etc..
I think that is a bug in the pathauto module though, and not so much in Drupal core. As suggested in #358722: Node aliases lost/changed when using i18n synchronize translations the pathauto_persist module might solve it (I haven't tested it). Danny_Joris (#241/#242): I assume that is the problem you are talking about?
All in all, given previous reviews and comments, I think the patch in #218 can and should be committed.
Comment #249neochief CreditAttribution: neochief commented
I think so too.
Comment #250GiorgosK CreditAttribution: GiorgosK commented
please everyone test 218
so we can push it in core (have a feeling with this there is going to be another bunch of problems solved)
just tested and it still works with 6.20
Comment #251sylvain_a CreditAttribution: sylvain_a commented
Comment #252philipz CreditAttribution: philipz commented
Patch 218 fixed the issue for me.
Before the patch I couldn't save the duplicate path alias of multilingual content with the same path.
Now it's working.
Comment #253roderik CreditAttribution: roderik commented
|Needs review
|» Reviewed & tested by the community
Thanks for #209. The thing is so clear in itself, that IMHO it should not wait for getting the other patches approved/committed. With that in mind, I created a separate issue #1104764: path_set_alias() being called with incomplete arguments with a story to hopefully convince people to set this RTBC -> get this committed independently/first.
I hope people who have tested this, will set RTBC soon, if they agree.
Edit: wait. After lots of talking to myself, even I don't agree with myself anymore. Need to look at this later.
Comment #254roderik CreditAttribution: roderik commented
|Reviewed & tested by the community
|» Needs review
Uh - wait.
Without testing the code, I see a functionality change.
Here's the comment from #183, which basically goes with #218:
Looking at how path_set_alias() is used in the path and the path_auto modules, I see these cases:
- we want to create a new alias; in this case $pid = NULL, and both $path and $alias are set;
- we want to update a specific alias; in this case $pid is set to the alias pid to update, and both $path and $alias are set;
- we want to delete a specific alias; in this case $pid is set to the alias pid, and at least one of $path or $alias is not set;
- we want to delete all alias for some source or destination; in this case, $pid is not set, and at least one of $path or $alias is not set.
The first part of the function (if ($pid) {...}) does a good job, except that it fails to properly check for duplicate aliases, as it is claimed in the function header comment.
The second part of the function (else if ($path && $alias) {...}) puzzles me. First, we use drupal_get_path_alias() to check for duplicates; the function does a poor job at telling us if an alias exists for a given language. Then, there is the UPDATE statement which could change the source or the language of an alias. I did not see any use for this in the current Drupal code. If I am missing something here, please point it out
The UPDATE statement which you are removing here, is actually used very often.
See #209 (or #1104764: path_set_alias() being called with incomplete arguments):
path_nodeapi('update') is called from almost every node_save() call.
In almost all cases (except when invoked from a manual node edit form), path_nodeapi('update') then calls path_set_alias() to update the alias, without $pid set - only with $path && $alias set.
Therefore: in the '($path && $alias)' case, the code is expected to update the existing node's path. (Granted, it does it badly and introduces bugs.) But patch #218 just throws that out the window and never updates the existing node's path. That's a fundamental change in functionality.
(...which could be overcome if you apply #209... but... it still doesn't feel right to just change things like that.)
Comment #255roderik CreditAttribution: roderik commented
I need to come back on this tomorrow when my head's clearer.
*sigh* that'll teach me for getting into other people's issues... :p
Comment #256pfournier CreditAttribution: pfournier commented
When we modify a node for which an alias was already set, path_nodeapi('update') receives a node with a pid set, thanks to node_form_submit_build_node() calling node_submit($form_state['values']).
The only case where path_nodeapi('update') would be called without $node->pid being set would be when we edit a node with no alias.
So to make my comment #183 clearer: I do not see any case in Drupal where we would try to update an alias by calling path_set_alias() with parameter $pid = NULL.
Comment #257roderik CreditAttribution: roderik commented
To make my comment #254 clearer: it happens quite often. Namely almost every time any drupal contrib module calls node_save() -- because $node->pid is almost never set in that case (except when a user is editing a node through the edit screen, which is the case you've described.)
node_save() calls node_invoke_nodeapi($node, 'update') -> path_nodeapi('update')
This is actually where a lot of people are losing their aliases on multilanguage sites: i18nsync.module calls node_save() on all translated nodes, which eventually calls that buggy UPDATE statement.
(But I'll need to revisit the code tomorrow, to grasp my own view on solving this... I've gone too far, to chicken out on that now.)
Comment #258pfournier CreditAttribution: pfournier commented
You are right.
In Drupal core, I do not think the intent of calling node_save() is ever to change the path alias (I do not see any attempt to set $node->path anywhere); however, it is not impossible that some contributed module does this.
I need some sleep too, but I think path_nodeapi('load') should provide $node->pid as well as $node->path.
Comment #259mike dodd CreditAttribution: mike dodd commented
I have been having issues with this for the last 2 years, I am currently on Drupal 6.20 and the latest version of revisions, and i18n.
The easiest way to recreate the problem is with a little custom moudle or using VBO. simply load the node, update any field and run node_save and the URL alias of the original English node will be removed (only if it has the same alias as the foreign language version).
This has been causing me a massive heartache as I have lots of banners and double click traffic directed to my site and these have to be to the alias and not the node id (as the redirect forces them to be rejected).
I have just applied patch 209 to Drupal core 6.20 and everything seems to be working brilliantly, at least in terms of i18n, revisions and updating nodes. I will be properly testing this patch on the weekend but I everything seems to be OK.
Comment #260roderik CreditAttribution: roderik commented
A lot of things have dawned on me while tracing code in the past 24 hours...
Re-edited message. In the end I may be concluding that the patch in #218 is correct in practice, even though I didn't agree with the 'theoretical backing', in #254...
#1 re the patch in #209:
I shouldn't have made that separate issue. #209 introduces a behavior change (if $node->path is different from any alias that is now in the 'url_alias' table, it will overwrite that alias, while at the moment it adds a second alias). Plus it doesn't account for nodes which have multiple aliases stored in the 'url_alias' table already. So I closed http://drupal.org/node/1104764#comment-4262840 again.
#2 re the patch in #183 / #218:
@jfournier: I see that you have / want to rewrite some behavior to do 'what it should have done in the first place'. (Also setting $node->pid on path_nodeapi('load') would likely influence nodes which have more than one alias.)
Even though your assumptions are (mostly? - #254) right in theory, I really don't think we should treat the code like this. Drupal 6 is a supposedly-stable piece of software with countless different exotic installs. We should be very careful not to introduce regressions. Don't 'fix' (i.e. change) any code behavior that isn't broken, to make it 'theoretically right'. 'Keeping behavior the same' is more important than 'being right'.
(Sure, path_set_alias() is ugly. We can't do much about that. Luckily, it does not exist anymore in D7, so that 'problem' will go away.)
The above remark is however complicated by a change which was introduced in D6.16...
#3 re indexes and SQL errors...
When I was reading patch #183 / #218, I thought that the SELECT statements used for checking stuff, were introduced to fix actual database errors.
It seems I was wrong. All SQL errors are gone already, because the constraints on the url_alias table have been deleted in D6.16! (For the cause of the deletion, see #1106030: Node edit form risks deleting another node's path alias.)
Edit this means that we have the choice between
1- just taking advantage of this, to keep the patch in this issue as non-intrusive as possible while fixing the reported errors (which still exist - i.e. not the SQL errors). Even if deleting the constraints was a mistake, this will not be easily 'fixed' again, since it's made its way into D7 too...
2- reverting the behavior to the way it was in D6.15 - which means introducing uniqueness checks for dst+language (and perhaps src+language) in the code.
Since we can't have both, in this case I'm leaning towards #2. This is close to what patch #218 does - or maybe completely equal.
I'm done again for today :) Will look at #218 and probably rewrite some of the comments, for review.
Comment #261roderik CreditAttribution: roderik commented
|» roderik
|Needs review
|» Needs work
*message deleted*
better patch in #263.
Comment #262roderik CreditAttribution: roderik commented
Comment #263roderik CreditAttribution: roderik commented
|» Unassigned
|Needs work
|» Needs review
This patch is equal in behavior to #218, except it adds back the UPDATE statement in the 2nd block, for one edge case.
This eliminates 'the bug which has been making so many lives miserable', but I'm pretty sure that this keeps 'what it was meant to do':
If an existing node's language is changed from 'language neutral' to another language, and then node_save() is called, it changes the language of the existing alias with it.
Amended code and tested. New patch attached.
The checks in #218 for existing aliases are left intact. (Comments are added about when the checks were and were not done.)
Other things I'm suggesting in this altered patch (which don't directly influence code behavior) are:
- when we're checking for existing aliases anyway... don't clear the alias cache if the database already contains the alias we want.
- log watchdog errors to replace the database errors which would be generated before D6.16. (No code is using the return value of the function, and similar functions in D6/D7 don't return TRUE/FALSE either.)
...and I added some comments (not code change) to explain the issue with #209.
(Also IMHO, #228 is better off in its own separate issue.)
Comment #264roderik CreditAttribution: roderik commented
The 'if () { SELECT statement } else { different SELECT statement}' is actually redundant.
(Can I enter a competition somewhere for 'most talking to myself in d.o issue queues'? :) )
Comment #265klonos CreditAttribution: klonos commented
...don't worry Roderik, there are quite a few of us following this issue. If there is something worth saying I'm sure it'll be said.
See... I just contributed to this issue reaching 300 posts without any good reason whatsoever ;)
Comment #266roderik CreditAttribution: roderik commented
I was more referring to me changing my mind all the time, while trying to get a complete grasp on all issues involved :)
(I already spotted a redundant but harmless little sub-expression in an 'if' statement again. But I'm afraid of uploading yet another patch ;) )
Comment #267pfournier CreditAttribution: pfournier commented
@roderik, your patch #264 does a good job at preserving the current functionality; on my side, I rather tried to reflect the use cases in Drupal core and pathauto, the main users (I think) of this function.
One of the problem with the original function is that it tries to update aliases even if the caller does not pass a $pid. I find it hard to code a function that would be able to update an alias without being passed a $pid and always do the right thing. That's why in my patch, if there is no $pid, we only attempt to create a new alias, and not to update an existing one.
For example, assume we have this alias in the database: ('pid' => 1, 'src' => 'node/33', 'dst' => 'alias', 'language' => 'en'). Now, someone calls path_set_alias( 'node/33', 'alias', NULL, 'fr'). Should we create an new alias or change the language of the current alias to fr? Both you and I decided to create a new alias, but original code was updating the alias. Same thing if someone calls path_set_alias( 'node/33', 'alias', NULL, '').
Now, another example. Assume we have this language neutral alias in the database: ('pid' => 1, 'src' => 'node/34', 'dst' => 'contact', 'language' => ''), and someone calls path_set_alias( 'node/34', 'contact', NULL, 'en'). Same question: should we create an new alias or change the language of the current alias to en? You decided to update the alias, as the original code did, but I decided to create a new one. I think your choice may cause problem, because we may legitimately want to create a "default" language neutral alias, and override it with specific aliases for some languages. Furthermore, your function is not symmetric: calling
path_set_alias( 'node/33', 'alias', NULL, '');
path_set_alias( 'node/33', 'alias', NULL, 'en');
does not give the same result as calling
path_set_alias( 'node/33', 'alias', NULL, 'en');
path_set_alias( 'node/33', 'alias', NULL, '');
Alias creation order should not be important.
Comment #268roderik CreditAttribution: roderik commented
For example, assume we have this alias in the database: ('pid' => 1, 'src' => 'node/33', 'dst' => 'alias', 'language' => 'en'). Now, someone calls path_set_alias( 'node/33', 'alias', NULL, 'fr'). (...) original code was updating the alias.
It wasn't. The drupal_get_path_alias() call returns FALSE in that case.
I think your choice may cause problem, because we may legitimately want to create a "default" language neutral alias, and override it with specific aliases for some languages.
Thanks for that thought. I might have skipped over that otherwise.
But... if your sentence is ended by "...which point to a different source path" ...then no, there is no problem. New aliases will be created by 'my' code too.
Furthermore, your function is not symmetric: (...)
Alias creation order should not be important.
That entirely depends on what is considered more important:
* "the function should be symmetric" <- making sense <- you
* "the function should behave as it did before, with the exception of bugs" <- being anal about not introducing regressions (in code that is already scrapped from D7) <- me
I'll let other people decide what's more important. But IMHO, this is the decision point.
...but we're really talking about theoretical use cases here. One way or the other, the normal core & pathauto use cases are covered. I wasn't sure of that when I was just finishing #253, because it was all too much info to ingest for me :)
The only thing the UPDATE statement does is indeed make sure that the second one in
path_set_alias( 'node/33', 'alias', NULL, '');
path_set_alias( 'node/33', 'alias', NULL, 'en');
causes an update instead of an insert. This is also the only reason I can see that the current drupal_get_path_alias() + comment has been put there in the first place. And I think "there probably was a reason for doing so".
(Though, granted, I really don't think it is as big as a reason as I 'feared' a few days ago.)
....(time passes)......
Hey, look what I found: png flowcharts specifying "the reason"! http://drupal.org/node/154517#comment-625541
These basically state the 'asymmetric behavior'...
Comment #269pfournier CreditAttribution: pfournier commented
Good find, this explains the intended behavior... I think the key in implementing this behavior is to use a $pid whenever we want to update an alias (and from what I found, this is what happens), so there is no point in having an UPDATE statement in the !$pid part of the function.
I just realized that your patch introduces a new behavior. Suppose we make those calls:
path_set_alias( 'node/33', 'alias', NULL, '');
path_set_alias( 'node/34', 'alias', NULL, 'en');
(node 34 is the English translation of the language neutral node 33, but both use the same alias). Original code would correctly create a new alias for node 34, whereas your patch would log a warning to the watchdog.
Comment #270roderik CreditAttribution: roderik commented
I think the key in implementing this behavior is to use a $pid whenever we want to update an alias (and from what I found, this is what happens),
Completely agreed, that is what happens in the overwhelming majority of node saves. But...
so there is no point in having an UPDATE statement in the !$pid part of the function.
...you seemingly insist on thinking only about what code in countless existing contributed and custom modules should do, and not about what it can do. I refer to my "That entirely depends..." comment in #268.
I guess Gabor can decide on the route to take, once people set this to RTBC.
(...) your patch would log a warning to the watchdog.
Good call! That is a mistake indeed.
Fixed patch attached.
Comment #271pfournier CreditAttribution: pfournier commented
Sorry for being insistent ;-) This is because I do not think we can implement a function that reliably update aliases without a $pid. I understand that you want to preserve as much as possible the existing functionality. However, for me, this case introduces inconsistent behavior: think about the case where you create aliases in the admin/build/path/add page. Wouldn't you be surprised to see your language neutral alias disappear when you create a new English alias with the same source and destination? I am, and that's what is happening right now (and your patch does not fix that). Try it: create a language neutral alias with source = node/1 and destination = my_alias. Then create an English language alias with the same source and destination: your language neutral alias is gone (in fact, it was transformed into an English alias, thanks to the UPDATE in the !$pid part of the function).
I had a look at your modified patch, and it does not create a new alias as the current code does.
Comment #272roderik CreditAttribution: roderik commented
I had a whole reply typed out, and then I spotted one word:
However, for me, this case introduces inconsistent behavior: think about the case where you create aliases in the admin/build/path/add page.
I had a look at your modified patch, and it does not create a new alias as the current code does.
I don't understand exactly what you mean. Are you saying that the current (unpatched) code does create a new alias, in the case you described? (It doesn't.)
...and then my (perhaps redundant) reply to your argument about the 'create alias' screen:
I can see it, I'm not disputing the argument.
But could we agree that you are discussing the intended behavior / the specification of the code, not the code itself?
Could we also agree that what you are calling 'inconsistent behavior' is exactly in line with the original specification?
It's fine if you say "this does not work for me", but IMHO this argument only has power if we explicitly establish that the original specification of the intended behavior is not valid anymore - or the reasoning behind it was wrong. And IMHO you are going to have to make a good case, if you want to convince people (Gabor) that the intended behavior of this function should change.
Maybe you can, maybe you can't. This ('spec/behavior is not valid anymore') is possible, but I'm really not that interested in investing more energy in finding that out, so far. I don't think "first adding a language neutral alias and then a language specific one" has much use; at least I can't think of a practical case for doing this, so far...
...so I'm still on my "it depends on what is more important" remark in #268. Other people can decide what they think is more important / if they want to change the specs.
Comment #273pfournier CreditAttribution: pfournier commented
I do not dispute the validity of the original specification, which says that if we change the language of a node, the language of the alias should change accordingly. However, we should pass a $pid for that, and that is what Drupal already does. Hence my (humble) opinion that the UPDATE in the !$pid part is unnecessary.
Regarding my claim about the new behavior: I justed tried my example in an unpatched Drupal 6.20. In the URL alias admin page, create those aliases:
Path Alias Language
node/33 alias All languages
node/34 alias English
(Note that the two Paths are different). You will see that you just created two distinct aliases. I did not test with your patch, but I am pretty sure that you will not be able to create the second alias. When creating the second alias, in the original code, the condition
($alias == drupal_get_path_alias($path, $language))
is false, thus INSERTing the new alias, whereas in your patch,
db_fetch_array(db_query("SELECT pid, src FROM {url_alias} WHERE dst = '%s' AND language = '%s' ORDER BY pid DESC", $alias, $language));
does not return an empty set, thus preventing the INSERTion of the new alias.
Comment #274pfournier CreditAttribution: pfournier commented
Sorry, I read your comment #272 to quickly. My reply #271 is about two things:
First paragraph: an inconsistent behavior in the actual code.
Second paragraph: Your revised patch does not fix the new behavior I reported in #269.
Comment #275roderik CreditAttribution: roderik commented
Extremely lame mistake on my part. Sorry for requiring you to explain #269 again to me. (I fixed something in #270 that wasn't the real problem.)
Back to the "I don't care that much what is done, but I care on agreeing about the underlying reasons for doing it" discussion about details:
However, we should pass a $pid for that, and that is what Drupal already does
That depends on your definition of 'Drupal'. I have the feeling we're going round in circles, so I'll try it differently:
Please confirm that you are implying here, that you do not care what currently existing contrib/custom code does & whether its behavior is changed.
Theoretical example: a piece of code which updates nodes, that does:
$node = node_load( some node set to language neutral );
$node->language = 'en';
You say "that piece of code should just pass a $pid for that." End of discussion.
(I guess we are disagreeing on whether this would be part of the situations covered by 'the original specification'...)
Comment #276pfournier CreditAttribution: pfournier commented
My definition of Drupal is Drupal core. I also checked pathauto, widely used, and it never updates a path without a pid.
Of course, as you say, there could be some module somewhere that relies on this UPDATE. However, I think making this change would increase the overall happiness of the Drupal community :-).
To make sure there is always a pid, we could modify path_nodeapi('load') to set $node->pid (I wonder why this isn't done; does someone has an explanation for that?).
Comment #277roderik CreditAttribution: roderik commented
I think your 2 first paragraphs neatly sum up the 'contention' here. Let's say that if the D6 maintainer agrees with this, the UPDATE statement can be scrapped.
As for the explanation: I can only guess, but I think it has something to do with the fact that a node can have several path aliases (and that this isn't reflected in node properties. I am guessing it was never hammered out completely.)
Comment #278keithm CreditAttribution: keithm commented
Comment #279infojunkie CreditAttribution: infojunkie commented
Hello pfournier!
Trying patch #275 and working fine for me so far. I'm using pathauto + i18nsync.
Comment #280roderik CreditAttribution: roderik commented
...2 months passed...
So like,
This issue is big, causing confusion in lots of places, and watched by many,
The patches have been tested extensively. So it really is RTBC already. But I suspect noone dares to touch the thing anymore ;)
The only issue left is the intended behavior of the code (in edge cases only, outside of node edit forms / pathauto). This IMHO has been discussed enough and just needs a comment/decision by Gabor.
So now -
do I set my own patch RTBC with a concise summary for Gabor (about when/why he'd want to set 'needs work' again, to implement what pfournier says)?
Comment #281dboulet CreditAttribution: dboulet commented
Patch in #275 works for me, what is needed to have this committed?
Comment #282infojunkie CreditAttribution: infojunkie commented
|Needs review
|» Reviewed & tested by the community
Comment #283dimitriseng CreditAttribution: dimitriseng commented
Subscribing. I am using Drupal 6.22, Pathauto 6.x-1.5 and i18n 6.x-1.9 with i18n sync enabled.
I have done some testing with both #275 and #218 and they both seem to be fixing the issue. Is #275 the final patch for this? If yes, has this been commited yet? Thank you very much.
Comment #284GiorgosK CreditAttribution: GiorgosK commented
#275 works as expected
please commit
big thorn on any installation
Comment #285tangent CreditAttribution: tangent commented
Comment #286guillaumev CreditAttribution: guillaumev commented
Confirming that #275 works with i18n 6.x-1.9, pathauto 6.x-1.5 and Drupal 6.22. Would be great to see this included in Drupal 6.23 :-)
Comment #287farrington CreditAttribution: farrington commented
Comment #288Toktik CreditAttribution: Toktik commented
#275 works as expected.
Please commit.
Comment #289checker CreditAttribution: checker commented
confirm #275 works
Comment #290jamonation CreditAttribution: jamonation commented
Comment #291goofrider CreditAttribution: goofrider commented
confirm #275 works, Drupal core 6.22, please commit
Comment #292Paracetamol CreditAttribution: Paracetamol commented
Comment #293Sborsody CreditAttribution: Sborsody commented
Comment #294mdorrell CreditAttribution: mdorrell commented
One warning to anyone who is using Pressflow and using the path_alias_cache module. You will need to patch that one as well. I won't paste a patch here and confuse people, but in the function path_alias_cache_lookup_path you will need to change the ORDER BY clauses in the SQL statements to "ORDER BY language DESC, pid DESC".
Comment #295greggles CreditAttribution: greggles commented
Comment #296liquidcms CreditAttribution: liquidcms commented
#275 works for me as well with 6.22 - would be nice to get this committed.
Comment #297clashar CreditAttribution: clashar commented
+1 for porting to D7
Comment #298klonos CreditAttribution: klonos commented
Yeah, is this ever going to happen for D7/D8?
Comment #299roderik CreditAttribution: roderik commented
Above is a bugfix to code which has been completely redone (i.e. does not exist anymore) in D7.
I'm pretty sure that if you see the same faulty behavior in D7 (which I doubt you do), you're better off opening a separate issue.
Comment #300clashar CreditAttribution: clashar commented
For D7 I've opened an issue for pathauto, but maybe it's a Core issue
#1236030: interface language change the path of the node