Allow unique "Update Action" settings per item type (node, taxonomy,etc) or item (eg. page, story, vocabulary, etc)
| Project: | Pathauto |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Moonshine |
| Status: | needs review |
Hello!
I've been working on a patch to clean up what I feel is some confusing behaviour re: the pathauto_update_action options in the admin and some of the associated UI. While I *thought* I was near completion, the more I looked the more questions started popped up. So I'm really hoping for some input here.
Currently, users have the following 4 options for pathauto_update_action (if they have path redirect also installed).
Update action:
0 = Do nothing. Leave the old alias intact.
1 = Create a new alias. Leave the existing alias functioning.
2 = Create a new alias. Delete the old alias.
3 = Creat a new aliast. Redirect from old alias.
What should pathauto do when updating an existing <i>content item</i> which already has an alias?So the big first question is: What is considered a "content item" ?
From the code, and experimentation, Pathauto currently considers a "content item" to be everything -- nodes, users and taxonomy aliases! Personally, I would think "content item" would just mean individual nodes though, for a couple reasons.
a) The core Drupal admin considers "Content items" to be individual nodes that are created.
b) I need it to be that way! :) Hehe..
But seriously, I don't think I'm alone here...
I'd like for existing node aliases to *not* be altered by node edits (especially for pages), but paths to users and taxonomy terms to always follow their current values. (ie. Fred changes his username to Freddy, I only want him at user/freddy now, and the same for taxonomy terms.)
I understand that others could have different requirements, but I think more flexibility and clarity would be useful to all.
Really I'd like to be able to set the "Update action" for nodes, users and taxonomy individually. (Ideally for each content type even -- I don't want my "pages" to ever move)
I'd like to get some of this in before an "official" Drupal 6 release of Pathauto, and I'm willing to do most of the legwork here submitting patch(es) if there is interest. I just don't want to end up with something that isn't useful/used in the end. I already do enough of that at my day job. :)
#1
The code leads me to believe (as you) that it means nodes, users, and taxonomy aliases.
I think that what you are proposing is:
1. update action specific to users
2. update action specific to taxonomy
3. update action specific to node type
right? That seems fine by me, but...the UI is already overly complex. I'd like to do this as follows (see if you agree):
1. Change the current help text to say "objects" or "items" or something so that it is more clear.
2. Leave the current update action as the "default" update action
3. Add code that checks the update action based on the three criteria above but defaults back to the "default" UI if no variable is present
4. This is a non-action, but is worth stating: don't add UI for setting those more specific variables. If someone wants to use these they'll need to set the variable themself...
Basically I think we should have a facility for these more fine grained things, but if we add them to the UI then it becomes unwieldy. So, we add the code for the option but not the UI and users can set the UI elsewhere. What do you think?
#2
You're correct, I was proposing:
However, I guess I was heading a different direction on implementation. I was planning on adding the settings to the GUI. I just think quite a few people do have a use for these specific options, but won't even know about them (much less set their own variables :) ) otherwise.
I guess I was sort of thinking of the following:
- The current "Update Action" radio box list in the General section is phased out.
- Under each section/content type, there is a nice "slim" select list with the 3(or 4) options. Initially these select lists will just default to the current value that was in pathauto_update_action.
Mind you I haven't looked through all the code yet to see how engrained "pathauto_update_action" is currently, but I think it should be doable.
So I guess I should find out here first, are you looking to avoid the admin form entries because:
a) the form is getting long
b) people are already confused :)
c) there is documentation involved
d) there is some code I haven't seen yet
I certainly won't proceed until we have things worked out further...
Thanks!
#3
After looking at the current Pathauto admin form further, I see where you might be heading with form length once there are lots of content types (or potentially vocabularies if we expand it to that).
I'll have to mull it over it bit. I'd really like the functionality to be in admin though, even if it's hidden in some way.
Maybe just a fieldset/section for "Update action settings" all in one location after General settings? I think it might be logical to have them all in one area like that which allows the user to think through update actions all in one area. Also it can be collapsed by default and won't need to have repetitive help information.
#4
Definitely only A and B (which are related).
I'm basically not accepting patches which extend the length of the admin page unless it's for something that 80% of Pathauto users will use/need. The form is just too long. Given that this module has been around for over 3 years and nobody has requested this feature, I'd say that it's a "corner" case and not deserving of any UI space.
How about we split this into two issues. The first issue (this one) could implement the code to leave the current sitting visible, use it as the default, but respect variables that are named in a way that people can figure out and customize themselves. It should also document it in the README.txt and/or on the Pathauto help page.
Separately, create an issue that adds radio buttons to the UI. That way if people really want those radio buttons they can apply the patch and vote +1 for including it.
While I agree that features based on hidden variables are unlikely to be changed, this is a concept (common features are exposed in admin UI, uncommon features are controlled by a hidden variable system) which I think we need to embrace in Drupal in order to balance extra features with a comples UI.
#5
Sounds fair! Although I doubt that massive "Punctuation" section would meet the 80% criteria. :) Sorry, couldn't resist...
Anyways, I'll start working on the first portion and see what you think.
Have a good weekend...
#6
I agree - the punctuation area is something that needs to be taken away and I have plans, but not time, to fix it. It was the "straw" that broke the camels back... (though it was more than a straw!).
#7
Well here is a first pass at the first portion...
Basically it allows you to overide the pathauto_update_action via other system variables at the module or module/type level. It's working for me with all my tests, but of course I look forward to other people kicking the tires. Hopefully without there being an admin section "yet", people will still give it a whirl.
There are just a few side things to note:
Basically when you go in to edit the node, the "Automatic Alias" checkbox is still ticked for mode 0 but is then ignored later in the code. I think this is unexpected behaviour. So now it's set to be unticked for mode 0, and ticking it will create a new replacement automatic URL as described. This way, by default, you have the proper mode 0 behaviour still and a checkbox that makes sense. In addition those with proper permissions can manually change the path or have a new path automatically created.
Anyways, I'm open for any comments or changes that will help my chances on this first stage :)
Thanks..
Edit: I should mention this is a patch against the current 6.x-1.x-dev dated 2008-Apr-05
#8
Sorry, I just saw the issue for #2 above at:
#228762: Editing nodes without an alias should create an alias even with update action = 'Do nothing with existing aliases'
However the fix I have in place here is needed relevant to the rest of the patch...
+1 on getting that checkbox fixed though, espcially as it doesn't affect the "default" (ie. user not clicking anything) behavior on editing.
#9
I like the idea of disabling the checkbox if update action == 0 - thanks for fixing that as well (though I generally prefer separate issues...I do understand that it's easier to combine them sometimes).
This seems reasonable. Can you just confirm what the behavior is if the user does not have "administer path aliases" permission? Thanks.
Also, I'm trying to get 5.x-2.2 out the door and will likely not commit this until after that at which point it will sit in the -dev version for a little while. Basically, I do definitely appreciate your work but may not commit this for a while do to other priorities with the project and due to my personal schedule. Thanks for your help on this.
#10
Sure, find a typo within hours! :/ Looks like I wrapped a pair of quotes around $node->type on a conditional statement. I've stripped them and things work fine for me both with and without permissions now.
During some further testing, I actually uncovered a couple other bugs in the current Pathauto code, which this *new* patch fixes as well. Sorry to toss them in here, but really I had to fix them in order to make sure things test clean here. (also I'm on a deadline here also for a project that will lean on pathauto heavily. :) ) They are rather simple bugs but they can have large results.
Basically in the current Pathauto code there is a section in pathauto_create_alias() as follows:
<?php// 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 '';
}
?>
This causes several problems, as it's not really testing for an existing alias. It just assumes that, if it's an update or bulkupdate, there must be an existing alias. So bulk updates will never occur for items when mode 0 is enabled. Also updates of existing items, that don't currently have an alias, will never have one created w/ mode 0.
The other bug is just that the "bulk update" query for the "users" was actually a copy of the query for the "user tracker" bulk update query. I just stripped the tracker info so that bulk updates of users aren't dependent on tracker aliases. You'll see from the patch.
Not a problem. To be honest, I'm only interested in getting 6.x-1.x-dev working well for my needs right now anyways. Once this patch is in place, as you see appropriate, I'll help you work through anything that may come up though.
In the end I just don't want to be on the outside of future Pathauto updates because of a feature I needed to add. As soon as everything seems fine, then I'll cook up that admin patch for people to try out. I really think it can be done in an unobtrusive manner.
Thanks again...
#11
Re-rolled the patch for current Pathauto-6.x-1.x-dev from 4-14-08...
#12
Re-rolled the patch for current Pathauto-6.x-1.x-dev from 5-07-08...
#13
Changing the title...
#14
Well here it is re-rolled against 6.x-2.x-dev. As I haven't patched it against pathauto in a while, I'll be kicking the tires this week.
As you'll see this patch does change and/or fix several things. I've listed below what I remember, but there are details in the previous posts here as well. As much as I'd like to dismantle it into seperate patches, there is some intertwining that's happend over time here to keep the keep the code size down. (I've been using it on a couple sites)
That said, feel free to catch me on IRC or here to pick apart anything you think is a bad idea, or code that sucks, etc.
#15
Hi, Moonshine. I can happily report that the Automatic Alias checkbox is staying turned off on subsequent edits of a node (and thus not overwriting the alias I handcrafted).
Thank you for this patch. Not sure I have occasion to test the other mods you made.
This is on 6.3, btw.
#16
+1! This patch is working beautifully for me. Thank you!