Pathauto Overwrites Existing Callbacks

I'm getting this too. Have a View set up a /foo and want all foo content types to be at /foo/[date]/[title] but the index aliases keep creating a /foo index alias.

I'm brainstorming ways to get around this as right now we just have this:

case 'user':
case 'blog':
case 'admin':
  // Don't create alias to system paths

Thoughts?

Darren wrote "prevent the Pathauto module generating URLs that already exist"

Is there a list of existing URLs somewhere?

Also, I'm not sure that preventing is the right behavior. I can conceive of users wanting to over-write those URLs on purpose (I know I have done this at times).

Another possible solution would be to have the "Create index aliases" on a per pattern basis. So, in RobRoy's example you would set the pattern you want for content types /foo/[date]/[title] and then you would turn OFF index aliases for that content type. This makes the interface and functioning more complex, but gives the user more control.

Check out this patch. It checks to see if the requested destination alias is an existing callback.

So 'blog' is a callback and won't allow an alias, but 'blog/1' is allowed. I opted for the former to be a bit less restrictive. I'm looking for some input though.

And this is a bug IMO.

On second thought, this seems like a bug with path.module since that is what ultimately is generated these bogus paths. Should we really be allowing users to create an alias of 'admin' for 'node/1'? I think not.

@RobRoy - any feeling on how to handle the situation where the user WANTS to over-write the "blog" page?

If we implement your patch it will just be a month before someone comes back and says "PathAuto won't let me over-ride the blog/ page and I really want to do that"

Yeah, I think we could have a setting in there for "advanced users only" to toggle this behavior (or, allow a user submitted list of URLs NOT to alias?). I'd like to get some more input on this first, before making a solid patch. (Sorry about the lang stuff in there, for some reason I can't get that clean.)

Yeah same problem here with Event-module.. Whenever an event is created it overwrites the /event/-path, and that particular path is pretty crucial since it provides the view for the calendar table.

Could be great if there would just be something like "ignore these paths" or something and then the user could write "event" there, besides the available user, blog and admin.

@jari - that sounds pretty reasonable.

One problem with the solutions that put the control in the user's hands (including my own proposed solution) is that they require the user to know lots of stuff that is not common knowledge. I'm starting to think that a combination of robroy's solution and jari's solution would be good. E.g. display a list of existing callback menu items to the user each time the settings/pathauto page shows and request that the user confirm items into a textarea list of items that pathauto won't over-write.

Should this go to the path.module? We already don't allow duplicates in path_set_alias() and I think we should not allow certain aliases there as well.

What about allowing modules or the menu.module to register callbacks and whether they can be overwritten or not. For example, when system_menu() registers the 'admin' callback, could we specify an 'overwritable' => TRUE (defaults to FALSE)? Kinda clunkey, just throwing that out there.

But by default, I think we should *not* allow aliases to overwrite existing callbacks, but allow overwriting either in an advanced settings page or with some sort of hook_path_alter() or whatever.

Or, maybe in pathauto we don't allow overwriting but you can manually overwrite an alias in admin > url aliases.

Eh?

Yeah, I really think that we should prevent multiple aliases. I know that pathauto has an option for "create a second alias leaving the current alias alone" but that seems like a bad idea on multiple levels...

This seems like something that belongs in core node/path modules...Hopefully I'll get around to doing something about it.

Review:

• line 433: the drupal_set_message needs to be translateable with t() and I think we should start saying why we aren't setting the alias, something like t("Ignoring alias "). $dst .t("due to existing path conflict")

• Other than that, it worked fine in my tests.

  About the "@todo: This menu call might not be all-inclusive"

  If you look at menu_get_menu and _menu_build it seems that the $user is mostly for cache and items in the $menu['visible'] portions of the returned array. I think the callbacks should be complete.

  http://api.drupal.org/api/4.7/function/menu_get_menu
  http://api.drupal.org/api/4.7/function/_menu_build

  Also, I've documented the message so that people can easily find how to override. - http://drupal.org/node/83957

  Attached is a cleaned up version of the patch. If you agree with it then I'll commit it.

I'm not very fund of this piece of code, and also the sentence itself strikes me:

+      drupal_set_message(t('Ignoring alias '). $dst .t(' due to existing path conflict'));

It should be something like:

  drupal_set_message(t('Because the specified alias %alias is already taken, bla bla bla', array('%alias' => theme('placeholder', $dst))), 'error');

Setting status to 'code needs work'

@Darren - can you be more explicit? Callbacks work differently than "normal" aliases so we have to treat them differently to some extent. But there is certainly an argument for consistency in certain areas - especially in the user interface.

@Stefan - pathauto currently says "Ignoring alias ..." in several places. I like the current sentence because it is consistent. Do you propose that we change it to your style in the other places where we use "Ignoring alias..."?

Thanks for the review.

Darren - I think we're headed there.

I talked about this more with Steef in IRC and we agreed that we shouldn't hold back this patch based upon his dislike for the wording.

So, I created a new issue to track the wording changes and have committed the patch to HEAD and 4.7.

Thanks everyone!