Problem:
- create content named "test" in a (any) language
- create content named "test" in language neutral

Result:
the alias "test" will be assigned to both content items.

Cause:
In path_pathauto_is_alias_reserved the current code makes sure that a localised alias does not interfere with any language neutral alias, which is fine.
however, it should check if _any_ alias exists when creating language neutral content.

Patch will follow

(there is some similarity between this and D6 issue #593048: _pathauto_alias_exists handles language-neutral aliases wrong like comment #25)

Comments

rv0’s picture

Status: Active » Needs review
FileSize
1.32 KB
PASSED: [[SimpleTest]]: [MySQL] 322 pass(es). View

Patch breaks up the query adding the language conditionally
Also removes order by (why would we want to order something we're casting to boolean later on?)

René-Marc Simard’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I confirm that this patch works as described. Thanks to this change I no longer end-up with colliding aliases when a language-neutral alias is being created while an identical language-specific one already exists. path_pathauto_is_alias_reserved() now properly flags the proposed alias as being reserved which leads to pathauto_alias_uniquify() testing iterations until a unique suffix is found to prevent collisions, as expected.

Thanks for the fix!

rv0’s picture

Thanks for the review @René-Marc Simard

Looking at this almost one year later, I don't know why I did not use db_select in the patch, would be a bit cleaner.

DamienMcKenna’s picture

DamienMcKenna’s picture

FileSize
1.32 KB
PASSED: [[SimpleTest]]: [MySQL] 337 pass(es). View

The only change this makes vs the original is that it comment has been reformatted to 80 chars, per the Drupal coding standards.

jotwede queued 5: pathauto-n2067191-5.patch for re-testing.

MiroslavBanov’s picture

I am not sure why more people are not reporting this problem. Patch #5 fixes it for me.

Dave Reid’s picture

+++ b/pathauto.module
@@ -454,12 +454,17 @@ function pathauto_is_alias_reserved($alias, $source, $langcode = LANGUAGE_NONE)
+  $sql = "SELECT pid FROM {url_alias} WHERE source <> :source AND alias = :alias";
+  $tokens = array(':source' => $source, ':alias' => $alias);
+  if ($langcode != LANGUAGE_NONE) {
+    $sql .= " AND language IN (:language, :language_none)";
+    $tokens += array(':language' => $langcode, ':language_none' => LANGUAGE_NONE);
+  }  ¶
+
+  return (bool) db_query_range($sql, 0, 1, $tokens)->fetchField();

This patch shoud definitely be converting this to db_select() now. I don't want to be have SQL strings constructed together with strings like this.

Leeteq’s picture

Status: Reviewed & tested by the community » Needs work
Martijn Houtman’s picture

FileSize
1.25 KB
FAILED: [[SimpleTest]]: [MySQL] 331 pass(es), 6 fail(s), and 1 exception(s). View

Patch rewritten to follow db_select() standards.

rv0’s picture

Status: Needs work » Needs review

Looks good to me

Status: Needs review » Needs work

The last submitted patch, 10: pathauto-n2067191-10.patch, failed testing.

rv0 queued 5: pathauto-n2067191-5.patch for re-testing.

Martijn Houtman’s picture

FileSize
1.25 KB
FAILED: [[SimpleTest]]: [MySQL] 331 pass(es), 6 fail(s), and 0 exception(s). View

Fixes use of $langcode, rather than $language.

rv0’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: pathauto-n2067191-14.patch, failed testing.

stefan.r’s picture

FileSize
428 bytes
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 337 pass(es). View
stefan.r’s picture

Status: Needs work » Needs review
kiricou’s picture

Hi,

I have the same pb with the recommended release 7-1.2

I try the 7-1.x-dev and same bug, and when I apply successfully your patch, it's work

Thank a lot,

I hope the dev version is stable because I put it in production.

Sim

stefan.r’s picture

Thanks for the review @kiricou. Can anyone else review so we can RTBC this?

Dave Reid’s picture

Issue tags: +Needs tests

This needs a test case to confirm it works and will not regress in the future.

bneil’s picture

Status: Needs review » Needs work

Setting to needs work per #21

bneil’s picture

Assigned: Unassigned » bneil
bneil’s picture

Assigned: bneil » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
804 bytes
FAILED: [[SimpleTest]]: [MySQL] 337 pass(es), 1 fail(s), and 0 exception(s). View
2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 338 pass(es). View
758 bytes

Here's a revised patch that adds a test to the end of the testLanguageAliases() method. It ensures that when a language neutral node is saved with what will end up being the same alias as a node with its language defined, the alias suffix is incremented correctly.

bneil’s picture

Status: Needs review » Needs work
bneil’s picture

Status: Needs work » Needs review

The last submitted patch, 24: pathauto-language_neutral_dupe-2067191-24-tests.patch, failed testing.

stefan.r’s picture

Status: Needs review » Needs work

Indentation is off lines 728 & 729 :)

Otherwise patch looks good.

bneil’s picture

Status: Needs work » Needs review
FileSize
801 bytes
FAILED: [[SimpleTest]]: [MySQL] 337 pass(es), 1 fail(s), and 0 exception(s). View
2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 338 pass(es). View
469 bytes

Ah, you're right! Here are revised patches with the indentation fix.

bneil’s picture

FileSize
2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 341 pass(es). View
801 bytes
FAILED: [[SimpleTest]]: [MySQL] 337 pass(es), 1 fail(s), and 0 exception(s). View

I noticed my comment was wrong for the test. the -1 is a suffix, not a prefix.

The last submitted patch, 29: pathauto-language_neutral_dupe-2067191-26-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: pathauto-language_neutral_dupe-2067191-27-tests.patch, failed testing.

bneil’s picture

Status: Needs work » Needs review
stefan.r’s picture

Thanks, patch looks good now. If anyone else can review this can be RTBC'ed

fullerja’s picture

Status: Needs review » Reviewed & tested by the community

Complete patch in #30 works for me. RTBC'ing based on this and #34.

baso’s picture

Patch #30 (complete version) also works for me. Thanks.

osopolar’s picture

Works for me too, thanks.

  • Dave Reid committed 1c89f85 on 7.x-1.x authored by bneil
    Issue #2067191 by bneil, Martijn Houtman, stefan.r, rv0, DamienMcKenna,...

  • Dave Reid committed 8a70a8f on authored by bneil
    Issue #2067191 by bneil, Martijn Houtman, stefan.r, rv0, DamienMcKenna,...
Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

Committed #30 to 7.x-1.x. Thanks!

Status: Fixed » Closed (fixed)

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