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.

status:

(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.)

Files: 
CommentFileSizeAuthor
#320 core-module-path-path_set_alias-269877-320-D6.patch4.82 KBroderik
#275 core-module-path-path_set_alias-269877-275-D6.patch4.82 KBroderik
#270 core-module-path-path_set_alias-269877-270-D6.patch4.81 KBroderik
#264 core-module-path-path_set_alias-269877-264-D6.patch4.79 KBroderik
#263 core-module-path-path_set_alias-269877-263-D6.patch5.03 KBroderik
#261 core-module-path-path_set_alias-269877-261-D6.patch4.48 KBroderik
#242 path_alias.png17.42 KBDanny_Joris
#228 path_alias_lang.patch1.99 KBakaliel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path_alias_lang.patch. View
#227 path_alias_2.patch1.7 KBakaliel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch path_alias_2_1.patch. View
#218 core-module-path-path_set_alias-269877-218-D6.patch2.61 KBfmjrey
#217 path-path_set_alias-D6.patch2.59 KBufku
#209 drupal_6_path_lang.patch1.34 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_6_path_lang.patch. View
#188 path-path_set_alias-269877-187-D6.patch2.59 KBpfournier
#185 path_module.patch2.59 KBpfournier
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path_module_3.patch. View
#184 path_module.patch2.85 KBpfournier
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path_module_2.patch. View
#183 path_module_269877#183.patch2.85 KBpfournier
FAILED: [[SimpleTest]]: [MySQL] Fetch test file: failed to retrieve [path_module_269877#183.patch] from project client. View
#146 path-module_269877-D6.patch1.21 KBmathieu
#137 path-module_269877-D6.patch1.07 KBjct
#135 path-module_269877-D6.patch1.07 KBmathieu
#91 path-module-D7_with tests.patch3.37 KBneochief
FAILED: [[SimpleTest]]: [MySQL] Fetch test file: failed to retrieve [path-module-D7_with tests.patch] from project client. View
#85 path-module-D6.patch937 bytesneochief
#85 path-module-D7.patch935 bytesneochief
Invalid patch format in path-module-D7.patch. View
#82 path-module_D6.patch1.2 KBneochief
#82 path-module_D7.patch1.18 KBneochief
Failed: Invalid PHP syntax in path.module . View
#78 path-module_D6.patch1.17 KBneochief
#78 path-module_D7.patch1.16 KBneochief
Failed: Failed to apply patch. View
#69 path.module_neochief.patch1.01 KBneochief
Failed: Failed to apply patch. View
#69 path.module_damien.patch960 bytesneochief
Failed: Failed to apply patch. View
#53 path_module.patch1.18 KBneochief
Failed: Failed to apply patch. View
#62 path_module_269877.patch755 bytesJo Wouters
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in path_module_269877.patch. View
#30 path.module-path-set-alias.patch1.06 KBnonsie
Failed: Failed to apply patch. View

Comments

greggles’s picture

Status: Active » Postponed (maintainer needs more info)

Do you have any steps for how to repeat this?

Steve Dondley’s picture

No, just started happening.

greggles’s picture

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...

Freso’s picture

@Steve Dondley: What other modules are you using?

xanga2008’s picture

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?

greggles’s picture

Project: Pathauto » Category
Version: 6.x-1.0 » 6.x-2.0-alpha1
Status: Postponed (maintainer needs more info) » Active

it started after i have installed the latest category module.

That explains it a bit...

keesje’s picture

Project: Category » Drupal core
Version: 6.x-2.0-alpha1 » 6.3
Component: Code » path.module

Hi,

I have the same issue (I believe).

Message:

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.

Kees

keesje’s picture

Component: path.module » translation.module

If someone has a better clue wich project/component is responsible, please shift.

John.Mulder’s picture

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.

greggles’s picture

Project: Drupal core » Category
Version: 6.3 » 6.x-2.0-alpha3
Component: translation.module » Wrapper modules

@qrios, @john.mulder - do you use the Category module?

This is an issue with the Category module, afaik.

John.Mulder’s picture

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.
John

greggles’s picture

Project: Category » Pathauto
Version: 6.x-2.0-alpha3 » 6.x-1.x-dev
Component: Wrapper modules » Code
Status: Active » 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?

Steve Dondley’s picture

I am not using the category module.

John.Mulder’s picture

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.

Cheers,
John

Steve Dondley’s picture

Yes, this is the behavior I'm seeing. I only get the error when submitting content. I do not get an error when automatic alias is unchecked.

Steve Dondley’s picture

I looked at the other modules, there is no other that I have that I think could affect pathauto. Non-core modules are:

admin menu
image
fckeditor
nice menus
token
webform
views
view ui

BTW, I have about 5 other very lightly used drupal 6 site but have not seen this problem. But those sites are not administered much.

greggles’s picture

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?

keesje’s picture

@qrios, @john.mulder - do you use the Category module?

No, Im not.

Steve Dondley’s picture

Everything is up to date.

John.Mulder’s picture

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 ;)

Cheers,
John

Steve Dondley’s picture

Everything is up to date.

nonsie’s picture

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.

greggles’s picture

Is everyone else having this problem using content translation module?

Steve Dondley’s picture

I'm not using the translation module.

Freso’s picture

@Steve Dondley (and everyone else answering #23):
How about the Locale module?

Steve Dondley’s picture

No, locale module either.

keesje’s picture

"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.

nonsie’s picture

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?

Freso’s picture

Title: mysql errors when saving a page » path_set_alias() doesn't account for same alias in different languages
Project: Pathauto » Drupal core
Version: 6.x-1.x-dev » 6.x-dev
Component: Code » path.module
Status: Postponed (maintainer needs more info) » Active

Well, 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. :)

nonsie’s picture

Status: Active » Needs review
FileSize
1.06 KB
Failed: Failed to apply patch. View

Attached patch fixes part of the problem - setting correct language alias for nodes. Still looking into taxonomy duplicate alias issue.

scottrigby’s picture

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

nonsie’s picture

does the patch in #30 remove the error for nodes?

scottrigby’s picture

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...

Freso’s picture

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? >_>

nonsie’s picture

hmm, I don't think there were any changes in path.module in 6.4 (at least they are not in the patch file)

scottrigby’s picture

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?

wdrupal100’s picture

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

keesje’s picture

"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.

Alauddin’s picture

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

nonsie’s picture

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.

earnie’s picture

Status: 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

nonsie’s picture

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.

earnie’s picture

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.

nonsie’s picture

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?

earnie’s picture

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');
}
nonsie’s picture

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

earnie’s picture

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.

Manuel Garcia’s picture

Also having this issue, subscribing

kesmeby’s picture

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.

greggles’s picture

@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.

Maciej Lukianski’s picture

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

Cheers

davexd’s picture

@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 :)

neochief’s picture

FileSize
1.18 KB
Failed: Failed to apply patch. View

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.

Jo Wouters’s picture

Status: 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.

earnie’s picture

Status: Needs review » Reviewed & tested by the community

per #54 this is tested and the patch code looks good.

Damien Tournoud’s picture

Status: 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.

Jo Wouters’s picture

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);
+      }
     }
Damien Tournoud’s picture

@Jo Wouters: the if ($language) check is useless. The only thing missing is a condition on 'language' in the update query.

Jo Wouters’s picture

@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 :-)

nonsie’s picture

I agree with comment in #59.
I've been using my own patch from #30 since I posted it here without any problems.

Damien Tournoud’s picture

@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.

Jo Wouters’s picture

Status: Needs work » Needs review
FileSize
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.

neochief’s picture

How about my changes in #53?

Damien Tournoud’s picture

@Jo Wouters: this is starting to looks great.

@neochief: it's unclear to me why your change can ever be needed. Please elaborate.

neochief’s picture

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.

Damien Tournoud’s picture

@neochief, may be that's a bug in pathauto, then :) Please elaborate on why the change is needed.

neochief’s picture

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().

Damien Tournoud’s picture

Status: 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.

neochief’s picture

FileSize
960 bytes
Failed: Failed to apply patch. View
1.01 KB
Failed: Failed to apply patch. View

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).

two2the8’s picture

Hey there,

I see the same user warning whenever I edit & save an existing taxonomy term or node:

Nodes:
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.

neochief’s picture

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.

neochief’s picture

Status: Needs work » Needs review
two2the8’s picture

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.

solutiondrop’s picture

Thanks neochief. The patch(s) cleared up the problem for me. I have noticed anything wrong in the last day.

Encarte’s picture

subscribing

momper’s picture

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?

earnie’s picture

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.

neochief’s picture

Version: 6.x-dev » 7.x-dev
FileSize
1.16 KB
Failed: Failed to apply patch. View
1.17 KB

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)

Status: Needs review » Needs work

The last submitted patch failed testing.

neochief’s picture

Damn, I hate Testbed and it seems that it hates me too :) Patch is applying cleanly to D7 HEAD, any ideas why it failed?

ainigma32’s picture

any ideas why it failed

Have you tried creating diff using Drupal's root as the diff's root?

- Arie

neochief’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
Failed: Invalid PHP syntax in path.module . View
1.2 KB

Ah, yes, Arie, your totally right! Here are new ones.

Status: Needs review » Needs work

The last submitted patch failed testing.

greggles’s picture

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

neochief’s picture

Status: Needs work » Needs review
FileSize
935 bytes
Invalid patch format in path-module-D7.patch. View
937 bytes

Updated.

neochief’s picture

Status: Needs review » Reviewed & tested by the community

Great! I think it can be pushed to RTBC now, doesn't it?

neochief’s picture

Priority: Normal » Critical
webchick’s picture

Status: 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.

Additionally:

+      // 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?

neochief’s picture

webchick’s picture

Yes. I want it in the source code, not in the issue queue. :)

neochief’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
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 :)

neochief’s picture

Hey, could comeone from these 90 comments finally review this patch and mark it as RTBC? :)

Damien Tournoud’s picture

Status: 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.
$alias);
       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.

earnie’s picture

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.

Damien Tournoud’s picture

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?

earnie’s picture

Sorry, I meant the src column is changing to update an existing dst for a new src.

telemako’s picture

Hello, I have a problem related to this issue too.
I posted this:
http://drupal.org/node/439668
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.
thanks

telemako’s picture

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.

neochief’s picture

telemako, did you tried #85?

neochief’s picture

@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.

telemako’s picture

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?
thanks

neochief’s picture

@telemako, Applying patches

GiorgosK’s picture

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 ?

hargobind’s picture

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.

nonsie’s picture

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);
 

to

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

mathieu’s picture

Subscribing - having similar issue.

neochief’s picture

I can state that the patch for D7 isn't working now, as database layer got major changes from that time.

mvc’s picture

Patch in #85 on 6.11 fixed my problem (without the modification mentioned in #105). Thanks, neochief!

asb’s picture

nonsie’s picture

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.

RikiB’s picture

#85 fixed me up :)

Lars Pohlmann’s picture

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...

jct’s picture

Status: 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?

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

neochief’s picture

this is 7.x issue queue. After it will be fixed in D7, we can backport and commit it to D6

asb’s picture

> 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

neochief’s picture

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.

asb’s picture

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

rmmcclay’s picture

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.

RikiB’s picture

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.

logicalpat’s picture

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.

GiorgosK’s picture

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)

jseffel’s picture

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.

RikiB’s picture

hmm, I just tripple checked and mine seems patched but Im still seeing the error. Strange.

miro_dietiker’s picture

Ran in to this very same issue... greetings to the issue tracker.

harrrrrrr’s picture

worked for me

DamienMcKenna’s picture

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.

hanoii’s picture

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.

DamienMcKenna’s picture

What would it take to get this patch rolled into a 6.x release? What further testing is required?

mathieu’s picture

Version: 7.x-dev » 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.

hanoii’s picture

I actually tried #85, and it worked, currently using it fine so far.

GiorgosK’s picture

Status: 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" ?

NiklasBr’s picture

Subscribing.

Status: Reviewed & tested by the community » Needs review

jct requested that failed test be re-tested.

mathieu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.07 KB

Re-submit of same file as in #85, to prevent testbot from changing status (is this how it works?) and hopefully moving this forward.

jct’s picture

Status: 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 :)

jct’s picture

FileSize
1.07 KB

One more try w/ attachment

mathieu’s picture

Status: 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?

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: 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.

sun.core’s picture

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.

marcoBauli’s picture

same issue on different sites (6.x)

dafeder’s picture

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.

catch’s picture

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.

nwe_44’s picture

subscribing

catch’s picture

Version: 7.x-dev » 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.

mathieu’s picture

Status: Needs work » Needs review
FileSize
1.21 KB

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?

klonos’s picture

subscribing

DarrellDuane’s picture

+1

mvc’s picture

Status: Needs review » Reviewed & tested by the community

works for me. thanks!

Mitson’s picture

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!

klonos’s picture

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 ;)

Gábor Hojtsy’s picture

Status: 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?

doublewhopper’s picture

Thank you very much!

ddanier’s picture

subscribing

b3liev3’s picture

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'));

if($pid){
path_set_alias('node/4/20', $path, $pid, 'en');
}else{
path_set_alias('node/4/20', $path, null, 'en');
}

klonos’s picture

@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)

b3liev3’s picture

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 :)

klonos’s picture

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.

blueblade’s picture

subscribing

klonos’s picture

@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.

mathieu’s picture

@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.

MyXelf’s picture

Subscribing...

SHAAAAAA’s picture

Status: Needs work » Needs review

#62: path_module_269877.patch queued for re-testing.

marcushenningsen’s picture

Status: Needs review » Reviewed & tested by the community

The patch works for me. Thanks a lot.

miro_dietiker’s picture

Status: 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".

klonos’s picture

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.

earnie’s picture

Status: 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.

Kasper Souren’s picture

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.

ufku’s picture

the patch works!

klonos’s picture

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 (?)

NiklasBr’s picture

Thanks, patch seems to be working on the one installation I tested as well. Let us hope for inclusion in D6.18 then.

Marko B’s picture

Tested #62 works fine. Bulk alias still is a mess and doesnt work, but i guess this is different issue.

klonos’s picture

@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?

Marko B’s picture

Yes i ment that :-) confirming its not working ok, recomend #62.

klonos’s picture

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.

Marko B’s picture

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 :)

klonos’s picture

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.

Marko B’s picture

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.

klonos’s picture

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?

Marko B’s picture

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? :-)

klonos’s picture

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.

klonos’s picture

... + 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 ;)

pfournier’s picture

FileSize
2.85 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test file: failed to retrieve [path_module_269877#183.patch] from project client. View

Hi,

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.

pfournier’s picture

FileSize
2.85 KB
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.

pfournier’s picture

FileSize
2.59 KB
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.

miro_dietiker’s picture

Status: 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.

Amarjit’s picture

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.

pfournier’s picture

Thanks Miro, I was not aware of that. Here is the correctly named patch file.

ufku’s picture

Status: Needs review » Reviewed & tested by the community
pfournier’s picture

Status: 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.

klonos’s picture

well... the patch needs to be named path-path_set_alias-269877-188-D6.patch

;)

checker’s picture

Subscribing

bbenone’s picture

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....)

(2)
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...

klonos’s picture

@bbenone: what is your language setting in ../admin/settings/language/configure ?

bbenone’s picture

@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.

klonos’s picture

ok then, can you please try the patch from #146 and let us know if that one solves the issue you are having?

magnestyuk’s picture

patch in #188 works for me.

pfournier’s picture

@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?

Patrick

bbenone’s picture

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?

Thanks...

mrfelton’s picture

@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.

mrfelton’s picture

@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?!

pfournier’s picture

@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.

mrfelton’s picture

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.

rup3rt’s picture

subscribe

spylvas’s picture

subscribe

pfournier’s picture

#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)

mdorrell’s picture

I'm using the patch from #188. This is working for us. What is the status of this issue?

ayalon’s picture

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.

fago’s picture

FileSize
1.34 KB
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.

pfournier’s picture

@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).

fmjrey’s picture

A patch submitted yesterday in issue #358722: Node aliases lost/changed when using i18n synchronize translations could also fix this one here:
http://drupal.org/comment/reply/358722#comment-3347984

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.

John T. Haller’s picture

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.

fago’s picture

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.

ayalon’s picture

@fago: your patch works but it does not solve the problem with i18n_sync decribed here:
http://drupal.org/node/358722

If you use i18n_sync, you also have to apply the patch here:
http://drupal.org/node/358722#comment-3347984

fago’s picture

I don't know about pathauto, but it solves the problem for i18n_sync + the path module.

AdrianB’s picture

Subscribing.

ufku’s picture

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.

fmjrey’s picture

Same patch as #217 but in a standard form.
ufku: could you tell us more about your patch?

ufku’s picture

@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.

fmjrey’s picture

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

GiorgosK’s picture

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

gnindl’s picture

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.

pfournier’s picture

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).

koorneef’s picture

Just my 2 cents: the patch in #209 by Fago works for me.
I don't use i18n sync anymore.

Thanks !

blaiz’s picture

Subscribing.

j0nathan’s picture

Subscribing.

akaliel’s picture

FileSize
1.7 KB
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.

akaliel’s picture

FileSize
1.99 KB
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!

GiorgosK’s picture

@akaliel
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 !!

@EVERYONE
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
--------------------------------------
1.
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"
--------------------------------------
2.
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"
--------------------------------------
3.
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)

aquila’s picture

Subscribing to this quite horrible issue.

aquila’s picture

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?

fago’s picture

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 node_save().

aquila’s picture

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!

GiorgosK’s picture

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"

1
admin/settings/language/edit/en "path prefix"=""

1a.
node "abcd" - en - alias "abcd" - view at "abcd"
node "abcd" - el - alias "abcd" - view at "el/abcd"

1b.
node "abcd" - en - alias "abcd" - view at "abcd"
node "abcd" - neutral - alias "abcd" - view at "abcd"

1c.
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"

2.
admin/settings/language/edit/en "path prefix"="en"

2a.
node "abcd" - en - alias "abcd" - view at "en/abcd"
node "abcd" - el - alias "abcd" - view at "el/abcd"

2b.
node "abcd" - en - alias "abcd" - view at "en/abcd"
node "abcd" - neutral - alias "abcd" - view at "abcd"

2c.
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

ayalon’s picture

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...

GiorgosK’s picture

@ayalon
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

neochief’s picture

Status: 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.

klonos’s picture

+ for moving on with this too and handle the other issue in its own queue.

aquila’s picture

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.

kuzyakiev’s picture

Subscribing.

Danny_Joris’s picture

Hello,

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.

Danny_Joris’s picture

FileSize
17.42 KB

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.

klonos’s picture

Are #218 and #228 meant to be used/tested in combination? If so, can someone please merge them in one?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, path_alias_lang.patch, failed testing.

BenMirkhah’s picture

Subscribing

BenMirkhah’s picture

#209 worked for me, thanks fago

ikeigenwijs’s picture

subscribing

haffmans’s picture

Status: 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.

neochief’s picture

I think so too.

GiorgosK’s picture

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

sylvain_a’s picture

Suscribe

philipz’s picture

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.

roderik’s picture

Status: Needs review » Reviewed & tested by the community

@fago
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.

roderik’s picture

Status: 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.)

Right?

roderik’s picture

Assigned: Unassigned » roderik
Status: Reviewed & tested by the community » Needs work

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

pfournier’s picture

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.

roderik’s picture

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.)

pfournier’s picture

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.

mike dodd’s picture

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.

roderik’s picture

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.

roderik’s picture

Assigned: Unassigned » roderik
Status: Needs review » Needs work
FileSize
4.48 KB

*message deleted*
better patch in #263.

roderik’s picture

Assigned: roderik » Unassigned
Status: Needs work » Needs review

.

roderik’s picture

Assigned: roderik » Unassigned
Status: Needs work » Needs review
FileSize
5.03 KB

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.)

roderik’s picture

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'? :) )

klonos’s picture

...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 ;)

roderik’s picture

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 ;) )

pfournier’s picture

@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.

roderik’s picture

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'...

pfournier’s picture

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.

roderik’s picture

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.

pfournier’s picture

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.

roderik’s picture

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.

pfournier’s picture

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.

pfournier’s picture

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.

roderik’s picture

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 );
do_whatever();
$node->language = 'en';
node_save($node);

You say "that piece of code should just pass a $pid for that." End of discussion.
Right?

(I guess we are disagreeing on whether this would be part of the situations covered by 'the original specification'...)

pfournier’s picture

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?).

roderik’s picture

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.)

keithm’s picture

subscribe

infojunkie’s picture

Hello pfournier!

Trying patch #275 and working fine for me so far. I'm using pathauto + i18nsync.

roderik’s picture

...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)?

dboulet’s picture

Patch in #275 works for me, what is needed to have this committed?

infojunkie’s picture

Status: Needs review » Reviewed & tested by the community
dimitriseng’s picture

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.

GiorgosK’s picture

#275 works as expected
please commit
big thorn on any installation

tangent’s picture

subscribing

guillaumev’s picture

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 :-)

farrington’s picture

subscribe

Toktik’s picture

#275 works as expected.

Please commit.

checker’s picture

confirm #275 works

jamonation’s picture

subscribe

goofrider’s picture

confirm #275 works, Drupal core 6.22, please commit

Paracetamol’s picture

subscribe

Sborsody’s picture

subscribe

mdorrell’s picture

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".

greggles’s picture

@mdorrell, the place for that is https://bugs.launchpad.net/pressflow/+filebug

liquidcms’s picture

#275 works for me as well with 6.22 - would be nice to get this committed.

clashar’s picture

+1 for porting to D7

klonos’s picture

Yeah, is this ever going to happen for D7/D8?

roderik’s picture

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.

clashar’s picture

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

Pages