path_set_alias deletes or ignores aliases

@earnie

I disagree. In the current implementation only 'dst' has an unique constraint in the database which is correct!

Thinking about a migration of an existing site where you want to keep existing urls but you don't have an 1:1 relationship between the new and the old site regarding the structure you're forced to map two or more urls to one node. (Yes, you can use apache for this too.)

Example:
Old site:
/marketing/about_us.html
/sales/about_us.html

New site:
/marketing_and_sales/about_us.html

In this case you need three urls (aliases) for one node.

Currently this is a feature of drupal 5 which works well as long as you don't modify such an alias after it's creation.

So your example isn't the expected result.

The correct result of the first part of your example (which currently works regardless if you use the current or my patched version) is:

pathA, aliasA

then

path_alias_set(pathA, aliasB);
db_query("SELECT src, dst FROM {url_alias}");

should give

pathA, aliasA
pathA, aliasB

And now we face the bug:

and then

path_alias_set(pathB, aliasB);
db_query("SELECT src, dst FROM {url_alias}");

should give

pathA, aliasA
pathB, aliasB

BTW I made a mistake regarding the comments when posting my reproduce code. Right comments are:

Wrong deletion of an alias:

    path_set_alias('pathA', 'aliasA');
    path_set_alias('pathB', 'aliasB');
    // change path for aliasA (aliasA should point to the same node as aliasB)
    path_set_alias('pathB', 'aliasA');

Ignorance of path change for an alias:

    path_set_alias('pathA', 'aliasA');
    path_set_alias('pathB', 'aliasB');
    path_set_alias('pathB', 'aliasC');
    // change path for aliasA (aliasA should point to the same node as aliasB and aliasC)
    path_set_alias('pathB', 'aliasA');

@Damien Tournoud

The "Wrong deletion of an alias" is a false issue: the first parameter of path_set_alias() is the destination, the second one is the source. There can only be one destination for a given source, so the current behavior is expected.

That's not correct. The first parameter is source, the second is destination:

function path_set_alias($path = NULL, $alias = NULL, $pid = NULL) {
  ...
  db_query("INSERT INTO {url_alias} (src, dst) VALUES ('%s', '%s')", $path, $alias);
  ...
}

So "Wrong deletion of an alias" is a true issue!

@earnie

I understand your arguments why path and alias should be a 1:1 relation. But regardless my bug report that's not how drupal 5 core works. Changing the relation from 1:n to 1:1 will be change of the API which is not good for a stable version.
From my point of view the core should allow 1:n and the optional pathauto module has to ensure 1:1 relations.

BTW If you only install the core you can access a node via it's alias and it's number. From a SEO point of view that's already duplicate content. So duplicate content could not be an argument to force 1:1 relations.

@earnie
I interpret "Set an aliased path for a given Drupal path, preventing duplicates" as preventing duplicates of aliased path / Drupal path combinations.

Nevertheless path_set_ailias is buggy and a patch is required. The question is, in which direction?

If you decide to set a unique constraint on 'src' or on the combination of 'src' and 'dst' in the database the update will fail at some of our installations and I'm convinced it will fail at others too.

What do you think about extending the path module and make the behavior configurable?

The patch attached to comment #13 doesn't work.

function path_set_alias($path = NULL, $alias = NULL, $pid = NULL) {
  ...
  else if ($path && $alias) {
    ...
    $alias_count = db_result(db_query("SELECT COUNT(dst) FROM {url_alias} WHERE dst = '%s'", $alias));
    ...
    else if ($path_count && !$alias_count) {
      if ($pid) {
        ...
      }
      else {
        db_query("UPDATE {url_alias} SET src = '%s', dst = '%s' WHERE src = '%s' OR dst = '%s'", $path, $alias, $path, $alias);

When reaching the UPDATE statement, $alias could not already exist as dst in database. So "OR dst = '%s'" is senseless.
Additionally if there are already stored several aliases (dst) for the source (src) they will all be updated to the same values for dst and src which will fail with a database error due to a unique constraint violation on dst.

@Damien Tournoud

Please review and test #9 instead.

Did you see my comment #10? I think there's a still problem with your patch in #9.

@Damien Tournoud
Your patch solves both issues. Expected and real result are the same in both cases.

But there's a little thing remaining which is a difference between our patches.
In your case the behavior remains that the dataset for aliasA is not changed but deleted and created again which results in a new pid due to the auto_increment.

I'm of the opinion that if someone uses the third parameter pid of path_set_alias to modify an alias, he expects that it's stable and not changed.

Same thing happens if you simply set the same alias again:

  path_set_alias('pathX', 'aliasX');

mysql> select pid, src, dst from url_alias;
+-----+-------+--------+
| pid | src   | dst    |
+-----+-------+--------+
|   1 | pathX | aliasX |
+-----+-------+--------+

  path_set_alias('pathX', 'aliasX');

mysql> select pid, src, dst from url_alias;
+-----+-------+--------+
| pid | src   | dst    |
+-----+-------+--------+
|   2 | pathX | aliasX |
+-----+-------+--------+

@drumm

So I do think we need to preserve pids.

So please review the patch I initially posted. It fixes all the issues mentioned in this thread and is preserving pids.