I have set my update action to 'Do nothing'. The way I understand this, it should do the following:
- When saving a node that already has an alias, nothing should happen.
- When saving a node that has no alias yet (and the box "Automatic alias" checked), a new alias should be created.

However, when I edit and then save a node without an existing alias, nothing happens. I looked at the code and I believe that the intention of the code is to do what I just described, so the failure to do so is a bug (in my view).

To fix this, please find the attached patch which changes a part in pathauto.inc from this:

  // Special handling when updating an item which is already aliased.
  $pid = NULL;
  $old_alias = NULL;
  if ($op == 'update' or $op == 'bulkupdate') {
    if (variable_get('pathauto_update_action', 2) == 0) {
      // Do nothing
      return '';
    }
    $update_data = _pathauto_existing_alias_data($src);
    $pid = $update_data['pid'];
    $old_alias = $update_data['old_alias'];
  }

into this:

  // Special handling when updating an item which is already aliased.
  $pid = NULL;
  $old_alias = NULL;
  if ($op == 'update' or $op == 'bulkupdate') {
    $update_data = _pathauto_existing_alias_data($src);
    $pid = $update_data['pid'];
    $old_alias = $update_data['old_alias'];
    if (variable_get('pathauto_update_action', 2) == 0 && !empty($old_alias)) {
      // Do nothing
      return '';
    }
  }

Comments

greggles’s picture

This looks like a good fix, but I'm not sure if it is the appropriate behavior. It's been this way for a long time and I think people expect the current behavior. Could you get some feedback from other pathauto users (perhaps using http://groups.drupal.org/paths ) so that we can get a consensus on this?

Bodo Maass’s picture

I can see that people may want to leave some nodes unaliased. Although when I edit a node and check the box "Automatic alias. An alias will be generated for you." I would expect an alias to be generated for me if there isn't one. The particular inconsistency arose from creating translations with the localizer module.
Step 1: Create a new node 'My english page'. Alias gets created fine.
Step 2: Create german translation for this node 'Meine deutsche Seite'. No alias gets created. This is because Localizer first creates the new translation programatically, and then opens a page to edit it. So to pathauto, this is an update action instead of an insert action, and it will therefore not create an alias. To me as a user, this is just confusing and feels inconsistent.

I thought for a while that the option "Create a new alias. Leave the existing alias functioning." does what I want, but this will always create a new alias in addition to the old one.

Maybe the update action should have another condition to preserve compatibility with the existing behaviour:

Update action:
1. Do nothing. Leave the old alias intact and don't create new aliases for unaliased nodes.
2. Leave existing aliases untouched, but create a new alias if there isn't one yet.
3. Create a new alias. Leave the existing alias functioning.
4. Create a new alias. Delete the old alias.

marcvangend’s picture

The option "Leave existing aliases untouched, but create a new alias if there isn't one yet" is exactly what I was looking for, so I'm glad to find that there is already a feature request for it. I would really like to see this option in a future version of pathauto.

However I think the text of this option should be formulated differently, so it is more in line with the other options:
2. Leave the existing alias functioning. Create a new alias if no alias exists.

chellman’s picture

With this patch, it looks like exactly what I expect from the options as they're named. It also seems like a good behavior (not just what I expect).

I ran into this with bulk updates where unaliased items (I deleted them manually to make sure there weren't any) would never get a new one unless I changed the update action.

If the current behavior does need to be preserved, I like Bodo Maass's idea of adding a new update action, and making sure they're all named as clearly as possible. If that happens, maybe some more documentation with use cases for each one could be a good idea.

greggles’s picture

Marked #256059: pathauto_create_alias doesn't fire if node exists but alias doesn't as a duplicate. Note that it has a slightly different solution (in non-patch form)

greggles’s picture

Version: 5.x-2.x-dev » 6.x-1.x-dev
Category: bug » feature
Status: Needs review » Needs work

So, Freso and I just discussed this. I think that we should implement this as a new update action and push this to the 6.x-2.x branch of code since it will be a more fundamental change. Old users get used to the behavior - right or wrong - and surprising them in a 6.x-2.2 release is not something I want to do.

So, given that, I'm assigning this to 6.x-1.x-dev (it should be 6.x-2.x-dev but we haven't branched yet so we don't have it in the dropdown...) and making it a feature.

@Bodo Mass or anyone else interested - if you could please re-roll this to function as a new update action it would be great. It will probably need a re-roll once 6.x-2 is branched, but if it's ready to go at the beginning we could commit it towards the beginning.

Thanks.

greggles’s picture

...surprising them in a 5.x-2.2 is not something I want to do...

robb’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
StatusFileSize
new3.16 KB

Attached is my attempt to resolve this issue, see also #274079: Aliases canot be set if Update Action is : Do nothing.

I have adjusted this to version 6.x.2, I hope that is appropriate.

The following changes were done:

  • Added a new option that allows a blank alias for an existing node to be set
  • modified the settings help text to remove the 'which already as an alias' text
  • Set options values instead of using defaults. This preserved all current settings
  • Added code to implement this logic
  • And the part I do not like: code to DELETE an existing /feed when it is updated in this way. Failure to do this results in messy URL entries and posisble database duplicate errors.

Some future ideas related to this. This type of idea is only useful for a site that has a mix of user defined URLs and auto generated ones. Most of my new sites will tend to have 10-20 manual URLs and the rest (100+) auto. With some auto's changing to manuals. So it can all get rather confusing.

  • A check box that marks the node as auto-maintain or manual. This would override global settings per node. But that will require either another table or possibly using 'src' in a unique name (?node/120 == managed by path auto) - I think a table in this case is probably better!
  • OR, detect if the path setting in the form is blank AND if the pathauto is checked then rebuild the alias regardless of current value. Not as clean, but might be easier ti implement.

In any case the patch seems to work and I will be testing it all this week.

Bodo Maass’s picture

My main problem with pathauto changing things is search engine optimization. It's fine for pathauto to suggest an initial path, but after that it should leave the path alone, so that the search engines can get used to it.

So the behaviour option that I would like to see is that pathauto should create a path if there isn't one yet, but otherwise it should do nothing. This is what has been discussed earlier in this thread. For my case, I don't see that I would need a checkbox to differentiate between auto and manual maintain.

robb’s picture

The new option should NOT change a URL once assigned. It simply allows a node with an existing BLANK URL (or that becomes blank) to have an alias auto generated. At least that was my intent. No prior functionality should be affected. If it is changing existing URLs then that is very bad.

I will start a new feature request for my FUTURE items.

xjm’s picture

To clarify (there seems to be some confusion): the option "Do nothing. Leave the old alias intact" does nothing even if there is no old alias--that is to say, it pretty much disables the module. This is a change from the behavior in the 5.x-2.x dev version from last July that I was running previously. Previously, with the "leave the old alias intact" setting, existing aliases were ignored, but paths were still created for nodes with no alias.

Tracking.

robb’s picture

Hm... Yes that is how I designed it. There were cases where we did not want ANY aliases.

I am currently on standby with this as there are about a zillion Path Auto tasks running now and I am not sure the proper direction to take for this. It kind of feels like Pathauto should have a hook to allow others to alter its behavior and UI without mucking with the core. But for now I am just trying to keep up with the project.

Freso’s picture

fletchgqc’s picture

Hi all,

+1 to robb's ideas in #8 to be able to mark nodes as having auto-maintain or manual-maintain paths. If it requires a DB table so be it - it's needed. Any site with a mix of auto and manually maintained URLs has trouble using pathauto without an option like this (you have to always remember to untick pathauto and retype the manual URL every time you edit the node - big hassle and error-prone). If this idea is feasible then I see no reason not to code it now while a new version of pathauto is being developed anyway.

Also I agree with the general consensus here that Pathauto should do what the English on the settings pages actually specify - create a new URL when there isn't one already.

If we don't go for robb's "mark node as auto or manual-maintain URL" idea then here's another which I quite like - it might sound a bit radical but I think it's the right way forward:
The node edit page should always give the user the option of what they want to do with that particular node's path alias. The module configuration page should set the defaults.

So on the module configuration page you should have something like this:

Default action to take on nodes with no path alias already defined [radio selection]

  • Let pathauto create a new alias.
  • Specify new alias manually (select this option and leave field blank for no alias creation).

Default action to take on nodes where a path alias already exists (whether created by pathauto or not) [radio selection]

  • Manually control alias (allows you to leave alias unchanged).
  • Create a new alias via pathauto. Leave existing alias in place.
  • Create a new alias via pathauto. Delete the old alias.

Then when you edit a node, in the URL path section you will be faced with the respective list of radio options depending on whether it already has an alias set or not. The path text box will also appear and will be populated with the existing alias (if there is one).

Most of the people who have commented in this thread will choose option 1 - "Let pathauto create a new alias" as their default action on new nodes, and option 1 "Manually control alias" as their default option on nodes with existing aliases. With "Manually control alias", just don't change what's in the text box and your existing alias will be preserved.

This allows great flexibility - if you like to keep your URLs unchanged you can set that as the default, but if one day you change the title of your node dramatically you might elect to change the URL on that node by clicking that option... or while you are working on the draft of that node... etc.

marcvangend’s picture

fletchgqc, I really like where this is going, the changes you propose will indeed offer a great deal of flexibility. Two remarks:

- You write "The path text box will also appear and will be populated with the existing alias (if there is one)", but don't forget there can be multiple aliases already. When we do it like this, we'll have to complete and give the user multiple text fields with all existing aliases.

- We're talking about a very advanced pathauto fieldset, with a lot of options. Drupallers are fond of options, but end users may get confused. Site developers must be able to choose if they want to present all options to certain roles. In other words: once we know what we want, we really need to think it trough on the permission level.

fletchgqc’s picture

Thanks. Well, regarding the multiple aliases already existing issue I would just have a look at how it is dealt with by the path module (without pathauto) and mimic this. I guess that they just grab one of the aliases and operate on that, ignoring the other (but haven't checked). Since this is not a common thing, you could perhaps just do that for simplicity's sake.

Good point about the many options. Of course you also want to retain simplicity for the developers! As a developer I will simply tell some end users "don't change this option" and generally they are happy - that's all they need to know to get started. There are lots of options on the node edit form that most users wouldn't consider touching. Of course I'm open to other suggestions.

marcvangend’s picture

I'm not sure if multiple aliases is not a common thing. Maybe we just don't notice... when we choose "Create a new alias. Leave the existing alias functioning", the number of aliases for one node can easily increase.

I never like it when I have to tell a customer "you see this? don't touch it." It's like underlining the shortcomings of yourself and the CMS. I can think of some permissions I would like to set:
- manually override the pathauto default
- delete existing aliases
- administer pathauto settings

fletchgqc’s picture

OK I've discovered that the issue of basic pathauto function in maintaining aliases manually etc. has been the subject of discussion since "time immemorial" and the bottom line is everyone agrees that it needs fixing but no-one has the time and motivation to do it! So in that case I'm sorry for hijacking this thread... we should actually just commit the patch that fixes the problem described in the title and then talk about the other functionality elsewhere... like here #180440: If an alias is manually created, don't automatically replace it on edit.

millions’s picture

Is this a patch that can be applied to 5.10? I see that it was originally for 5 but the issue has been moved to 6.

Also, in terms of functionality and search engines, is it recommended to leave the old alias intact or replace it? Is there an easy way to find aliases pointing to the same node for maintenance?

fletchgqc’s picture

This thread has died.

Did this already get fixed separately in 2.x or is it still an issue? If not fixed how about we look at committing one of the proposed patches? In my mind it's actually a critical bug because some of the core functionality of the module is not working, that is, it's working differently from the description of the functionality.

xqi’s picture

subscribing.

has this patch been applied to the 1.x dev version yet?

greggles’s picture

No, it hasn't been applied and no, it hasn't "died". If it gets applied Freso and I are very consistent in marking it as fixed and noting the commit message.

The status is "needs work." @fletchgqc - could you review/test the patch and, if you like one, state it is ready to be committed.

This thread has been hijacked all over the place. That makes it harder to come back and review and understand what is going on.

dave reid’s picture

Status: Needs work » Fixed

This is now how the module works. Please re-open if its not the case and you've tried with the latest code.

Status: Fixed » Closed (fixed)

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

DoctorWho’s picture

Is this fix already in 6.x-1.3, or is it only in -dev? I tried it in 1.3 and it did not work for me, but I'm still not sure if it is supposed to work already with this version. I tried looking through the commit messages but I did not find any clue in which version this is fixed.

kingandy’s picture

Status: Closed (fixed) » Active

Neither the patch in #8 nor the originally-suggested logic change appear to be present in either 6.x-1.3, 6.x-1.x-dev or 7.x-1.x-dev.

pathauto_create_alias in 7.x-1.x-dev currently reads as follows:

    if (variable_get('pathauto_update_action', PATHAUTO_UPDATE_ACTION_DELETE) == PATHAUTO_UPDATE_ACTION_NO_NEW) {
      // Do nothing
      return '';
    }

.. Which is essentially the same as before (using constants instead of hardcoded integers). It appears in the same position and does the same thing.

Personally, I'd be fine with the "Do nothing on update regardless of whether or not there is an existing alias" behaviour, except that the #description for the radio element is the pretty unambiguous "What should Pathauto do when updating an existing content item which already has an alias?"

EvanDonovan’s picture

greggles referred me to this issue from IRC. I am unclear as to the best way to proceed with it, since it seems the issue has changed scope multiple times. I would like to see aliases created for nodes through Views Bulk Operations's bulk update paths function when they don't already have them, even if "Do nothing" is set.

Should I roll a new patch for that?

fletchgqc’s picture

StatusFileSize
new1.31 KB

EvanDonovan, if you want something that changes the meaning of the first option into "create alias when there is none" then attached is a patch against 7.x-dev that I think does that. I haven't tested it. It's based on Bodo Maass' patch, but I think he missed something which I added. If you want to make this change, it must be done NOW before 7.x is released.

After writing this patch, on consideration I've realised that my problem is that the title "Update action" makes me think of updating a node in general but the grey help text underneath refers to updating a node that already has an alias - which is a different thing. As it stands the module is actually technically doing what it says in the grey text - maybe someone changed that text since this issue first started. If you want to change this functionality, such as by applying my patch, you have to change this grey text slightly.

Sorry for the turnabout, and despite attaching the patch, I've become fairly impartial on the issue. I think that we should probably not change the functionality, if you want to change it I think the only correct thing is to add another option along the lines of what is described in #2 and #3. This is not what my patch does, but if my patch works then it should be easy to adapt it to one of the changes descibed there.

dave reid’s picture

Title: Editing nodes without an alias should create an alias even with update action = 'Do nothing with existing aliases' » Un-aliased nodes should still get an alias generated even if the update action is 'Do nothing with existing aliases'
EvanDonovan’s picture

Sorry, haven't had time to test this; my organization's site is currently running 6.x, and we passed the time when we faced this issue.

dave reid’s picture

Assigned: Unassigned » dave reid
Status: Active » Needs review
StatusFileSize
new4.75 KB

Patch against D7 with an extensive test for all the update actions attached for review.

dave reid’s picture

Category: feature » bug

Latest patch does not add a new action because I feel strongly this is a bug. The description for the action field clearly states "What should Pathauto do when updating an existing content item which already has an alias?". This action should not prevent an un-aliased item from being generated.

Status: Needs review » Needs work

The last submitted patch, 228762-pathauto-donothing-except-when-unaliased-D7.patch, failed testing.

dave reid’s picture

Status: Needs work » Needs review
StatusFileSize
new15.74 KB

Revised patch that should pass tests. Makes _pathauto_existing_alias_data() a pure query & return function for simplicity, and moves the update action logic to where we should care about it in _pathauto_set_alias. Converts the pathauto_create_alias() and _pathauto_set_alias() functions to work with the raw results from _pathauto_existing_alias_data() and to also build a proper URL alias array to pass to path_save().

dave reid’s picture

Helps to spell 'language' correctly...

dave reid’s picture

Status: Needs review » Needs work

The last submitted patch, 228762-pathauto-donothing-except-when-unaliased-D7.patch, failed testing.

dave reid’s picture

Status: Needs work » Needs review
StatusFileSize
new16.4 KB

One more try before bed...

EvanDonovan’s picture

Sorry to clutter up the issue, but does this mean that contrib modules with test suites can have patches automatically tested now?

marcvangend’s picture

dave reid’s picture

Also leading to my bug report, this was a regression introduced in #188606: universal aliasing for taxonomies (was: separate aliasing for image galleries). There's nothing in that issue about changing the behavior for un-aliased nodes.

It also fixes a bug with creating alternate feed aliases even if there was 'Create a new alias. Delete the old alias.' option selected. To sovle this (and to reduce code) I introduced a new function _pathauto_existing_alias_data() which consist of part of the pathauto_create_alias().

dave reid’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thanks everyone! Committed to 7.x-1.x and 6.x-2.x:
http://drupal.org/cvs?commit=400806
http://drupal.org/cvs?commit=400808

Should we backport the basics of this to 6.x-1.x?

xjm’s picture

Hooray! Trumpets! Fanfare! Thank you Dave Reid. Hopefully now I can start using pathauto for static site pages again. :)

A backport to the 1.x branch would be appreciated if it's not too much trouble, since there doesn't appear to be a 2.x release yet.

dave reid’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Fixed

Committed to 6.x-1.x with tests: http://drupal.org/cvs?commit=400824

Status: Fixed » Closed (fixed)

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