Path alias saving is not language-aware

Tested before the patch, and lo, path.module is indeed quite broken. With the patch, things are much more sane, and mostly working fine. In clicking through the UI to add, edit, and remove path aliases, I found 2 minor problems, only 1 of which should hold up this patch:

1) The question on the confirm page for deleting the alias is wonky:
Are you sure you want to delete path alias welcome? from path node/1?
There's an extra ? in there... this should be fixed for this patch (it's trivial).

2) The breadcrumbs are wrong on admin/build/path/edit/% (when editing an existing alias).
This should happen later. There are a bunch of breadcrumb issues in D6 right now, so no reason to make this patch any larger than it already is.

In reviewing the patch, the only other problem I noticed is that we remove the call to drupal_clear_path_cache() but never seem to re-add it. Looks like that belongs in path_admin_delete_confirm_post(), no?

I'll re-roll for the punctuation and drupal_clear_path_cache() and re-post in a moment... stay tuned.

upon further discussion w/ hunmonk in IRC and more testing, things work fine for the admin UI, but removing path aliases at node/%nid/edit is still broken. stay tuned.

I guess I should mention... I tested that quite thoroughly for adding/removing path aliases from node/%nid/edit and from the admin UI, and all is working fine. Probably this is RTBC, but it's now my patch, too, so someone else should give it a look. Thanks.

I've done a bit of testing with this patch, path admin additions/modifications/deletions and path additions/modifications from node/edit all seem to work properly.
The code appears OK, so RTBC

Here is the patch at #4 rerolled without dapi stuff.
In summary attached patch :

1. removes unnecessary access checks for the admin UI paths.
2. fixes a broken path add/edit form
3. makes path and alias fields required on the add/edit form. previously there was a weird "if it's empty auto-delete it" kind of functionality in place, which was confusing and unnecessary.
4. removes some garbage in path_set_alias which, to hunmonk's examination, was unnecessary, and also causing endless loops on certain path edits.
5. fixes bad usage of path_set_alias() in path_nodeapi -- this was never properly updated from the path language patch.
6. Fixes deletion of path aliases at node/%nid/edit

Plus:

• Fixes deletion of path aliases from admin UI (it was fixed in http://drupal.org/node/154046 , but that fix has been lost after dapi rollback)
• Fixes admin UI path filtering which was broken.
• Adds a key to reset the path filtering when a keyword is active (I think it is quite useful and is the same behavior as the content management admin UI filtering)

I tested this quite thoroughly for adding/editing/removing path aliases from node/%nid/edit and from the admin UI;
also tested path alias filtering from the admin UI and all is working fine.

Hunk failed after http://drupal.org/node/156910 committed to HEAD -- Code style clean-up: indentation

Rerolled.

Most of the glitches are fixed in the patch at http://drupal.org/node/147143 Including proper deletion api rollback, $form_state variable, bad usage of path_set_alias in path_nodeapi(to delete path for all languages, also other changes), removing some garbage in path_set_alias. Just the Reset button is not covered, not sure if we need; it's a nice one to have though.

Trying to get attention to that patch: http://drupal.org/node/147143 It really needs reviews....

Thanks in advance!
:D

patching file modules/path/path.module
Hunk #1 FAILED at 73.
Hunk #2 FAILED at 89.
Hunk #3 FAILED at 105.
Hunk #4 succeeded at 75 (offset -39 lines).
Hunk #6 FAILED at 176.
Hunk #7 FAILED at 186.
Hunk #8 succeeded at 132 (offset -81 lines).
Hunk #10 FAILED at 234.
Hunk #11 FAILED at 397.
Hunk #12 FAILED at 408.
8 out of 12 hunks FAILED -- saving rejects to file modules/path/path.module.rej

• On the path edit form we make the original path and the destination requried.
• Same form, check for the existence of the source.
• The filter form redirect is not converted to fapi3.
• Same form lacks a reset button.
• removes some ancient garbage in path_set_alias which, to hunmonk's examination, was unnecessary, and also causing endless loops on certain path edits.
• fixes bad usage of path_set_alias() in path_nodeapi -- this was never properly updated from the path language patch.
• Fixes deletion of path aliases at node/%nid/edit

I have reused some code for these from the existing patch above but some of it is totally new (and fixed because even the patch was broken).

Patch gives a fatal error because function 'path_admin_filter_form_submit_filter' was double declared; second one should be 'path_admin_filter_form_submit_reset'. After altering that I tested the patch.

Fixes deletion of path aliases at node/%nid/edit:

Does not work correct when the alias is created on 'admin/build/path'. In that case it must first be altered one time on 'node/%/edit' and then it can be deleted on 'node/%/edit'; else the alias will simply stay. Deleting directly from the admin-page works correct.

Reason is that saving from the admin page does not save the language and 'path_set_alias' checks for that to delete an entry. After editing a node-path via 'node/%/edit' the language is set and the alias can be deleted.

All the rest seems to work well.

Oh, I want to add that this problem only exists when a language is set for a node. If the node is 'language neutral' deletion is working well.

As per previous post - patch produces:

Fatal error: Cannot redeclare path_admin_filter_form_submit_filter() (previously declared in E:\www\htdocs\drupal\modules\path\path.admin.inc:222) in E:\www\htdocs\drupal\modules\path\path.admin.inc on line 231

As far as I see, the fatal redeclaration is right in the patch:

+function path_admin_filter_form_submit_filter($form, &$form_state) {
+  $form_state['redirect'] = 'admin/build/path/list/'. trim($form_state['values']['filter']);
+}
+
+/**
+ * Process filter form submission when the Reset button is pressed.
  */
-function path_admin_filter_form_submit($form, &$form_state) {
-  return 'admin/build/path/list/'. trim($form_state['values']['filter']);
+function path_admin_filter_form_submit_filter($form, &$form_state) {

Also by "Reason is that saving from the admin page does not save the language" you mean the path admin page does not save the language? It should do that.

Gábor,

There are some 'evedence' about language awareness in the code; but not in practical :-)

I spoke with Chx on Irc and I took myself some time to analize the problem; but at firtst site the problem looks big to me. When I reply to an issue i like to provide a patch for it but at the moment I'm not capable to do that.

I'll come back to it; I'll promise, but it needs some time.

Nick

I found part of the problem.

The 'form_alter' in locale.module was not correct and the language selection was not added to the form.

Remaining is that it's still not possible to delete a path alias from a node/%/edit when it is in a specific language while the alias created as admin is 'language-neutral'.

Attached a patch that fixes the function-collision in path.admin.inc and a correction for form_alter in locale.module

I set it 'code needs review' although the issue needs work. But what the patch from Chx is doing; it's doing well. I just corrected the function-collision and made the form appear again. I will continue with the node/%/edit problem.

Not tested too much but it seems to work well; I altered 'path_set_alias' to delete in-depend of language because the whole language-stuff is already dealt with while loading the form at node/%/edit.

Attached a new patch that should solve all issues.

Seems to work here too. Tested with different languages, and different nodes.
Nice patch 'noticable guy'! ;-)

Nicely done indeed!

Indeed as it seems path module forms were changed without locale form_alter being changed. Unfortunate. Also path saving was not using the language when done on node save, that was a missing feature for sure.

What I am puzzled about in this patch is that you basically removed the language condition on path alias deletion. This is quite dangerous. If you have aliases or four languages and you are about to modify one, removing all the aliases seems to be a fatal error. I am talking about:

 function path_set_alias($path = NULL, $alias = NULL, $pid = NULL, $language = '') {
   if ($path && !$alias) {
     // Delete based on path
-    db_query("DELETE FROM {url_alias} WHERE src = '%s' AND language = '%s'", $path, $language);
+    db_query("DELETE FROM {url_alias} WHERE src = '%s'", $path);
     drupal_clear_path_cache();
   }

This is an API function. If someone calls it with path_set_alias('contact', NULL, NULL, 'hu'), then the intention is to delete the Hungarian alias, not to delete ALL aliases. This also has effects on changes elsewhere in the function, which you simplified by introducing this change.

Hmmm, difficult....

I don't understand the 'concept' of multi-language aliasses. At least not as implemented now.

Example:

• I have a two language site: English (main) and Dutch
• I create an english node; it will be node/1 and is english
• I create 2 aliasses for that:
• - dutch: mijn_toffe_node
• - english: my_great_node

All works well; when changing the site to Dutch the Dutch alias is used. But; when I'm going to edit my node I see the English alias again in the form. There is no way in the world I can change the Dutch alias unless in the admin screen.

Example continue:

I remove all the aliases in the admin screen and make a new 'language neutral' one. This works fine; but when I go back to edit again it's impossible to delete the alias because function function path_set_alias() is language aware and will only remove an English alias.

Conclusion:

Gábor; you are right that my solution is not correct. But as both examples above shown there are fundamental problems in the implementation:

1. multi-aliases create and edit only available in the admin page
2. a node/%/edit shows a 'random' alias (well; not really; but at least it is confusing).
3. deletion of a 'neutral' alias from a language-node does not work. You first have to 'edit/save' what makes it basically a language-alias that can be deleted.

Proposal:

I added the same patch as yesterday; but removed the function path_set_alias() simplification. There are some very valuable changes in it and it did not create the language-problem in the first place. It would be nice if this part can be committed.

Then a new issue should be created with a better 'edit' form. Just thinking at loud; but it looks the best to me to make all alias in all languages available; basically removing the dependancy of the admin screen.

Nick

Well, the concept of multilanguage aliases are geared towards the other types of paths, not the node paths per say. Let's say you have contact module enabled. Then you have /contact, which you need to alias to different languages. Let's say you have English and German set up, and the path prefixes are 'en' and 'de' (just like the default). So you have /en/contact and /de/contact (if you have the mandatory path prefix setting enabled). Now you can alias 'contact' to a German word, like 'ferbindung', so you have /en/contact and /de/ferbindung. When German links are printed (ie. when the /de/ prefix is put there), the alias lookup also gets that a German alias should be looked up.

For nodes, we expect all nodes to be in a specific language, or be language neutral. So having aliases for a node in more then one language is an "interesting idea". This might be applicable to language neutral nodes for some reason (although they can get language neutral aliases, because we expect the whole node to be language neutral with all information attached). When you enable node translation, you get different nodes for different language versions, so different language aliases correspond to different node paths.

Gábor; thanks for clarification; now I understand. Same like Chx told me on Irc I'm not suppose to create multi-language-aliases for a language-node.

You call it an 'interesting idea' and Chx told me I should simply not do that. I agree with Chx and the only reason I made those aliases was for testing purposes. I don't see any use in it; but as it is possible it has to be tested what will happen.

Anyway; I still want to fix this issue and as you stated that 'for nodes, we expect all nodes to be in a specific language'. Well; let's not just 'expect' that but just allow 1 alias for a node; were the language is set in the $node->language and the allias itself will be neutral.

To quote the documentation in path.module:

/**
 * Implementation of hook_nodeapi().
 *
 * Allows URL aliases for nodes to be specified at node edit time rather
 * than through the administrative interface.
 */
 

So path_nodeapi is only for editing a node; so in my opinion the 'language awareness' of that api can be dumped. The node itself is already language-aware. Then, in the admin form there should be a validation that there can be only 1 alias for a node.

This should fix all the problems:

• All nodes will be in a specific language; because there will be 1 alias maximum
• A node can always be deleted in node/%/edit
• No confusion in node/%/edit
• No dependency on the admin screen for node-aliasing under some circumstances
• No 'mis-use' possible of the language-alias-possibilities like I did :-)
• non-node-pages will still work as now; like the 'contact' in your example
• In case a node will be altered from 'language-neutral' to 'Dutch' there is no problem with the alias. This is a real example; I have a site now that is a mix of English and Dutch on D5. When upgrading to D6; all nodes will be neutral in the beginning but I change them to the corresponding language after the update. This will be a nightmare for aliases as currently handled.

Patch is forthcoming although I would like to hear some response about this concept before I spend too much time for implementing something that might be not correct.

Nick

Having a neutral alias for a Dutch node might seem like a good idea from the outset, but it is confusing when you get to the path alias admin page. When you filter by Dutch, you expect your Dutch node aliases to show up. When you filter by language neutral, you don't expect your Dutch node aliases to show up.

Also Drupal 6 as it stands now does not disallow nodes to be in multiple languages, it just does not support it by default. So we kept the door open for contributed modules to implement multilanguage nodes (as opposed to different language nodes related), if they want to. This is shaky ground of course, but up until now, I don't know of anything which would stop contrib modules to do such an implementation. By blocking path aliases, we block some of the possibilities out.

I think it is enough if we always edit the alias of the node in the language the node is in on the editing page and we don't care about other aliases being possible or not.

- when a node is created, we save the alias with the language associated to the node
- when a node is deleted, we remove the alias in the language associated to the node
- when a node is edited, but language is kept, we update the alias
- when a node is edited, but language is modified, we remove the previous alias and add the new one

The last one is tricky, if we need to think about multiple aliases possible for whatever reason, so in case the new source and language already have a target alias, we overwrite what is in there I guess.

Ok; good point about the filtering. The rest... about keeping the possibilities open; as path.module behaves now I don't see that happening :-)

But I don't like to discuss that; I want to fix this issue.

Function drupal_lookup_path() falls back on an alias without language. Wow; function path_set_alias() should do the same and then the only problem in my review in #15 of Chx's patch is fixed.

So I did and attached a new patch. Now it's possible to add as many alias as you like in the admin screen in whatever language or neutral and node/%/edit will only deal with it's own language and falls back on language neutral. Other languages are not affected at all.

When you change a language the alias is already properly updated.

Tested the patch and it works as expected.
* filter is working
* reset is working
* unknown system paths throws error message

Thanks for this patch and feature!

Though I marked the previous patch RTBC, I did some further development and added the ability to delete multiple aliases at once (screenshots: #1, #2, #3).

Maybe its a good addition.

Tested stBorchert's patch:

Add alias works
delete works
multiple delete works (and just made my day)
filter and reset works.

All clean, first time I've tried the patch and all the changes are very, very nice compared to 5.x.

Still RTBC.

Just a quick code style review (I've not tested the actual patch):

1. // switch is redundant.
2. Code comments should have proper punctuation (e.g Implementation of hook_theme()).
3. A newline after array('data' => t('System'), 'field' => 'src') should go.
4. Spacing problems after return confirm_form($form,
5. The doxygen header for path_theme() is formatted wrong

1. // switch is redundant. Ok, I've inserted it for future features.
2. Code comments should have proper punctuation (e.g Implementation of hook_theme()). Ok
3. A newline after array('data' => t('System'), 'field' => 'src') should go. Ok
4. Spacing problems after return confirm_form($form, Fixed this (just copied the code from user.module so there's the problem from).
5. The doxygen header for path_theme() is formatted wrong Ups, fixed it.

Ok, took the time to read through the actual issue. A core commiter probably has to decide whether the multiple delete addition should get in (feels like an UI improvement to me) or if it's the patch in #30 that should go in.

Testing the patch in #37 I have "AllAllAll" rendered under the table (one "All" is rendered for every alias [3 in this case].) I'm *guessing* it's a pager issue.

// array_filter returns only elements with TRUE values doesn't make much sense to me (and should be written array_filter() and with punctuation).

Ok, took the time to read through the actual issue. A core commiter probably has to decide whether the multiple delete addition should get in (feels like an UI improvement to me) or if it's the patch in #30 that should go in.

For this reason i didn't changed the status of #30 (my patch is an addition).

Testing the patch in #37 I have "AllAllAll" rendered under the table (one "All" is rendered for every alias [3 in this case].) I'm *guessing* it's a pager issue.

No, it was a "copy and paste" error. I wrote $form['operations'][$key] instead of $form['language'][$key] and therefore the language column wasn't rendered correct.

// array_filter returns only elements with TRUE values doesn't make much sense to me (and should be written array_filter() and with punctuation).

Change it to // array_filter() without any callback function supplied removes all entries equal to FALSE. and corrected some other comments.

I set this issue back to 'RTBC'; but just for my patch in #30,

This is a long issue and path.module is seriously broken now. Although stBorchert additions are nice and useful it's holding-up this issue.

We are already in beta-2 with D6; let's first fix bugs and then think about better usability.

You're right.
I will create a feature request and append a patch on top of yours.

Lets commit this one to minimize the critical list. :-)

greetings,

Stefan

While waiting for your message "committed to HEAD" ( ;-) ) I've created the new feature request: http://drupal.org/node/186571 for deleting multiple aliases.

So just to clarify for committers: the patch that is RTBC for this issue is in #30.

Let's summarize what happens in this patch, so I understand:

- both fields on the path alias admin form are made required
- an error message is added to check for path validity (or accessibility) on submission
- a reset button is added, which resets the path admin filter
- makes alias deletion fall back on language independent aliases (so deleting a language based alias explicitly is not possible anymore through the API)
- simplifies the update, which I don't understand
- uses $language consistently in path_nodeapi()
- fixes the form ID in locale_form_alter()

The deletion fallback is "interesting", or at least should be documented and I'd like to understand how/why was it possible to simplify that part of the code.

Let's summarize what happens in this patch, so I understand:
- both fields on the path alias admin form are made required

Yes

- an error message is added to check for path validity (or accessibility) on submission

Yes

- a reset button is added, which resets the path admin filter

Yes

- makes alias deletion fall back on language independent aliases (so deleting a language based alias explicitly is not possible anymore through the API)

Partly true. Assuming I have a Dutch and Russian allias for the same page I can still delete only the Russian-one for example trough the API and leave the Dutch as-is.

It will only fall-back on the language independent alias if the specified language does not exists. So if I delete the French version of an page in the example above it will delete language-independent one because French did not exists.

This is a fallback for language-neutral aliasses created in the admin page for nodes-with-a-language-set.

- simplifies the update, which I don't understand

I don't undstand exactly what you don't understand. If you mean 'function path_set_alias'; it was from chx's patch that I tested and it works as expected.

- uses $language consistently in path_nodeapi()

Yes

- fixes the form ID in locale_form_alter()

Yes

The deletion fallback is "interesting", or at least should be documented and I'd like to understand how/why was it possible to simplify that part of the code.

Explained above; or still not clear?

About simplifying path_set_alias, I said above "removes some ancient garbage in path_set_alias which, to hunmonk's examination, was unnecessary, and also causing endless loops on certain path edits". Now, it seems that hunmonk, desbeers and myself tested stuff and it's still not necessary. Putting back into Goba's queue.

People, instead of repeating countless times that it is not necessary, can we take our time more efficiently and get it explained, why it was not required, and how was it possible to simplify it. Eg:

- this was how it worked
- it was bad because of this, this and this
- it is better now because of this

I know you think it is better, there is no need to repeat that to me. I don't know why, and that was my question.

Ps. I said I can accidentally remove a language independent alias with the API if I try to remove a language specific one, that was my comment. The function needs a phpdoc comment extension to this effect. So it clearly says that on deletion, the language code is advisory and not restrictive.

Gabor, why we should bother trying to understand some code we are removing? That's like saying the new menu system can not be committed until we understand the old. We know the new works, period.

chx, you are getting better and better in convincing me I should not value your opinion here. You seriously argue that it is the best thing to replace some code which you don't care what did before with another block of code? Again, our time is better spent on getting this forward instead of arguing on principles. The menu system is orders of magnitude bigger than this, and we already took more time arguing about whether this needs explanation or not then what it would require for patch authors or reviewers to explain why this is better.

I am not going to modify the status here, so you are happy and all. But neither I am going to commit before I know you have a reason to support this patch knowing the effect it has (which requires knowing what was there before). I have things really ready for commit to deal with instead for now.

Indeed I argue that. We know what path_set_alias needs to do, we know it does just that, so why would I care how it did that beforehand?

OK, I took some time to look into this. So the EXISTING update logic was as follows:

- path_count = number of entries for this source path and language combo
- alias_count = number of entries for this alias and language combo (can be either 1 or 0(?) as commented in the code)

- if alias_count >0 and path_count == 0: move THE existing alias to the new source path (same language)
- if alias_count >0 and path_count > 0:
- remove (any) entry pointed to this alias (same language)
- remove (any) entry which used this source path (same language)
- add new entry with source path, alias and language

So this code seem to ensure you always have one path, alias combo in the given language, removing anything, which is in the way.

Now with the suggested code, you always do:

- if alias_count > 0: move existing alias to the new source path (same language)

This code works differently and obviously does not enforce the limitation documented.

If this limitation should not be enforced, then probably the existing API should be reviewed (yes, the existing code). Or at least this should be walked through and documented. I think we need to understand our borders here (what's the feature set exactly required) and then implement this to the spec. Randomly changing how this function works does not seem to be a good idea to me. I think is pretty enough to set this code needs work.

That makes two. I am out of path module now for good and I will work hard on not getting entangled in it for a very, very long time.

Chx, thats fine. Here is an overview of what's a problem here for anyone who picks this issue up:

- path_set_alias() is an API function:

contrib-5.x $ grep -R path_set_alias *
modules/advcache/all_patches.patch: function path_set_alias($path = NULL, $alias = NULL, $pid = NULL) {
modules/advcache/all_patches.patch:       path_set_alias($path, $alias);
modules/advcache/path_cache.patch: function path_set_alias($path = NULL, $alias = NULL, $pid = NULL) {
modules/advcache/path_cache.patch:@@ -145,7 +143,6 @@ function path_set_alias($path = NULL, $a
modules/advcache/path_cache.patch:@@ -154,11 +151,9 @@ function path_set_alias($path = NULL, $a
modules/advcache/path_cache.patch:@@ -167,6 +162,10 @@ function path_set_alias($path = NULL, $a
modules/advcache/path_cache.patch:       path_set_alias($path, $alias);
modules/category/category_legacy/category_legacy.module:                  path_set_alias('node/'. $legacy_map[$tid], $path->dst, $path->pid);
modules/category/category_legacy/category_legacy.module:                  path_set_alias('node/'. $legacy_map[$tid] .'/feed', $path->dst, $path->pid);
modules/category/category_legacy/category_legacy.module:                  path_set_alias('forum/'. $legacy_map[$tid], $path->dst, $path->pid);
modules/category/category_legacy/category_legacy.module:                  path_set_alias('taxonomy/term/'. $legacy_map[$cid], $path->dst, $path->pid);
modules/category/category_legacy/category_legacy.module:                  path_set_alias('taxonomy/term/'. $legacy_map[$cid] .'/0/feed', $path->dst, $path->pid);
modules/epublish/epublish.module:                                       path_set_alias("epublish/$ppid/$peid",$pnid,$pthid);
modules/epublish/epublish.module:                                       path_set_alias("epublish/$ppid/$peid",$pnid);
modules/import_html/import_html.module:      path_set_alias($normal_path, $node->old_path);
modules/liquid/liquid.module:    path_set_alias("node/$nid", drupal_urlencode("wiki/".$wid->toURLString()));
modules/liquid/liquid.module:    path_set_alias(NULL, drupal_urlencode("wiki/".$wid->toURLString()));
modules/liquid/liquid.module:    path_set_alias("node/$nid", drupal_urlencode("wiki/".$new_wid->toURLString()));
modules/liquid/liquid.module:    path_set_alias(NULL, drupal_urlencode("wiki/$wid"));    
modules/mysite/plugins/types/path.inc:      path_set_alias("mysite/$myuser->uid/view", $path);
modules/openresort/business.module:          path_set_alias('taxonomy/term/'.$category->tid);
modules/openresort/business.module:          path_set_alias('forum/'.$category->tid);
modules/pathauto/pathauto.module:    path_set_alias($src, $dst, $pid, 1, 10);
modules/pathauto/pathauto_taxonomy.inc:          path_set_alias('taxonomy/term/'. $category->tid);
modules/pathauto/pathauto_taxonomy.inc:          path_set_alias('forum/'. $category->tid);
modules/pathauto/pathauto_user.inc:      path_set_alias('user/'. $user->uid);
modules/pathauto/pathauto_user.inc:        path_set_alias('blog/'. $user->uid);
modules/pathauto/pathauto_user.inc:        path_set_alias('user/'. $user->uid .'/track');
modules/url_access/url_access.module:      path_set_alias('node/' . $node->nid, 'protected/' . $uuid);

- the proposed code changes the 6.x-dev API two places:

1. possibly deletes language independent path aliases if a language dependent was not available and you intended to delete the language specific one
2. modifies language alias update setting code to allow more aliases per source path for a specific language

I am not saying either of these are inherently bad changes, but they are definitely API changes. By looking at the code unchanged, it seems that this also changes the intention documented in the function. So TODO:

- review what should exactly be allowed by the API
- review how to obey to the "specs" properly and whether the previous or the new proposed code does it better
- if the current code seems to does it better (although it does not look good for sure), get hunmonk's test case mentioned by chx, so we know more about the "certain path edits" (http://drupal.org/node/154517#comment-329915) when this could end up in an endless loop

So far it seems to me that applying the patch brings us from one bad state to another bad one.

Gábor, thanks for the overview. Now I understand what your problem is with this patch. I'm still on this issue; and will spend some time on it today to find a proper solution. Might take some time.

As a start, attached a patch that fixes the 'locale_form_alter' function in locale.module that is simply broken now. It's a one-line patch but it makes the 'admin/build/path/add' page working as expected again. I set it RTBC because it's a non-issue te review. I hope you don't mind.

RTBC :-)

Gábor, I'm making progress, I implemented all usability fixes from Chx's patch in 'path.admin.inc' now. No function changes, just usability.

What I want to know if you want to connect a node strictly to one language. You want to keep the door open for 'contrib-modules' to make 'a node multiple-languages' as far as I understood. (see #29). I don't agree. And you don't either, to quote #27:

For nodes, we expect all nodes to be in a specific language, or be language neutral.

Let's stick to that. And let 'node/edit/%' take 'ownership' after submitting if there is no language set. Don't keep a door open for other options concerning nodes. Better to 'redirect'. If I ask for 'node/%' in Dutch but I'm Russian and there's a Russian version, give me 'node/% in Russian', so change the %.

Don't be too flexible. Don't open the door for '1 node, X languages'. Give a redirect (what's already in D6 more or less). Make a choice.

Back to 'path.module':

• A node is is bonded to a language.
• Other pages, set-up in admin.

I'm more than willing to spend a lot of time on this.

• First, I have to make a 'multi-language' site for my work and I'm actually payed (partly) to solve this issue :-).
• Second (more important), a good friend of mine (who is Dutch too) is a great creative artists and want to go international. He has a Dutch site made by me (Drupal off course) but his international site is made by somebody-else (I simply had no time) in Joomla. It looks great but he likes his Drupal site much more. I promised him an international Drupal site by the end of the year.

Great, committed the locale alter fix.

As far as possibly "multilanguage nodes" go, the use case is that you have a language neutral(!) node, but you might be interested in having aliases in more languages. A dumb example is a photo node of your baby, with possibly different path aliases per language. This might be an overly edge case, and it is not a disaster probably if we don't support it.

subscribing

Still a patch, still needs work.

Attached a patch with only the usability changes from #30, so skipping the changes in function 'path_set_alias'.

What this patch does:

In 'admin/build/path':
- added a filter.

In 'admin/build/path/add':
- set both fields as required.
- check for original path (existence and access).

And it also cleans-up function 'path_nodeapi' but no functional changes.

I'm still thinking about what to do with 'path_set_alias'.

By the way; none of this code is mine, it's from chx's patch in #13

Patch applies cleanly. All my comments from #35 stand - this is much easier to use, the filter was previously only available via contrib module (and not as nice as this at all). I also enabled an additional language, added aliaes, all of that is quite useable, and again a big improvement. Going to tentatively mark this RTBC, then clean-up code could be in a followup patch.

Thanks catch! Now let's fix the problems with editing nodes. Usability improvement is nice; but currently, doing a node/edit/% is broken concerning aliases when dealing with languages.

Attached a flowchart for what to do when updating a node.

I did not write any code for it; but this is what function 'path_set_alias' should do in my opinion when updating a node.

I would like some feedback before I start to write code because I'm not sure I'm on the right track.

OK, comitted #60. Let's get rolling with the last part of the fixes.

I looked at the flowchart, and although not everything looked right for me at first look, thinking more about it, all looks fine now. When a node has no language, we should fiddle with the neutral alias only, when one changes from being neutral to language dependent, we should move the alias over to the language, and when a node is in a language, we should only fiddle with the language dependent one. Although this implements the "leaving possibilities open" strategy, depending on where you implement the neutral -> language dependent update, it might not be possible to jump from having a language neutral alias to having a neutral and a language specific alias too if so desired (again, there is no core use case for this, but contrib modules might do such things). So if this is implemented in path_nodeapi() then this is the default behaviour for nodes, but if this is enforced in path_set_alias(), we enforce a single language alias basically, or one needs to go through hoops first removing the neutral alias, then adding it back in.

Attached a new flowchart that shows that the implementation is in 'path_nodeapi()'.

I want to keep the logic concerning 'nodes' in the node_api and let 'path_set_alias' just be an API function.

Comment on the flowchart (sorry for nitpicking, but I was confused for a moment). The first end action is reached if this applies:

- the node has no language OR (the node has a language AND the node has no alias yet)

This results in the following action:

Save the alias; overwriting optional neutral language alias or if we have a language, delete the neutral alias and add the language alias.

The bold text belongs in the end action immediately below it (reached if the node has a language and a neutral alias). It is already in there, so in this block it's redundant. It should probably be rather like this:

Save the alias, overwriting the neutral language alias or adding a language alias if we have a language.

title change since the usability is in now.

OK, let's define what is left here to fix. From the comments, it seems path_set_alias() needs some fixing still, but Desbeers probably have better insight.

I think I did my best to comment on the flowcharts. How else can I help in fixing this?

I was on a course the last two weeks that took a lot more of my time than expected but I will continue to work on this issue tomorrow. I think it's all clear what has to be done; it just has to be done :-)

Retitled to make more sense with the current issue at hand.

A new patch as a proof of work in progress:

* function path_nodeapi:
Case 'insert' did not respect the language of the node when inserting. Now it does.

* function path_set_alias:
When deleting an alias from a node it will be done based on it's pid instead of path. This solves the problem with deleting 'neutral' aliasses when editing an 'language' node. Function path_nodeapi('load') loads it's value via function drupal_get_path_alias() who falls back on neutral, ignoring aliases in other languages. So a 'node/%/edit' will never screw-up other languages.

TODO:

- editing a node-alias from a form.
- cleanup path_set_alias()

Reworked path_set_alias()

Did not test it too much yet and code can be simplified a bit more I think but a second look would be appreciated to know if the 'flow' is alright now.

I recently figured out finaly why path_set_alais was calling itself back , three times. if you had "contact" on node/1 and has written a new node/2 page with an alias of "contact" then "concat" needed be dleted from node/1 and "contact" needed to be set on node/2. For one reason or another, it also deleted the alias on node/2 . So the intention might have been to only allow one alias per path -- this never worked this way, however, you could all path_set_alias to set another alias on the same node. To my understanding, this never happened though in core.

But path_nodeapi('validate') checks if the alias already exists and returns an error if so. So it looks like it was never the intention to 'relocate' an allias in a node-form.

Also the admin-page checks for existing aliases.

One path can only have several aliasses when in a differend language, not with the same language. I thought that was by design. The only way to 'relocate' an alias from a node-form is to remove it and add it again somewere else. Or that's not the idea? On the admin you can relocate it in the form.

(boiling-path-head now)

Updated the patch:

Documentation:

I asked a college to play with it and she was confused about the mix of 'path' and 'URL alias'. She also had no clue what a 'node' was. So I went true the documentation and made it consistent:

• node = post
• path = URL aliases

Functions:

* path_set_alias():

• I forgot a few urldecode()'s so I moved it to the beginning to avoid I have to repeat myself over and over again.
• I added a drupal_set_message() to the actions.

* path_admin_delete_confirm_submit():

• let it use path_set_alias() instead of path_admin_delete().

* path_admin_delete():

• deleted, see above.

More consistence in documentation, now also in 'locale.module'.

http://drupal.org/node/195408 was duplicate, I think.

Desbeers: why aren't we concentrating on fixing the actual issues with path saving. I could argue about URL aliases, as they look very bad in some cases. URLs can be much more than paths, so telling people they can alias their internal addresses "to a URL", tells them we can alias to other domains, etc.

Anyway, we look for bug fixes in this issue, so let's try to concentrate on that. We are very short on time. Let's not screw ourself arguing on UI text when e need to fix bugs here as soon as possible.

So I see your changes to path_set_alias(), path_admin_delete() and path_nodeapi() are relevant for the bugfix review here. Feel free to save your UI suggestions to another issue, where it can be debated to great lengths. I reviewed changes to these three functions:

- path_set_alias() is considered to be an API function, so drupal_set_message()-ing in it is not possible, return status values based on what was done, and do the user messages outside
- Why do we need to urldecode() the path and alias in the function? We did not do this before.

Otherwise the new code flow looks good, and interestingly much easier to follow than the previous one. Looks like a good job.

Ok Gábor, a new patch without the UI stuff (keep it on my list for D7, or not too late for a new issue?).

- removed the drupal_set_message() in path_set_alias(), not correct indeed. Forgot it was an API function.
- removed urldecode(), not necessary.

So what's left is the rewritten path_set_alias() function and a small correction to path_nodeapi() not saving te language.

Great, feel free to submit your issues even against Drupal 6. It might not get in, as the string freeze is expected to be with RC1 which is approaching quickly. But then it might get in in D7 early. I have been reading through the two path_set_alias() implementations (before and after patch), and will even copy here for reference:

// BEFORE
function path_set_alias($path = NULL, $alias = NULL, $pid = NULL, $language = '') {
  if ($path && !$alias) {
    // Delete based on path
    db_query("DELETE FROM {url_alias} WHERE src = '%s' AND language = '%s'", $path, $language);
    drupal_clear_path_cache();
  }
  else if (!$path && $alias) {
    // Delete based on alias
    db_query("DELETE FROM {url_alias} WHERE dst = '%s' AND language = '%s'", $alias, $language);
    drupal_clear_path_cache();
  }
  else if ($path && $alias) {
    $path = urldecode($path);
    $path_count = db_result(db_query("SELECT COUNT(src) FROM {url_alias} WHERE src = '%s' AND language = '%s'", $path, $language));
    $alias = urldecode($alias);
    // Alias count can only be 0 or 1.
    $alias_count = db_result(db_query("SELECT COUNT(dst) FROM {url_alias} WHERE dst = '%s' AND language = '%s'", $alias, $language));

    if ($alias_count == 0) {
      if ($pid) {
        // Existing path changed data
        db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE pid = %d", $path, $alias, $language, $pid);
      }
      else {
        // No such alias yet in this language
        db_query("INSERT INTO {url_alias} (src, dst, language) VALUES ('%s', '%s', '%s')", $path, $alias, $language);
      }
    }
    // The alias exists.
    else {
      // This path has no alias yet, so we redirect the alias here.
      if ($path_count == 0) {
        db_query("UPDATE {url_alias} SET src = '%s' WHERE dst = '%s' AND language = '%s'", $path, $alias, $language);
      }
      else {
        // This will delete the path that alias was originally pointing to.
        path_set_alias(NULL, $alias, NULL, $language);
        // This will remove the current aliases of the path.
        path_set_alias($path, NULL, NULL, $language);
        path_set_alias($path, $alias, NULL, $language);
      }
    }
    if ($alias_count == 0 || $path_count == 0) {
      drupal_clear_path_cache();
    }
  }
}

// AFTER
function path_set_alias($path = NULL, $alias = NULL, $pid = NULL, $language = '') {
  // First we check if we deal with an existing alias and delete or modify it based on pid.
  if ($pid) {
    // An existing alias.
    if (!$path || !$alias) {
      // Delete the alias based on pid.
      db_query('DELETE FROM {url_alias} WHERE pid = %d', $pid);
    }
    else {
      // Update the existing alias.
      db_query("UPDATE {url_alias} SET src = '%s', dst = '%s', language = '%s' WHERE pid = %d", $path, $alias, $language, $pid);
    }
  }
  else if ($path && $alias) {
    // 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);
    }
    else {
      // A new alias. Add it to the database.
      db_query("INSERT INTO {url_alias} (src, dst, language) VALUES ('%s', '%s', '%s')", $path, $alias, $language);
    }
  }
  else {
    // Delete the alias.
    if ($alias) {
      db_query("DELETE FROM {url_alias} WHERE dst = '%s'", $alias);
    }
    else {
      db_query("DELETE FROM {url_alias} WHERE src = '%s'", $path);
    }  
  }
  drupal_clear_path_cache();
}

Again this looks much more slick and understandable. Great.

1. What I noticed, is that urldecode() was indeed used for whatever reason in the unpatched implementation. It would be great to know why, and why would we only do that on saving a new alias, not when removing based on paths or aliases. Strange.

2. You also changed the code to always attempt doing something, unlike the existing code which tried to be safe about possibly getting NULLs where data is expected (and became very long-winded at it). That you always clear the cache shows this nicely. I am not sure we would need any safety nets here, but again another pair of eyes would be great.

Hm, hm. Digged up the source of the urldecode(): http://drupal.org/node/46746#comment-373177

Basically to be able to set URL aliases for stuff in them which should be encoded, path module should enforce storing urldecode()d path storage, and these two urldecode() calls were supposed to enforce this. However it only did this in some cases (incidentally, when called from a form when both path and alias are set), not in all cases. So we should either add urldecode() for both $path and $alias at the very start of the function or leave this to callers, cleanly standing as an API function. Requiring callers to have these decodes might not help us much with enforcing this rule so we might be better off adding the decodes to the top.

Code needs update.

Thanks for the clear explanation about urldecode() Gábor!

Updated the patch to urldecode() at the beginning of function path_set_alias(). Better than leave it up to the callers because now we don't have to enforce it.

Well, I applied this patch, and tested the following:

- saving different language aliases for non-node paths
- updating ...
- deleting ...

All worked fine, and even the proper alias was selected for display in the menu. (Although that's another part of path handling) Also tested node forms, added a language neutral node, moved the node between languages, and the alias moved as well. Also saved aliases in other languages, and that worked also.

Because my testing shows this is fine and the code also looks fine, I committed this fix, thanks.

Desbeers: Now let's move back together to http://drupal.org/node/176282 (the forums patch :) and fix that as well. Thanks for your efforts!

After the patch for http://drupal.org/node/196410 the following error is shown when you try to assign a menu link title to a multilingual node:

user warning: Duplicate entry 'about-en' for key 2 query: UPDATE drupal6rc1_url_alias SET src = 'node/1', dst = 'about', language = 'en' WHERE pid = 1 in C:\xampplite\htdocs\drupal6rc1\modules\path\path.module on line 98.

Steps:

1) Activate modules Locale, Path, Content translation
2) Add language German
3) Set language negotiation to "Path prefix with language fallback"
4) Add prefixes to both languages (en, de)
5) Enable content translation for content type "page"
6) Create original page node (in English)
7) Add translation (in German)
8) Edit English node: Menu settings / Enter menu link title ("about_en")
9) Save -> Error is shown

Whenever you try to edit this node and save the error pops up again (even without any changes to the node).
No error is shown when you try to edit the German node.

Looks like you have an existing 'about' alias for some English page? (It would be better to open a new bug report instead of reopening a completely different issue BTW. Please post an issue URL with the newly opened issue (we'd need to solve this SQL error anyway, if there was indeed an 'about' alias for an English page already.

First of all - you were the one asking me to reopen this issue because you thought it is connected:

see your last comment on http://drupal.org/node/196410 :

The duplicate entry issue is a problem however. But that's connected to http://drupal.org/node/154517 so reopen that one please, and carry over your report about the duplicate entry for the alias. Thanks!

And secondly, no I don't have another English node with the path "about"...
The steps that I've described were all done on a fresh install of drupal (rc1) - no data.

So which issue do you want me to reopen / create?

Here are the steps to reproduce (I forgot to mention to add path "about" to both versions of the node):

1) Activate modules Locale, Path, Content translation
2) Add language German
3) Set language negotiation to "Path prefix with language fallback"
4) Add prefixes to both languages (en, de)
5) Enable content translation for content type "page"
6) Create original page node (in English)
7) Add translation (in German)
8) Set path to "about" for both languages of the page.
9) Edit English node: Menu settings / Enter menu link title ("about_en")
10) Save -> Error is shown

No error is shown when you don't use a path alias (Step 8).

Following the steps in #87 I can confirm the error. The problem is that after saving the 'english' node the alias of the 'german' node is updated (look at the PID number); giving a duplicate error.

I don't know if this is related to the patch from http://drupal.org/node/196410 but because the error is created in function path_set_alias() I set this issue open again, assign it to myself and will do further investigation later today.

Function path_form_alter() was not language-aware so in this example it was looking-up the first 'about' it could find returning the wrong PID for the node.

One line patch attached.

daniel.soneira: Uhm, sorry for the misunderstanding.

Desbeers: patch looks good and logical. After some testing by Daniel or someone else it looks like it is a nice fix.

No problem Gábor.

I applied Desbeers patch - the error is gone :)

I still don't think the primary links that are shown as a result are correct (I really would expect both links to use a path alias), since it shows ONE of them correctly ( see #15 in http://drupal.org/node/196410 ).

From a user's point of view it is not easy to understand why it works in the content administration but not in the primary menu.
Will the core menu module ever support i18n? - but that's another story.

Thanks for the patch.

Thanks for testing, I committed the patch from #89.

As for the menu, in Drupal 6, the menu does not know whether you link to a node in a different language, so that it needs to look for an alias from a different language. Menus are not i18n aware in Drupal 6. You'll need i18n or localizer module to handle this.

Hi everybody,

I don't really know if this topic is related to the problem I had, but I post it as I spend some hours and I hope someone will have some time saved by this.

I explain my problem:

- I built a site in 3 languages: italian, english, french
- I set english as the default language, so that www.mysite.com gives the english content
- all the nodes I created are translated into the three languages, and work good, but there are (at least) 2 links which don't:

www.mysite.com/it
www.mysite.com/fr

They should point to the home page in italian and french language, but they actually give english content (default) and language menus.
The problem I encountered is that the browser, after some navigation in italian, went to www.mysite.com/it insted of home www.mysite.com, so that a "mixed language" page was returned.

Here's the solution I found:

Let's say the default home page is "node/1".
Let's say the translations of it are "node/2" for italian and "node/3" for french.

I set the following URL Aliases:

Alias, System

it/node/1, node/2
fr/node/1, node/3

and everything works fine.

That's why drupal, when a URL with only the language is submitted, builds the path:

/node/

which gives the mixed page.

I hope it was clear and useful to someone

Daniele

Erm, this was a fixed 6.x issue, please leave it at that.