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. :)

CommentFileSizeAuthor
#30 pathauto-enhanced-update-actions-242456-30.patch6.25 KBundertext
#14 pathauto-enhanced-update-actions-v5.patch18.32 KBMoonshine
#12 pathauto-enhanced-update-actions-v4.patch17.43 KBMoonshine
#11 pathauto-enhanced-update-actions-v3.patch17.54 KBMoonshine
#10 pathauto-enhanced-update-actions-v2.patch18.56 KBMoonshine
#7 pathauto-enhanced-update-actions-v1.patch17.21 KBMoonshine

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Category: support » feature

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?

Moonshine’s picture

Moonshine commented

You're correct, I was proposing:

1. update action specific to users
2. update action specific to taxonomy
3. update action specific to node type

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!

Moonshine’s picture

Moonshine commented

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.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

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

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.

Moonshine’s picture

Moonshine commented

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...

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

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!).

Moonshine’s picture

Moonshine commented
StatusFileSize
new pathauto-enhanced-update-actions-v1.patch17.21 KB

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:

  1. I added a pretty lengthy writeup added to the README.txt file as requested. That should give a good overview of how the variables work. However it might be a little overpowering there. Really I'm still optimistic about getting this in the admin (yeah 80% I know), so maybe it's not a big deal.
  2. In making this patch I "fixed" another item that was getting in the way. Currently pathauto_update_action = 0 with node updates has an odd behaviour, as others have noted.

    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.

  3. I changed the weight on that checkbox field so that it's always above any existing path as the help descriptions state. For me it was above on node creation and below on node editing otherwise.
  4. I have added mode specific descriptive text for the different node editing senarios (now that they are more flexible), and changed references to fit in line with Drupals terminology better.
  5. Personal preference WARNING: I did put a little javascript change in here so that if you are requesting an Automatic URL path to be created, the entire manual path section is hidden. IMO it just seemed sort of dirty to only remove the description and then disable/grey the manual path field. I think this option is "cleaner" but of course YMMV. I figured you could at least take a look. If there is an actual reason for the current behavior I'd love to know though.

    6. 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

Moonshine’s picture

Moonshine commented
Status: Active » Needs review

Sorry, I just saw the issue for #2 above at:

#228762: Un-aliased nodes should still get an alias generated even if the update action is '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.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

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).

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.

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.

Moonshine’s picture

Moonshine commented
StatusFileSize
new pathauto-enhanced-update-actions-v2.patch18.56 KB

This seems reasonable. Can you just confirm what the behavior is if the user does not have "administer path aliases" permission? Thanks.

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:

  // 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.

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.

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...

Moonshine’s picture

Moonshine commented
StatusFileSize
new pathauto-enhanced-update-actions-v3.patch17.54 KB

Re-rolled the patch for current Pathauto-6.x-1.x-dev from 4-14-08...

Moonshine’s picture

Moonshine commented
StatusFileSize
new pathauto-enhanced-update-actions-v4.patch17.43 KB

Re-rolled the patch for current Pathauto-6.x-1.x-dev from 5-07-08...

Moonshine’s picture

Moonshine commented
Title: Questions re: updating aliases and what Pathauto considers a "content item" » Allow unique "Update Action" settings per item type (node, taxonomy,etc) or item (eg. page, story, vocabulary, etc)

Changing the title...

Moonshine’s picture

Moonshine commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Assigned: Unassigned » Moonshine
StatusFileSize
new pathauto-enhanced-update-actions-v5.patch18.32 KB

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.

  • Allows for more specific "update action" modes to be set via system variables. (ie. just for users, certain vocularies, specific node types, etc) Instructions patched into the readme.
  • Fixes bulk update actions for "update action" mode 0.
  • Fixes the use of the "Automatic Alias" checkbox on node forms such that it's set according to the current "update action" choice and not just ignored according to update action as currently.
  • Fixes case where a user reverts back to an existing alias for a node while in "update action" mode 1. (As discussed with greggles on IRC (this is @line 360 in pathauto.inc)
  • Added more descriptive help for the node forms based on if a path is currently set, and what the current update action is.
  • String update so that "Automatic Alias" checkbox reads "Automatic URL Alias" to better match Drupal's admin.
  • String update in pathauto.admin.inc as more then just "content items" are aliased by pathauto.
  • Changes the Javascript to provide a (IMO) cleaner interface, without items that are visble yet disabled. (Yeah this is a personal thing, but I want you guys to at least check it out ;). Be sure to clear your browser cache.)
aangel’s picture

aangel commented

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.

James Andres’s picture

James Andres commented

+1! This patch is working beautifully for me. Thank you!

emilyf’s picture

emilyf commented
Version: 7.x-1.x-dev » 6.x-1.x-dev

Any chance of getting this patch re-rolled for the current 6.x-1.3 version?

I tried patching the patch in #12 to the 6.x-1.3 version, the 6.x-1.1 version from 6/26/08, the latest 6.x-dev version. Everytime it failed. I desperately need to bulk delete by content type and this patch seems to be my only option. Any chance?

Status: Needs review » Needs work

The last submitted patch, pathauto-enhanced-update-actions-v5.patch, failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

This is a new feature so it would only be accepted for 7.x-1.x (and backported to 6.x-2.x).

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
emilyf’s picture

emilyf commented

I can't seem to find 6.x-2.x. I tried locating it, but it all redirects me to 7.x-1.x which is incompatible with 6. Is there anywhere I can get this?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

It's not listed on the project page but is available here: http://drupal.org/node/710692

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

I am looking for a way to have the auto-alias checkbox setting remain 'sticky' on a per node/node translation basis. Consider this use case...

Important notes:
- D6.16 is used. If any version of drupal prior to 6.16 is used, then you'll need to apply patch from post #77 in #358315: drupal_lookup_path() not respects alias' order.
- The patch from post #146 in #269877: path_set_alias() doesn't account for same alias in different languages is applied in order to allow nodes and their translations to have the same path. Language negotiation is being taken care of with prefixes + fallback. 'Path prefix only' won't work with this, since there is this core bug still standing and no D6 patch for it yet: #244162: Links not rewritten to include prefixes with 'Path prefix only' language negotiation

step1: a content type is set to sync its translations.
step2: a node of that content type is created in default language (usually english).
step3: a translation is added to that node, but the auto-create alias checkbox is unselected and path is set to be that same as original node.
step4: at a later point, the original node is edited -> its translations are synced -> paths for the translated nodes are updated.

Issue1: In step3 above, I would expect that once I hit the save button, the setting for the auto-alias would be remembered. It is not and nodes are being updated, which is why this issue exists I guess.

Issue2 (minor): Also it would be useful if in step 3, once the auto-alias checkbox is unchecked, it would default to the path of the original node. Same as body and title contents are + other settings of that node.

The patch in #14 states that it solves issue1 above, but I cannot apply it to latest 6.x-2.x-dev (no wonder, since more than one and a half years have passed since Moonshine rolled it). I've tried to update the patch myself without any luck (noob), but I've spotted some of the changes that were made in the meantime. Here is what I found:

- $src has been changed to $source
- in pathauto.inc the cases for pathauto_update_action have changed form 2 & 3 to PATHAUTO_UPDATE_ACTION_DELETE and PATHAUTO_UPDATE_ACTION_REDIRECT respectively. So, I guess the added 0 case should be changed to something matching as well (PATHAUTO_UPDATE_ACTION_NO_NEW perhaps?).

... anyone skilled and willing to re-roll this? I'll help test it.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

Ping? I have the impression that some sub-issues of this one are (to be) handled by patches in other issues. For example #358722: Node aliases lost/changed when using i18n synchronize translations

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

I know that according to #19 this is set to 7.x and once implemented then backported to 6.x, but the patch in #14 needs to be rerolled against latest 6.x-2x. So... @Moonshine: care to take this task James? I mean since you've done this in the past and all.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

Can we please have an update on this one? It was set to 7.x in #20, but no patch was created for that branch. I know for sure that it doesn't apply in 6.x-2.x anymore either.

From what I can tell part of the patch in #14 is handled elsewhere. These are two issues I am aware of:

#991986: Fix bulk generation queries
#358722: Node aliases lost/changed when using i18n synchronize translations

So what actually remains to be handled here and what was moved to other issues?

killua99’s picture

killua99 commented

So there no implementation for this option I mean ""Update Action" settings per item type (node, taxonomy,etc)" for pathauto 1.x (1.6 right now).

I was thinking that will be there another option in the "update action" something like "Setup different update action by content type." in the simple way to do this. And in each content type make a edit option to setup the tree options like. And leave a default options for the rest of types, "taxonomy, users, etc .."

  1. Do nothing. Leave the old alias intact.
  2. Create a new alias. Leave the existing alias functioning.
  3. Create a new alias. delete the old alias.
  4. Setup different update action by content type. For the rest of path, Create a new alias. Delete the old alias.

So some nodes I'm interesting to create once alias and don't update the alias. Another content type I'll leave with the default options "Create a new alias. Delete the old alias." may be.

this is another way to do this, and I see that some people are looking for this kind of option.

STINGER_LP’s picture

STINGER_LP commented
Version: 7.x-1.x-dev » 6.x-2.x-dev

So is this option going to be implemented? Because I'm also need to set this differently for nodes and taxonomy terms. The ability to set separately for nodes, taxonomy terms and users would be enough, but I think that to eliminate this issue ones and for all it's better to add ability to set this per content type/taxonomy term besides default separate settings for nodes, taxonomy terms and users in Pathauto settings.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
Version: 6.x-2.x-dev » 7.x-1.x-dev

...new features go first in latest branch (7.x) - then backported to previous branch(es) if still maintained.

undertext’s picture

undertext commented
Status: Needs work » Needs review
StatusFileSize
new pathauto-enhanced-update-actions-242456-30.patch6.25 KB

So... Here is the patch for 7.x-1.x version. It is mostly the reapplied Moonshine's patch without fixing side things.

jennypanighetti’s picture

jennypanighetti commented

Is this patch supposed to be applied to the dev code? Because when I ran this (patch from #30) on pathauto-7.x-1.2, I got:

patching file README.txt
Hunk #1 succeeded at 62 with fuzz 2.
patching file pathauto.inc
Hunk #1 succeeded at 94 (offset 5 lines).
Hunk #2 FAILED at 401.
Hunk #3 succeeded at 443 (offset -20 lines).
Hunk #4 FAILED at 526.
Hunk #5 succeeded at 558 with fuzz 1 (offset -13 lines).
2 out of 5 hunks FAILED -- saving rejects to file pathauto.inc.rej

Nick Lewis’s picture

Nick Lewis commented

Confirmed that #30 resolves the issue using 7.x-1.x-dev/

aangel’s picture

aangel commented
Status: Needs review » Closed (outdated)