Closed (fixed)
Project:
Pathauto
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
1 Mar 2008 at 13:45 UTC
Updated:
14 Aug 2010 at 17:00 UTC
Jump to comment: Most recent file
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
Comment #1
gregglesThis 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?
Comment #2
Bodo Maass commentedI 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.
Comment #3
marcvangendThe 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.
Comment #4
chellman commentedWith 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.
Comment #5
gregglesMarked #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)
Comment #6
gregglesSo, 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.
Comment #7
greggles...surprising them in a 5.x-2.2 is not something I want to do...
Comment #8
robb commentedAttached 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:
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.
In any case the patch seems to work and I will be testing it all this week.
Comment #9
Bodo Maass commentedMy 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.
Comment #10
robb commentedThe 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.
Comment #11
xjmTo 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.
Comment #12
robb commentedHm... 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.
Comment #13
Freso commented@robb:
Something like #212208: allow other modules to affect strings (to help with custom accent/string replacement), you mean?
Comment #14
fletchgqc commentedHi 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]
Default action to take on nodes where a path alias already exists (whether created by pathauto or not) [radio selection]
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.
Comment #15
marcvangendfletchgqc, 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.
Comment #16
fletchgqc commentedThanks. 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.
Comment #17
marcvangendI'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
Comment #18
fletchgqc commentedOK 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.
Comment #19
millions commentedIs 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?
Comment #20
fletchgqc commentedThis 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.
Comment #21
xqi commentedsubscribing.
has this patch been applied to the 1.x dev version yet?
Comment #22
gregglesNo, 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.
Comment #23
dave reidThis is now how the module works. Please re-open if its not the case and you've tried with the latest code.
Comment #25
DoctorWho commentedIs 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.
Comment #26
kingandyNeither 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:
.. 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?"
Comment #27
EvanDonovan commentedgreggles 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?
Comment #28
fletchgqc commentedEvanDonovan, 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.
Comment #29
dave reidMarking #285042: "Do Nothing" Should Still create aliases for unaliased items as a duplicate of this issue.
Comment #30
EvanDonovan commentedSorry, 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.
Comment #31
dave reidPatch against D7 with an extensive test for all the update actions attached for review.
Comment #32
dave reidLatest 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.
Comment #34
dave reidRevised 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().
Comment #35
dave reidHelps to spell 'language' correctly...
Comment #36
dave reidComment #38
dave reidOne more try before bed...
Comment #39
EvanDonovan commentedSorry to clutter up the issue, but does this mean that contrib modules with test suites can have patches automatically tested now?
Comment #40
marcvangend@EvanDonovan: it's in beta, see #689990: Contrib projects to be included in beta stage of automated testing for modules.
Comment #41
dave reidAlso 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.
Comment #42
dave reidThanks 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?
Comment #43
xjmHooray! 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.
Comment #44
dave reidCommitted to 6.x-1.x with tests: http://drupal.org/cvs?commit=400824