template base_path() code - breaking il8n menu links [#739416]

HI @all,

I must mentioned that the following code block in the pathauto_create_alias() function works incorrectly when you try to add aliases for the same source path, but in different languages during the bulkupdate operation:

function pathauto_create_alias($module, $op, $placeholders, $src, $entity_id, $type = NULL, $language = '') {

  // ... more code

  // Special handling when updating an item which is already aliased.  
  $pid = NULL;
  $old_alias = NULL; 
  if ($op == 'update' or $op == 'bulkupdate') {
    if (variable_get('pathauto_update_action', 2) == 0) {
      // Do nothing
      return '';
    }
    $update_data = _pathauto_existing_alias_data($src);
    $pid = $update_data['pid'];
    $old_alias = $update_data['old_alias'];
  }

  // ... more code

}

So the $update_data is loaded without respecting the passed $language variable... So the new alias is always considered an existent alias, if another language already uses the same alias. Is this what it should work like?

Example

The alias value is the same in english and german.
The first call sets the alias in english:

pathauto_create_alias('module', 'bulkupdate', array(), 'the/path', 'entity_id', 'type', 'en');

The second call tries to set the alias in german:

pathauto_create_alias('module', 'bulkupdate', array(), 'the/path', 'entity_id', 'type', 'de');

Unfortunately the second call wonÄt create a german alias, because the $update_data used contains the english alias and so the system treats the german alias as already existent.

I'd appreciate to see this fixed as soon as possible, as I really need the possibility to update paths with same source but in different languages.

Thanx in advance & cheers

hctom

Comment	File	Size	Author
#25	739416-pathauto-existing-alias-data-language-D7.patch	7.36 KB	dave reid
#24	739416-pathauto-existing-alias-data-language-D7.patch	7.4 KB	dave reid
#23	739416-pathauto-existing-alias-data-language-D7.patch	3.68 KB	dave reid
#21	739416-pathauto-existing-alias-data-language-D7.patch	3.68 KB	dave reid
#9	pathauto-739416.patch	8.23 KB	hctom
#8	pathauto-739416.patch	8.23 KB	hctom

Comments

Comment #1

klonos

he/him

English

90% Melbourne, Australia - 10% Larissa, Greece

commented 11 March 2010 at 12:35

Please take a look at these three issues:

#357185: template base_path() code - breaking il8n menu links
#358315: drupal_lookup_path() not respects alias' order
#269877: path_set_alias() doesn't account for same alias in different languages

Are they similar to your issue or perhaps one of them is the same (duplicate)? Especially pay attention to this comment of mine:

http://drupal.org/node/357185#comment-2700610

Let me know.

Comment #2

hctom

German

commented 11 March 2010 at 13:17

@klonos:

As far as I see those issues deal with different things... In my case the alias records are not even tried to be created in the database because the $updata_data holds wrong information that does not repsect the passed language for the new alias. So there are no DB errors or DB records in the database for the second alias of the example.

cheers

hctom

Comment #3

klonos

he/him

English

90% Melbourne, Australia - 10% Larissa, Greece

commented 11 March 2010 at 15:38

ok then. It was just a thought...

thank you for taking the time to reply.

Comment #4

hctom

German

commented 11 March 2010 at 16:26

no problem... and as far as I examined the problem, extending the _pathauto_existing_alias_data() function signature with an optional language argument and using this while querying the database for existent aliases should solve the problem.

cheers

hctom

Comment #5

hctom

German

commented 11 March 2010 at 16:30

Here's a draft how the function should look like:

function _pathauto_existing_alias_data($src, $language = NULL) {
  $output = array(
    'pid' => '',
    'old_alias' => ''
  );
  $args = array($src);
  $sql = "SELECT pid, dst FROM {url_alias} WHERE src='%s'";
  if (!is_null($language)) {
    $args[] = $language;
    $sql .= " AND langauge = '%s'";
  }
  $result = db_query($sql, $args);
  if ($data = db_fetch_object($result)) {
    // The item is already aliased, check what to do...
    switch (variable_get('pathauto_update_action', 2)) {
      // Replace old alias - remember the pid to update
      case 2:
      case 3:
        $output['pid'] = $data->pid;
      // Add new alias in addition to old one
      case 1:
        $output['old_alias'] = $data->dst;
        break;
      // Do nothing
      case 0:
      default:
        break;
    }
  }
  return $output;
}

And pathauto_create_alias() should then call like this:

function pathauto_create_alias($module, $op, $placeholders, $src, $entity_id, $type = NULL, $language = '') {

  // ... more code

  // Special handling when updating an item which is already aliased. 
  $pid = NULL;
  $old_alias = NULL;
  if ($op == 'update' or $op == 'bulkupdate') {
    if (variable_get('pathauto_update_action', 2) == 0) {
      // Do nothing
      return '';
    }
    $update_data = _pathauto_existing_alias_data($src, $language);
    $pid = $update_data['pid'];
    $old_alias = $update_data['old_alias'];
  }

  // ... more code

}

Comment #6

hctom

German

commented 19 March 2010 at 16:28

Component:

Code

» I18n stuff

Are there any news from the maintainers on this? I'd appreaciate to hear from you, as this is quite urgent, because this bug really destroys my pathauto behavior... and the site that uses the module has to be launched on April 1st

I'd appreciate any feedback

cheers

hctom

Comment #7

greggles

he/him

English

Denver, Colorado, USA

commented 24 March 2010 at 18:39

Thanks for your bug report and for helping make this a better module. Could you provide this as a patch?

Also, we are trying to get simpletests for everything we add. So, if you could write a simpletest that shows this bug and then runs properly when your patch is applied that would help us understand the problem and be more confident in your solution.

Comment #8

hctom

German

commented 24 March 2010 at 22:53

Status	File	Size
new	pathauto-739416.patch	8.23 KB

Hi @all,

I finally got my first patch done :) This one includes the function signature changes of _pathauto_existing_alias_data() and all changed calls to it. I also added a test (in the unit test case) for the new functionality (including a test module that provides a language dependant token to test against - I guess it is not in the right place, but I am not that familiar with CVS).

To reprodcue the problems occuring without my changes, just apply the patch and revert the pathauto.inc to its base state. When you then unit test the module, you will see, that only the last alias in the line is created for the same path.

I hope this helps and will make it into the module as soon as possible :)

Cheers

hctom

Comment #9

hctom

German

commented 24 March 2010 at 23:05

Status	File	Size
new	pathauto-739416.patch	8.23 KB

Damn... I uploaded the wrong file... so here comes the real patch :)

... And I also forgot to mention: This interferes a little with the known path sorting problem of the path module... but there is a comment in _pathauto_multilingual_test_languages() in the pathauto_multilingual_test.module file, describing the issue.

cheers

hctom

Comment #10

greggles

he/him

English

Denver, Colorado, USA

commented 24 March 2010 at 23:09

Status:

Active

» Needs review

Great work! I haven't reviewed the patch/test but really appreciate that you have taken the time to provide both!

Last step: when you upload a patch that you think is in a good state be sure to mark the status as "needs review."

Comment #11

hctom

German

commented 24 March 2010 at 23:31

You're welcome! I'll keep the status change in mind next time :)

Comment #12

hctom

German

commented 6 April 2010 at 08:29

hmmm.... I'm a little bit confuised why nothing is happening here... am I missing something, or shouldn't this be reviewed at last by somebody?

I'm quite disappointed that my work does not get recognized or even processed as this is really an issue that in my opinion should be fixed as soon as possible...

hctom

Comment #13

sirian commented 21 April 2010 at 18:26

I agree. It's issue is bugging all my internationalized sites. I don't understand why this patch wasn't included in the latest release of pathauto. Is there a Why?

Comment #14

ufku commented 5 May 2010 at 21:58

the patch works!

Comment #15

risca commented 11 June 2010 at 10:10

Great! I was really looking for it. I try also others but this is the only one that has worked properly.

Comment #16

zualas commented 11 June 2010 at 13:07

@risca

I checked your website http://risca.no-ip.info, tried to look at the About page in different languages. The path still doesn't seem to be right for you. Try this:
1) go to your website risca.no-ip.info, click on the About page. The path will be http://risca.no-ip.info/en/content/about
2) Switch the language to Italian. The path will be http://risca.no-ip.info/it/content/about
3) hover your mouse over the About link. The path will be http://risca.no-ip.info/it/node/50 (when you click on it, it gets the correct alias though)

I will open a new issue specifically for the hyperlinks on the page.

Comment #17

risca commented 14 June 2010 at 10:48

@ zualas
Here more infos:
I am currently using the patch as at post n. 9.
As you can now check the page "about" is now working on both translation (ITA-ENG). The problem, that still affects me, is that the patch works only with new pages, it doesn't fix the previous one. In fact what I did was first to apply the patch, then to translate the page "about" to ENG but, exactly as you've already noticed/debug, it hasn't work. Therefore I try to delete it completely (both the ITA and the ENG version) and to create it again and so it starts working.
Keeping trying I can confirm that the patch works only with new pages and that it doesn't still allow to translate the old one (the one that I have written before to apply the patch).

I think it is a problem in updating the database but I have no clue on how to solve it, I'm not a programmer... Anyway if you need to know about logs and files, just ask me.

---EDIT---
It only works if the new page that I create is already set in a language. Meanwhile if it is "language neutral" and I decide to translate it later on (making it ITA and adding a ENG translation) it wont work and will cause the problem properly described by you:
1) go to your website risca.no-ip.info, click on the About page. The path will be http://risca.no-ip.info/en/content/about
2) Switch the language to Italian. The path will be http://risca.no-ip.info/it/content/about
3) hover your mouse over the About link. The path will be http://risca.no-ip.info/it/node/50 (when you click on it, it gets the correct alias though)

Comment #18

ufku commented 14 June 2010 at 10:54

Status:

Needs review

» Reviewed & tested by the community

@risca
I think you should also be using the patch at #269877: path_set_alias() doesn't account for same alias in different languages

These two issues needs to be fixed in order to get same aliases work for multiple languages.

Comment #19

risca commented 14 June 2010 at 12:26

Sorry, but I notice just now that it is already working. The funny thing was that I was surfing on the website using my profile which had the preferred language set to Italian, therefore I was redirect to any Italian translation, if any available. I try instead to set personal user's language to English and in fact I've been redirect straight to every ENG page.
Sorry sorry sorry.

Comment #20

5 July 2010 at 21:50

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, pathauto-739416.patch, failed testing.

Comment #21

dave reid

he/him

English

Nebraska USA

commented 22 July 2010 at 01:13

Version:	6.x-1.x-dev	» 7.x-1.x-dev
Status:	Needs work	» Needs review

Status	File	Size
new	739416-pathauto-existing-alias-data-language-D7.patch	3.68 KB

Re-rolled and improved. Doesn't include the test from the latest patch because there's got to be an easier way to test this.

Comment #22

22 July 2010 at 01:30

Status:

Needs review

» Needs work

The last submitted patch, 739416-pathauto-existing-alias-data-language-D7.patch, failed testing.

Comment #23

dave reid

he/him

English

Nebraska USA

commented 22 July 2010 at 02:13

Status:

Needs work

» Needs review

Status	File	Size
new	739416-pathauto-existing-alias-data-language-D7.patch	3.68 KB

Odd. Apparently I can't use :language-none as a placeholder. Oh well. Modified the patch.

Comment #24

dave reid

he/him

English

Nebraska USA

commented 22 July 2010 at 06:59

Status	File	Size
new	739416-pathauto-existing-alias-data-language-D7.patch	7.4 KB

Worked on an easy test case that tests a node with two different language aliases gets the proper language alias deleted when the node is updated and alias changed.

Comment #25

dave reid

he/him

English

Nebraska USA

commented 22 July 2010 at 07:00

Status	File	Size
new	739416-pathauto-existing-alias-data-language-D7.patch	7.36 KB

Without a debugging line left in....

Comment #26

dave reid

he/him

English

Nebraska USA

commented 22 July 2010 at 14:03

Confirmed that rolling back the patch but leaving the test file confirms a failure, so this should be fixing it properly. This would be great if someone else in this issue could test this patch on D7.

Comment #27

dave reid

he/him

English

Nebraska USA

commented 24 July 2010 at 20:33

Status:

Needs review

» Fixed

Finally got this committed to both D7 and D6-2 branches with working tests.
http://drupal.org/cvs?commit=397220
http://drupal.org/cvs?commit=397222

Thank you everyone for your help on this issue.

Comment #28

7 August 2010 at 20:40

Status:

Fixed

» Closed (fixed)

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

Comment #29

clashar commented 31 July 2011 at 10:38

I can't see Pathauto version of July 25, 2010 or later
Please confirm that patch is committed, the last 7-x version is dev-version of 2011-Jun-22

Comment #30

clashar commented 1 August 2011 at 12:43

Status:

Closed (fixed)

» Active

and I can't apply
739416-pathauto-existing-alias-data-language-D7_0.patch
739416-pathauto-existing-alias-data-language-D7_1.patch
739416-pathauto-existing-alias-data-language-D7_2.patch

to latest Pathauto dev 2011-Jun-22
as patches refer to 5 July version

Comment #31

dave reid

he/him

English

Nebraska USA

commented 1 August 2011 at 14:17

Status:

Active

» Closed (fixed)

This is already included in the latest releases.

Comment #32

clashar commented 1 August 2011 at 14:30

please explain in which 2011-Jun-22?

Comment #33

dave reid

he/him

English

Nebraska USA

commented 1 August 2011 at 14:39

You are greatly over thinking things. All you have to do is download one of the latest releases listed on the project homepage (http://drupal.org/project/pathauto). For example, the 7.x-1.0-rc2 release has a date of June 16, 2011, which is later than the date the fix was committed in June 2010.

Comment #34

BrianLewisDesign commented 8 March 2012 at 08:00

Title:

_pathauto_existing_alias_data() should respect passed language

» template base_path() code - breaking il8n menu links

il8n prefix was removed from menu links in my template.php file:

$href = $link['href'] == "<front>" ? base_path() : base_path() . drupal_get_path_alias($link['href']);

Fixed it by sticking $languge->prefix in the path:

global $language;
$prefixx = '';
if ( $language->prefix ) { $prefixx = $language->prefix."/"; }
$href = $link['href'] == "<front>" ? base_path() : base_path() . $prefixx . drupal_get_path_alias($link['href']);

I guess the best way would be to use l() or url()? This worked easily for me in my template.

Comment #35

vflirt commented 15 August 2013 at 12:35

Status:

Closed (fixed)

» Needs work

Hi,

it is all great but the function _pathauto_existing_alias_data orders the language DESC which should be wrong.
as you have language 'und' and 'en' for example then if I try yo take the current alias for language 'en' it will return me the one for 'und' and that is wrong.

The correct order is : $order = $language < LANGUAGE_NONE ? 'ASC' : 'DESC'; as it is in path.inc of the core

If i am wrong please show me how to get the current alias for language 'en' when there is alias for language 'und'.

Kind Regards,
Dobromir

Comment #36

ricovandevin commented 12 February 2014 at 13:02

I agree with vflirt that the order can be wrong. In some cases however it can be correct. There should be distinction between $language < LANGUAGE_NONE and $language > LANGUAGE_NONE. I have created a patch for that in #2194431: _pathauto_existing_alias_data() should return language-specific alias if there is one.

Comment #37

1 February 2015 at 21:35

Dave Reid committed c0a2c03 on 8.x-1.x

#739416 by Dave Reid, hctom | risca, klonos: Fixed...

Comment #38

dave reid

he/him

English

Nebraska USA

commented 4 October 2015 at 19:29

Status:

Needs work

» Closed (fixed)

Going to mark this back as fixed.