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

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

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

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

StatusFileSize
new428 bytes
new1.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
StatusFileSize
new804 bytes
FAILED: [[SimpleTest]]: [MySQL] 337 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 338 pass(es).
[ View ]
new758 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
StatusFileSize
new801 bytes
FAILED: [[SimpleTest]]: [MySQL] 337 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 338 pass(es).
[ View ]
new469 bytes

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

bneil’s picture

StatusFileSize
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 338 pass(es).
[ View ]
new801 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.