Test for translated path aliases

While Drupal 6 stores the path alias for nodes in the language you saved the node, when it emits a link to node/x, it does not know that it need to look up a path alias for that path in that language (and it should use the path or domain prefix for that language). This is a missing feature, and you can plug this in with contributed modules like i18n or localizer when they become available for Drupal 6.

I don’t think this is a missing feature in Drupal 6, but a very serous bug.

Take the following scenario:

A site has English and Dutch content.

• Site is build with Drupal 5, without any internationalization.
• Site is upgraded to Drupal 6 and ‘story’ content-type is made language aware now and optional translatable. The Dutch language is added with ‘path prefix = nl’ as secondary language.
• All the Dutch nodes are converted to there respective language, one by one, with the intention to add translation in the future.

What happened:

• All incoming links are broken for the ‘Dutch nodes’.... When re-saving a node the original alias became ‘language aware’ while making them Dutch.
• All outgoing links for the ‘Dutch nodes’ have no more alias when visiting the site with the default language.

What I was expecting:

Drupal should fallback to the most suitable alias, but it doesn’t. It looks for an alias in the active language and as only fallback it looks for a ‘non-language’ alias too....

Solution:

It looked to me that a contributed module can’t fill this gab because it’s a fundamental problem in function drupal_lookup_path() in path.inc.

Because path aliases are currently broken in HEAD and that issue is a bit out of my league so to say [1] I created a patch for Drupal 6 now.

What this patch does:

Function drupal_lookup_path() still likes the alias set for a specific language the most, but if it can’t find it, and we are looking for a ‘node/$nid’, it will take the first option it could find, whatever it has a language set yes or no.

• Lets say I have a Dutch node with a Dutch alias. If I’m visiting in the ‘English language mode’ it can’t find an English alias but it will find the Dutch one and use it.
• Incoming aliases are not broken anymore; in depended of the used prefix an alias will be found. So example.com/nl/nederlands will give me the same result as example.com/nederlands
• When you have a Dutch node with an alias but like to create a German alias as well it will take the German version over the Dutch one.

Please note about this patch:

It will ignore aliases in ‘non-preferred’ language if the $path doesn’t contain ‘node/’. So you are still able to create an alias for ‘/contact’ in Dutch (‘kontakt’ as we call it) and in English it’s still ‘/contact’. It only became a bit smarter concerning ‘node’ aliases.

I set this issue to ‘critical’ and ‘code needs review’. I consider it critical because it’s breaking existing sites that upgrade to Drupal 6 and are using the (great) multi-languages capabilities and although the patch is not for Drupal 7, the solution is basically the same. The current alias-issues in HEAD are not related to this issue.

Nick

[1] path alias is broken in HEAD:

http://drupal.org/node/196862
http://drupal.org/node/222109
http://drupal.org/node/259412

oops, wrong patch...

Another patch; found a typo and tabs....

I agree with Desbeers. This is rather unfortunate behaviour. Desbeers, if it's the same approach for 7.x, could you make a patch for that? There's no need to go about reviewing a 6.x patch when it won't be checked in until 7.x has been fixed...

Oh, and the patch from #4 introduces trailing white-space (new lines 74 and 89).

It works for me.

and why Drupal 7?

dvinegla: Because the (unwritten? written? (where?)) commit policy says that stuff needs to be committed to n.x before being applied to n-1.x before being applied to n-2.x before being applied to... (Where n is the version of HEAD.)

As a heads up, patch #4 creates a problem I haven't been able to unravel as of yet with any pages that have the same alias in different languages. ^1,2

Setup:

• 4 language site
• Path prefix with language fallback selected
• i18n, pathauto installed¹
• First example:
  • front page set to alias 'home', which is node/1
  • translations of node/1 all aliased 'home' as well
• Second example:
  • node/666 alias set to 'coca-cola'
  • translations of node/666 aliased to 'coca-cola' as well

Issue after applying patch:

• testsite.com defaults to the node of the last language returned from the query on (post patch) line 89
• every other [lang code]/home alias returns same node as above
• ditto for pages aliased 'coca-cola'
  • menu block set to show on all pages disappears on pages with same alias²
• on all other pages, all paths to 'home' or 'coca-cola' in menu blocks are unaliased unless one is navigating in the last language returned

¹Possible conflicts caveat #1.
²Possible conflicts caveat #2. This particular menu is a primary menu fed through a custom module similar to nice menus.

The issue seems to stem from the second part of path #4 when multiple rows from the query on line#89 causes $src to be set to the last row result at the end of the while loop.

Patching just the first part, the front pages are properly aliased, menu block shows up, and all subsequent links are aliased as outlined in the initial & #2 posts - I haven't found any other weirdness as of yet.

I'm still picking apart path.inc, patch #4, & where/if the above caveats cause a conflict; if I manage a fix I'll post it. In the meantime, thoughts or ideas from those that know the path & menu system better than I would be greatly welcomed.

I just ran into this bug, very simple steps to reproduce:

1. Enable an additional language e.g. French) on your site + content translation + path.module
2. Set the 'page' content type to be translatable in admin/build/types
3. Create a page in English, give it a path alias 'english-one'
4. Translate the page into French, give it the path alias 'french-one'.
5. View the English node at /english-one - it'll work fine.
6. View the French node at /french-one - get a 404.

In admin/build/path, node/1 and node/2 are both aliased to their respective paths.

Here's a re-roll of Desbeers patch from #4 - applies cleanly to both 6 and 7 and fixes the steps outlined above with no path negotiation, and with and without language fallback.

Same patch, with an added test. You'll get one fail without the changes to path.inc

@catch: that behavior is by design.

Aliases are defined in a per-language basis and node aliases are generated according to the language the node is in.

It should work if you change:

to:

@catch: Seems like you found the blocker for #318605: Patch for tracking translated nodes as the original and case #319993: url() with language does not return url_alias in specified language maybe a duplicate of this case. I will try to test the patch in #10 later if it helps. Have you already tested if url() with language behaves correctly with your above patch?

@Damien: Do you think the behaviour documented at http://drupal.org/node/318605#comment-1054744 is not a bug? This is a really serious bug... and it does not work as you wrote... (alias is build based on node language setting)

@hass: your bug report is really awful. I listed in http://drupal.org/node/319993#comment-1057379 the behavior you should see. Are you seeing anything different?

Damien - no, that doesn't work either. Did you try it? Additionally, when viewing 'English', no alias is displayed for the link to the French translation, just the regular node/nid, whereas it is with the patch. Patch also fixes aliases with path negotiation, which I think is what you were referring too.

Updated patch with additional url() check and converted the queries to dbtng. In the issue Hass linked to, running url('node/' . $node->nid, array('language' => $node->language)) in HEAD returns node/nid instead of the alias - that's also fixed by the current patch.

Following damien's update in #319993: url() with language does not return url_alias in specified language here's a revised patch (again). The test for url() fails without the changes to path.inc, so I think that's verified here.

Indeed this is a problem that needs fixing.

But we shouldn't need node-specific tests in drupal_lookup_path(). The problem seems to be that we're not feeding in the node's language. It looks like we need to need to pass a 'language' key (the node's language, if set) in the $options array when calling l() and url() for nodes.

Note that when passing a 'language' in, it needs to be a language object--not a string that's the language name.

@nedjo: - the test explicitly deals with passing in the language key to the $options array when calling url() - which fails prior to the patch and passes with it it.

url() calls drupal_get_path_alias(), which itself calls drupal_lookup_path() - in all cases, the language is passed around between those functions, so I don't think that'll help here. The direct 404 on example.com/alias or example.com/fr/alias shows that it's an issue with drupal_lookup_path() rather than url() itself. So, tentatively marking this back to needs review, but if you could clarify your concerns (and mark back to needs work if you think I've not addressed them), that'd help.

Here is a test that works flawlessly (38 passes, 0 fails, 0 exceptions).

@catch: differences from yours:
- path aliases only make sense when language negotiation is not LANGUAGE_NEGOTIATION_NONE. If not, aliases in languages other than the default one will never be used;
- we need to call drupal_init_language() forcibly, because language.inc is not included in the testing side if we don't;
- we to call drupal_lookup_path('wipe') before using url() on the testing side, but there is a bug in that function (#315656: drupal_lookup_path('wipe') not working properly). You will need to apply the patch in that issue to get 0 fails.

Damz is right. The only issue here is if you have LANGUAGE_NEGOTIATION_NONE - in that case you can enter aliases for nodes, but they will never work. Otherwise everything works as you'd expect it to. We absolutely should not let people enter information on the node form that's going to be disregarded - either the alias should work, or the field needs to be form_altered out in those cases. New issue for that #320750: LANGUAGE_NEGOTIATION_NONE disregards path aliases for translated nodes

The test in #20 is still valid so leaving at needs review, retitling and moving to that component.

Ran #20 on an otherwise clean HEAD, and it's all passes. Since this functionality wasn't tested before, marking RTBC. When we have a decision on behaviour with no path negotiation, we should write a test for that too.

Committed to CVS HEAD. Thanks.

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

Is this eligible to be backported to Drupal 6? It seems like from the comments that it would be now that it's in HEAD. Some initial testing looks like it wouldn't be too difficult.