[ Please credit fago; this patch appeared in http://drupal.org/node/269877#comment-3351152 .
It's only been put into a separate issue to -hopefully- streamline fixes, and renamed to fit this issue. ]

Premise behind this issue: When updating an existing path alias through path_set_alias(), it is always best to pass the 'pid' field for the existing alias. (And this will stay true, irrespective of what changes may or may not be made in path_set_alias() through other issues.)

This often isn't done.
path_nodeapi('update') checks for the presence $node->pid, but since this is not a 'regular' node property, $node->pid is not present except after a manual node edit (as filled by path_form_alter()). Any other node_save() call will result in path_set_alias() being called without the 'pid' argument filled.

The attached patch fixes this.

While this patch does not fix potential problems inside path_set_alias(), the beauty of it is:

- given above premise, it is easy to deduce that there are no adverse effects. It does not need any tests or discussions about which exact circumstances triggered a bug. (Unlike some of the fifteen million other open issues relating to path_set_alias() and i18n and updating nodes.)

- it has no influence on the fifteen million other patches floating around, since the change does not touch the code in these patches. It can therefore be applied independently.**

- it may however have a mitigating effect on the circumstances that trigger the issues with path_set_alias(). It is therefore hoped that it will make discussions w.r.t. solving the other issues easier - like determining whether or not some issues are duplicate.

**I didn't check all fifteen million issues, but IMHO this holds true for:
#269877: path_set_alias() doesn't account for same alias in different languages #218 (and#228 too)
#790338: All aliases get deleted when translatable nodes with same URL alias are reverted to previous revision
#370089: warning shown when the same url alias name is used for different languages
#358722: Node aliases lost/changed when using i18n synchronize translations (separate issue from above three)

CommentFileSizeAuthor
path-setalias-pid_D6.patch1.34 KBroderik
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

roderik’s picture

FWIW: note that the extra database query does not increase overhead, because it reduces the checking inside path_set_alias() in many cases.

roderik’s picture

Assigned: Unassigned » roderik
Status: Needs review » Needs work

Uh-oh.

The deleted (from the form_alter) select statement selects the pid by dst. (Which is kinda logical. If the user is changing the textbox in this alias, she's overwriting the current alias.)

The new (in the nodeapi) select statement selects the pid by src.
This isn't that logical / is a functionality change.

Explanation:
Despite 'every day usage' (which is one alias per node), nodes can actually have several aliases. (See Pathauto setting "Create a new alias. Leave the existing alias functioning.")
After the patch included here, it will pick one alis and overwrite it with a new one.

But current behavior is basically "If $node->pid isn't set, we don't know whether or not this is a new alias. So we don't know whether we want to overwrite anything or create a new alias. Leave that up to path_set_alias() to decide".

*sigh* I got myself in knee deep again, by commenting on someone else's patches.
Will need to look at this later - and if I still agree with myself, replace the current patch with a "no code, only extra comments" patch.

roderik’s picture

Status: Needs work » Closed (won't fix)

Closing this.

Not only does it a potential behavior change as indicated in #2 (if $node->path is different, it overwrites the alias, where before it would add a second one), which is creepy in a stable Drupal release...

...but the patch in #269877 is also more complicated than it should be IMHO. So when that one is simplified, this one doesn't require a separate issue.

(If we'd decide to introduce this behavior change anyway, then we'd need to add a "ORDER BY pid DESC" to the query, to be consistent.)