I had provided a patch in #180440: If an alias is manually created, don't automatically replace it on edit that seemed to get lost in the discussion and the issue has been closed to comments without review of the patch. I'm posting this separately so this can solely focus on the patch at hand. Have been testing this locally and I can write a SimpleTest test case also if it is wanted.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Basically this patch compares the current node's alias path to the one that would be generated by pathauto. If they are equal (or it's a new node and doesn't have a path), the 'automatic alias' checkbox is checked by default.

Dave Reid’s picture

Quick fix in the case that the node has been created and the user edits the path textfield to remove the alias, then the checkbox should not be checked.

Dave Reid’s picture

Removed the cruft from the path_redirect API integration, sorry.

Dave Reid’s picture

Removes the check for user_access('create url aliases') in pathauto_form_alter() since path.module already implements '#access' => user_access('create url aliases') on the $form['path'] fieldset.

Dave Reid’s picture

I revised the logic used and found a couple errors. I improved the documentation in the patch and I'm confident that this is good. I have tested the following conditions:

1. New new node - pathauto checkbox is checked (expected)
2. New new node, pathauto checkbox unchecked and clicked preview - pathauto checkbox is not checked (expected)
3. Existing node with pathauto alias - pathauto checkbox is checked (expected)
4. Existing node with the pathauto checkbox unchecked and a custom alias, node saved - pathauto checkbox is not checked (expected)
5. Existing node with the pathauto checkbox unchecked and the path alias field empty, node saved - pathauto checkbox is not checked (expected)

Also, I'm not sure even why isset($form['path']['path']) needs to be checked. It should always be there because of the reason posted in #4, so I removed that.

Dave Reid’s picture

Even more cleanups possible now that $node->pathauto_perform_alias is always set.

greggles’s picture

I think this works in most situations, but not for people who use the current date as part of the url, right?

(btw, this is what I was trying to say in http://drupal.org/node/180440#comment-1237790 ).

Dave Reid’s picture

On intial thought, I don't see why it wouldn't work that way. I'll give it a try and report back.

Dave Reid’s picture

I just tried it with a node pattern of "[yyyy]/[mm]/[date]" and it worked exactly as expected. When I had entered my own custom alias or had the path field empty, the automatic alias checkbox was left unchecked on the next time the edit node page was loaded.

Dave Reid’s picture

greggles or anyone else, have you had a chance to test and review?

greggles’s picture

Here is the kind of pattern I'm concerned about and the bug it causes:

1. Use a pattern like page/[user-name]/[site-date-day]/[title-raw] and let Pathauto create the alias
2. Create a node as uid2 on a monday and get a url like page/greg/monday/some-content
3. Edit that node on Tuesday as uid1 and the Automatic alias checkbox will automatically be changed to unchecked

Does this patch work for 90% of the people who use Pathauto? Yes. If we commit this will we ever get the right fix? Probably not. I'm torn about what to do.

Dave Reid’s picture

Hmm...I'd argue that use case is very extreme and doesn't work with the patch. I would maybe see page/[author-name]/[site-date-day]/[title-raw] as something more normal, and that path in fact would work with the patch.

greggles’s picture

@DaveReid #12 - Yeah. I agree :p

Does this patch work for 90% of the people who use Pathauto? Yes. If we commit this will we ever get the right fix? Probably not. I'm torn about what to do.

Let's wait a little bit for Freso to provide his input.

akahn’s picture

What if this was a setting, something like "Preserve custom URL alias when editing node"? This could be on by default, and do what this patch does, but users for whom this would mess up their workflow could turn it off and have aliases always update when they edit a node.

greggles’s picture

What if this was a setting

Any proposal that starts with that is a dead-end for Pathauto unless the proposal also removes options somewhere else.

We need fewer settings, not more.

akahn’s picture

Ok. I agree that simplicity and fewer settings are good ideas. I was just trying to think of a way to help get this feature into Pathauto.

japanitrat’s picture

Thats a really good idea. It's really a pain in the ass to always check-off path-auto when editing nodes that are node meant to have a manual path. Say you have a frontpage with a manual path, the whole site is unusable if you forget the checkbox some day. Definitely a must-have fix.

Subscribe.

agentrickard’s picture

Patch does work as advertised. I wonder if there is a way to identify the tokens that are not safe to use with this patch (things like [site-date-day]) and keep the box checked if they are present. But that would likely require an API change in order to register those tokens.

Another option is to set a form_error() on this field if a conflict is present between the user-supplied alias and the automatic alias. That would force the editor to correct the issue before saving.

Manuel Garcia’s picture

I'm probably way out of my league here, but hear me out please :

Would it be possible to save the value of the checkbox in the node itself? This way there wouldn't be a need for fancy checks or anything, then again, I am not a pro developer, nor do I know the inside-out of pathauto... The only thing that would be needed is to check for this value on node save, which it's already being done (right?)

Has this already been considered and discarded, or could this be a way to do it?

I'm sorry if this implies performance issues or anything like that, again, not very hardcore developer...

Dave Reid’s picture

@Manuel It's one approach, but its a lot of work/data just for one little checkbox. A new table, two columns, that could have just as many records as the {node} table, when we could just compare the current node alias to the one that would be generated via pathauto. This works in 99%, citation needed :) of situations.

Manuel Garcia’s picture

OK, thanks for explaining :)

Freso’s picture

I'm still somewhat uneasy about this patch, I must admit. 90% (or even 99%) are still not 100%. I'd prefer if we do something, we do it completely right (even if I do get the argument about not needing to rely on the database). This just seems too hackish to me, I'm afraid. Feel free to try and convince me otherwise.

keff’s picture

Thank you for the patch, Dave, I thought I would have to do this myself, as it is confusing to the authors of my site (I would otherwise have to explain that they always need to check the pathauto checkbox when editing old node). We don't use dates in url, though...

mrfelton’s picture

The patch at #6 has been working for me for a couple of weeks now with no problems (I'm not using dates in the URL either). Thank you, this makes the life of my content team so much less painful. It's the little things that matter!

HXn’s picture

Version: 6.x-1.x-dev » 6.x-1.1

Thanks Dave, works great with Pathauto 6.x-1.1.

binhcan’s picture

I've been working day and night on this Chinese Furniture website and I faced this problem since the start up. I've been looking for a solution, till now when I finally got your patch. Thank you so much!!!

BTW< it took me few hours just to learn how to apply the patch, so here i upload the working files for everyone to replace their current one, no need patch.

greggles’s picture

@binhcan - please don't upload entire zips of the module... Also, dropping links to your site here is somewhat inappropriate (note also that drupal.org uses nofollow so you get no page rank benefit).

mikeker’s picture

@greggles #15 -- I understand that you want to keep settings to a minimum, but this might be one worth adding. If not, what about adding an 'edit pathauto aliases' permission that can be given/revoked to roles that should/should not be changing the path. Using appropriate default values for this permission would cover the 1 - 5% (citation needed... <smirk>) that would get tripped up by Dave's patch. Yet the rest of us can unset the permission for various roles and not have to worry about changing paths every time we edit a node.

Right now, I've got a situation where a writer has no path or pathauto permissions, but they still change the pathauto-generated path when they edit a node. I can't even tell them to clear the Automatic Alias checkbox since they don't see it.

Thoughts?

Anonymous’s picture

Just wanted to say thanks Dave for the patch. The default behaviour was always making me nervous when editing pages but this works great. FWIW am not using dates.

moonray’s picture

subscribing.

Freso’s picture

Version: 6.x-1.1 » 7.x-1.x-dev
Status: Needs review » Needs work

The patch doesn't apply to 6.x-2.x.

Jennifer_M’s picture

Not actually a comment on updating for 6.x.2.x - just wanted to say a big THANK YOU for the patch, which will save me a lot of clicking! Very much appreciated, & seems to be working fine for me.

AdrianB’s picture

subscribing

Btw, on the other issue that "has been discussed completely" and is closed "because there is nothing more to discuss" (although this issue seems to indicate otherwise) the only way to give feedback is to chip in, but the chip in has expired. Is there a new secret chip in or has that idea been abandoned?

Dave Reid’s picture

Re-rolled for 6.x-1.x. Do we need this rerolled for 6.x-2.x as well?

Freso’s picture

Yep. It won't go into 6.x-1.x, if it hasn't gone in to 6.x-2.x. :) I try to follow core's release principles as much as possible, as I find them to be very sound. But that also means that the latest version has to have the fix, before it can be backported to previous versions. (I know you know this, Dave, but other people reading along might not.)

WesleyTx’s picture

subscribing

z33k3r’s picture

When can we see an official release including the required changes for this patch?

betamos’s picture

I've been looking for something like this a long time now, and it's nice to see ppl are doing something about it. Before I saw this thread, I was thinking: how about adding an extra checkbox, e.g. called "Persistent alias", which is stored in the database. However, the solution provided here is more logical to the user.

The logic in a nutshell is: "If you uncheck the checkbox, it should stay unchecked" which, of course, kicks ass!

I haven't reviewed the code yet but it seems like the comparison method is a little unstable. What if someone changes the automated url-alias setting? Or if someone has the date in the url-alias and the published date is changed? Say you get around those problems, how can you be sure that in the future it will still be comparable, even if the logic for automated aliases get more advanced? I think it's got to be a 100% waterproof solution before you implement it in pathauto, and unfortunaltely I don't see how to do that without altering the database structure.

Maybe another column would be needed after all?

sime’s picture

Patch at #34 works for me. Is this already in the 1.x dev version?

Nelson Wu’s picture

Trying to detect custom aliases is too hackish and unreliable for my liking.

I'd prefer the method of storing it in the database - you would only need to store one record per custom alias. I fail to see the bloat and it would work 100% of the time, instead of working for most people in most situations.

Either way, I'm looking forward to an official fix!

edit: To clarify, I was thinking of a table with a single column to record the nodes with custom aliases. The interface from the user's point of view would be exactly the same but the state of the existing checkbox would be saved. In this example, two nodes have custom alias set and the others are auto-generated.

node id
1
3
apemantus’s picture

This issue is pretty key for me at the moment, as I'm editting (and reeditting) a multilingual English/Chinese site (where the Chinese URLs are the same as the Path Auto generated English ones) and I'm continually forgetting to untick the box when I edit a Chinese page.

Ultimately, I guess adding another field to url_alias would be the neater solution, but whatever happens, I see this as a pretty useful fix for me either way.

z33k3r’s picture

+1 to Whom ever was the first to mention that the easiest and cleanest way to fix all of this was to add another column in the alias table... so simple. Why are we trying to hack our way into a solution when a column is so much more reasonable?

Dave Reid’s picture

We can't just add another column to the alias table because 1. it doesn't make sense, and 2. it is a node relationship and not a alias relationship. Think about it:

node id | using automatic alias
1       | TRUE
2       | FALSE
3       | TRUE
z33k3r’s picture

Right, so my apologies about making it sound that it would go in the alias table, wouldn't it be more simple to just create a relationship table though for the aliases?

greggles’s picture

If someone rolls a patch for all three branches, this solution will be committed. This issue has been discussed to death. Further discussion is not helpful at this point.

aangel’s picture

When I leave the checkbox set I get:
user warning: Duplicate entry '/feed-' for key 2 query: INSERT INTO url_alias (src, dst, language) VALUES ('node/352/feed', '/feed', '') in /home/u5/jbwebadmin/staging/html/modules/path/path.module on line 112.

with 1.x-dev (June 15) using the patch from #34.

Freso’s picture

Since it works for so many people, I'm fine with this approach. If all hell breaks loose (for some), we can do the DB stuff for 6.x-2.x. This patch still needs to go the 6.x-2.x -> 6.x-1.x -> 5.x-2.x route though, so we're sure all supported versions are patched. (Hence why I requested a 6.x-2.x version a wee while back - I intended to commit this behind Greg's back to surprise him, but since a 6.x-2.x version was never rolled... I didn't get to do this. Oh well.)

Poieo’s picture

Subscribing

doniking’s picture

Subscribing

z33k3r’s picture

Any luck on creating an official update to include the pre-mentioned patch?

Dave Reid’s picture

Assigned: Unassigned » Dave Reid

Today is my recovery day from DrupalCampColorado, but I will be rolling this again tomorrow for 6.x-2.x and 6.x-1.x.

mr.j’s picture

I just want pathauto to create the path the first time a node is saved, and never update it unless an editor specifically asks it to. So I wrote a simple fix for a 5.x system that deselects the checkbox whenever an editor edits a node, and the path settings field group is collapsed to keep things tidy.

It doesn't handle users who don't have permission to edit the url alias (pathauto just goes ahead an auto-generates anyway), but for us this is confined to regular users changing forum topic titles which is both infrequent and not a deal breaker.

I post the code below in case anybody wants to use it. You must add it to a custom module and ensure the module has a weight setting that is greater than pathauto's weight (in system table).

function yourmodule_form_alter($formid, &$form) {
  if (isset($form['#id']) && ($form['#id'] == 'node-form') && arg(0) == 'node' && arg(2) == 'edit') {
    if (isset($form['path']['pathauto_perform_alias'])) {
      $form['path']['pathauto_perform_alias']['#default_value'] = FALSE;
      $form['path']['#collapsed'] = TRUE;
    }
  }
}
Jeff Burnz’s picture

Subscribe, really looking forward to this, would fix a long term pita:)

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
5.89 KB

Patch re-rolled for Pathauto HEAD (6.x-2.x)! Tested and works as expected.

Dave Reid’s picture

An alternate more future-looking idea for this would to be:
1. Implement pathauto_schema_alter() to add a 'pathauto' tiny-int column to the {node} table (0 = no automatic alias, 1 = automatic alias). Or add a {pathauto_node} table, but using the schema alter is more fun and seems more appriopriate.
2. Now the automatic alias checkbox value can be automatically loaded in node_load since it's a part of the node table. We can just check $node->pathauto.

aangel’s picture

Thanks for that snippet mr.j. I've modified it slightly for 6.x and it's working well for me:

 /**
  *  Uncheck the update alias checkbox automatically
  */
function mymodule_form_alter(&$form, $form_state, $formid) {
  if (isset($form['#id']) && ($form['#id'] == 'node-form') && arg(0) == 'node' && arg(2) == 'edit') {
    if (isset($form['path']['pathauto_perform_alias'])) {
      $form['path']['pathauto_perform_alias']['#default_value'] = FALSE;
      $form['path']['#collapsed'] = TRUE;
    }
  }
}
svogel’s picture

Subscribe

darktygur-1’s picture

Is there any particular reason why the checkbox must be checked by default when editing a node? I really like the idea in comment #52 (and updated in #56).

By the way, to get that working for myself, I had to make it set the #value member of the array, instead of #default_value (actually, i just made it do both, to be safe). Also, in my case, I had to use a #pre_render callback because pathauto was kicking in after my module.

Freso’s picture

@ darktygur: Because people who install Pathauto are likely to want to have the aliases automatically managed. Thus, this is the default value - easily overruled by a simple FormAPI call, as you yourself note.

Also, the second issue is a module weight one. Google is your friend.

mikemccaffrey’s picture

+1 For the suggestion of storing the value of the checkbox from the node edit forms.

Yes, it would require a schema change, but it seems like a relatively simple and common sense solution.

The proposed patches that involve complex autodetection would make me more nervous, rather than less, about enabling automatic path updating.

Seems like something that should be backported to the 5.x version as well.

darktygur-1’s picture

@Freso
I knew that was a module weight issue. But I wanted my override to Just Work, without being dependent on Drupal settings. Forcing my way up the pipeline (using #pre_render) allowed me to bypass the whole module weight thing, which was more attractive to me (seemed less brittle). I mostly mentioned that approach here in case someone else would've preferred the approach I took over the more obvious module weight adjustment.

I'd also like to see the value get stored in the database. It does seem to be the more natural way to handle this. Is there anything wrong with Dave Reid's idea in comment #55 to get around the creation of another table?

R.Hendel’s picture

Subscribing

askibinski’s picture

FYI:
It looks like the SEO friend module is also addressing this issue,
http://drupal.org/node/528836

Dave Reid’s picture

Marked #532326: Pathauto Overwrites User's Custom Alias as a duplicate of this issue.

j0nathan’s picture

subscribing...

autopoietic’s picture

Thankyou Dave, I have patched and tested ok.

mr.j’s picture

The "save the checkbox state" approach is mainly good, but I can foresee a couple of problems:

1. Users without pathauto permissions will never see the checkbox so it will autogenerate every time they save a node regardless. As I said in #52, a typical use case might be users editing a forum topic title where you use the title in your path.

2. I'd prefer it on the first time the node is saved, but don't want the path updated after that unless I request it. So it means that after creating a node with auto alias switched on, I'd have to edit and save again with it switched off.

The problem with the existing "Do nothing when editing" global option is that (correct me if I'm wrong here), you can't override it on a node by node basis as it just ignores the node's autogenerate checkbox.

Now if it could be overridden you could do the following:

When adding a node, the pathauto checkbox is on by default. However, do not save the checkbox state to the database when saving the brand new node. On first edit of the node use the global regenerate setting as a default for the pathauto checkbox, then store the individual node setting when saving the edit to the database.

I think it handles all cases, and the most common ones require little or no user intervention:

You want it to autogenerate for new nodes and edited nodes (i.e. like now):
- Box is on by default when adding node, and global flag to regenerate is on by default (i.e. do nothing).

You want it to autogenerate for new nodes but never again:
- Box is on by default when adding node, and you set the global flag to not regenerate on edits.

You do not want it to autogenerate for new nodes and edited nodes:
- Switch box off manually when adding node, and set the global flag to not regenerate on edits.

You do not want it to autogenerate for new nodes but you do after editing (strange but can be handled):
- Switch box off manually when adding, and global flag to regenerate is on by default.

alison’s picture

(1) subscribing

(2) sorry, I just read through this whole thread and managed to really confuse myself...

(re: drupal 6 version) --> is it or is it not the case that the latest dev version includes the latest (tested, successful) patch?

Thanks, and sorry for the dumb question.

Les Lim’s picture

@alisonjo2786: Nope. No patch from this issue has been committed to CVS yet. You can always browse through the latest CVS messages if you're not sure.

AdrianB’s picture

I like the approach presented by mr.j in #67. That would solve my issues with this problem.

(I also like how WordPress automagically handle this in a superb way:
1) An URL is autogenerated and regenerated as long as the post is not published.
2) If you don't like the autogenerated URL, then you're free to manually change it.
3) Once published it is never changed unless you manually do it.
4) If you do change it manually, then the old path is saved and redirected to the new path.
This has worked flawless for me in WordPress and I think it's the way 99,99 percent of the users want it to work.)

J V’s picture

Edit: No more threaded, this was meant as a reply to #70

This is a good idea, and would solve the problems, however this is already a part of pathauto (Going to settings and choosing "Update action: Do nothing. Leave the old alias intact.") the problem with this is that there is a bug that makes terms unaccessable if you delete and bulk update the aliases.

Quite a catch 22 as the fix is in the dev version and greggles won't release it until this bug has been fixed (sorry I really had to laugh at that)

Anyway mr.j makes a good point, if something that effects the auto-generated url is changed then it will always consider the old url a custom made one.

2. I'd prefer it on the first time the node is saved, but don't want the path updated after that unless I request it. So it means that after creating a node with auto alias switched on, I'd have to edit and save again with it switched off.

As I said before, that's pretty much what happens if you change settings.

The only problem is this "Global flag" - this is basicly an override of the settings on a per node basis (If I'm not mistaken)

Shouldn't this be easily done by adding an extra column to the table and checking the value whenever its been edited? (Sorry my servers phpmyadmin is down I can't see if there even is a pathauto table but I think I remember seeing one)

okeedoak’s picture

+1 For storing the value of the checkbox in the database.

okeedoak’s picture

On clean site applied patch from #34 to dev version.

Error on edit form:
Fatal error: Call to undefined function pathauto_get_placeholders() in .../sites/default/modules/pathauto/pathauto.module on line 294

Removed patched version, reverted to version 1.1, no errors.

alison’s picture

Thank you! And thanks for the tip for the future.

greggles’s picture

@gettingpissed - there is no pathauto table and I don't want to create one.

For the folks who want to put it in an extra column and store per node, please read all the comments on this issue and especially those around #20. We're not going to do that.

@okeedoak - try the 6.x-1.x-dev version?

okeedoak’s picture

@greggles Tried the 6.x-1.x-dev version. No problems. Repatched same dev version, same problem.

Suggestions?

Dave Reid’s picture

Patch in #54 is for 6.x-2.x-dev, not 6.x-1.x-dev. #34 may still work for 6.x-1.x, but I'm not sure. Either way, the patch will be backported when it is accepted for 6.x-2.x.

okeedoak’s picture

I was using the patch from #34 against 6.x-1.x and had the error. Would be happy to check 6.x-2.x-dev patched with #54 later tonight.

Is 6.x-2.x-dev suitable for production?

greggles’s picture

6.x-2.x is not suitable for production. It works ok, but may change drastically.

okeedoak’s picture

Patch in #54 works for 6.x-2.x-dev where the patch in #34 against 6.x-1.x didn't. This wasn't a comprehensive test but there was no error and the checkbox remained unchecked after manually editing the URL: the expected behavior.

alison’s picture

Yay for #34! Perfect!

okeedoak’s picture

Wonder why #34 worked for you and threw an error for me?

alison’s picture

Well, if there's a chance you made the same silly/simple mistakes I did, check your version of Pathauto, and check the version of the patch you tried to apply. I definitely tried to apply the wrong patch to the wrong version (more than once, believe it or not).

What sort of error -- when you tried to apply the patch, or on your website as an error message, or...?

okeedoak’s picture

The patch from #34 applied with no errors. The error was when I went to edit a node of a type pathauto was set to control the url of. See #73 for error.

Les Lim’s picture

#84: that would indicate that _pathauto_include() needs to get called in pathauto_form_alter() before pathauto_get_placeholders does. I'd be hesitant to roll a new patch for 6.x-1.x, though, since the effort needs to focus on testing 6.x-2.x.

The patch in #54 for 2.x doesn't need this revision, BTW.

Les Lim’s picture

The patch in #54 works as advertised for me, though at the moment, I've only tried it with a menupath-raw pattern. Not the most comprehensive of reviews.

Dave Reid, greggles or Freso, could one of you weigh in on what kinds of patterns or settings combinations should be given particular scrutiny while reviewing this patch?

greggles’s picture

Installed and reviewed tonight.

-      if (empty($pattern)) {
+      if (!$pattern) {

Is there a particular reason to do this? I find the empty to be more legible and, of course, robust against E_ALL notices (they may not be possible now, but using empty makes it more resilient against changes in the future).

This seems to work but it also changes a lot of the logic in the form_alter which was a tricky area to get ironed out and took a lot of debugging from people with various combinations of roles/permissions.

After clearing up the empty question I think the best approach is to commit this and ask for reviews of the dev branches. Hopefully we can get at least that.

gpk’s picture

Very happy to text a fixed patch for 6.x-1.x (see #85), or 6.x-1.x-dev itself when patch is committed. Subscribing. (Currently I have a custom module that does something equivalent/similar and would be very pleased to lose that in favor of this enhancement.)

gpk’s picture

@87:
>changes a lot of the logic in the form_alter
Re. the change from:

    // If there is a pattern AND the user is allowed to create aliases AND the path textbox is present on this form
    if (!empty($pattern) && user_access('create url aliases') && isset($form['path']['path'])) {

to just

    // If there is a pattern, show the automatic alias checkbox.
    if ($pattern) {

see #4 and #5 where Dave notes that the visibility of the Path fieldset is in any case controlled by $form['path']['#access'], the difference being that now $form['path']['pathauto_perform_alias'] and $node->pathauto_perform_alias will always be set, even when these form elements are not visible. Possibly this means that everything continues to behave even when the current user can't see the path fieldset. (Also means that there is possibly redundant code in pathauto_nodeapi(), which currently tests for !isset($node->pathauto_perform_alias) amongst other things, but maybe prudent to keep that in. Possibly the comments there could do with an update though, and also the comment "show the automatic alias checkbox" which should perhaps read something like "add the automatic alias checkbox." since it will only be shown if you have 'create url aliases' permission.) You were probably already up to speed on all of that, hope I've not confused the issue.

gpk’s picture

Suggested tests (based on #5, some reworded since they were ambiguous and some added):
1. New node - pathauto checkbox is checked (expected), save node, alias correctly generated (expected)
2. New node, uncheck pathauto checkbox and click preview - pathauto checkbox is not checked (expected), save node, no alias generated (expected)
3. New node, uncheck pathauto checkbox and enter custom alias, save node - custom alias created (expected)
4. New node, uncheck pathauto checkbox, enter custom alias, recheck pathauto checkbox, auto-alias generated (expected)
5. Existing node with pathauto alias - pathauto checkbox is checked (expected)
6. Existing node with a custom alias, pathauto checkbox is unchecked (expected), save node, - edit again and pathauto checkbox is still not checked (expected)
7. Existing node with the path alias field empty - pathauto checkbox not checked (expected), save node again - edit again and pathauto checkbox is still not checked (expected)
8. Existing node with a custom alias, [depending on automatic alias definition] modify the node so that the auto alias would match, save node, edit again - pathauto checkbox is checked (expected)
9. Existing node with pathauto alias, uncheck and enter custom alias, preview - pathauto box not checked (expected), recheck the box, preview - pathauto box remains checked (expected)
10. Existing node with pathauto alias, modify the node so that the alias changes, save node - auto alias is correctly modified (expected)

Repeat for different options selected for pathauto_update_action dropdown on admin screen (some behaviors will change e.g. not updating existing aliases).

Repeat for user without 'create url aliases' permission - the checkbox won't be visible on the form but it is present in the $form data structure and should behave in the same way when the node is saved (test 2 not relevant).

alison’s picture

Here's something interesting I noticed (at least, I didn't expect this) -- when I edit existing nodes whose pathauto aliases were generated by obsolete Automated alias settings, I'm finding the Automatic alias checkbox unchecked. Just to be extra clear, these nodes' aliases are pathauto aliases. But, these nodes are old and I haven't gone in to edit them in quite a while, and since they were created (since their pathauto aliases were created), the site-wide, General settings have changed. Specifically, I've changed the settings related to non-ASCII-96 characters (I've since checked the "Reduce strings to letters and numbers..." and "Transliterate prior to creating..." checkboxes). It's as if Pathauto is protecting, or maybe just alerting me to the fact that if the Automatic alias box is checked, the node's alias will change. Personally, I find this to be great, but regardless, just wanted to put it out there so y'all were aware if you weren't already.

Also, my site is multilingual (English/Spanish -- I'm using i18n 6.x-1.1), but I've finally been able to confirm that what I've described occurs for English and Spanish nodes (at first I couldn't find any fitting English examples, but I just found one).

(Notes: I'm using Pathauto 6.x-1.1, I've applied the patch from #34, I have Path redirect (6.x-1.0-beta4) installed, though I only enable its features in specific situations, so its features are not currently active; and I have Token 6.x-1.x-dev -- I know, it's old, long story...)

alison’s picture

@90:

**With Pathauto 6.x-1.1, with #34 patch applied, on a multilingual (English/Spanish) website (testing examples for both languages).

1. Worked as expected, worked as expected
2. WAE, WAE
3. WAE
4. WAE
5. WAE (aside from what I described in my comment)
6. WAE
7. WAE, WAE
8. WAE
9. WAE all the way through.
10. WAE

(Yay!)

alison’s picture

@91 (me, earlier):

While I was doing the tests from #90, I was thinking that what I described above (#91) seems like it's Pathauto realizing/indicating that those nodes' aliases are in fact custom aliases, really, even though they used to be Pathauto aliases. Maybe this isn't news to anyone else, and if that's the case, never mind, but... this definitely didn't happen before I applied the patch in #34, so it seems like a feature to me. Maybe a feature worth adding to the list of tests to try out? (Unless, obviously, I'm the only one to whom this is news.)

greggles’s picture

#91 seems like a bug to me, but hopefully a small one. Maybe we should add some documentation that says "if you change your patterns pathauto will get confoozed. sorry."

mxmilkiib’s picture

subscribe

alison’s picture

@94:

Certainly if it's not intended it could be a bug/a sign of confusion -- but, if it's simply an unintended but logical effect, that could be okay, as long as it's clarified in the docs. I, at least, think it's (a) rational, and (b) good/useful, because:

  • The box will be automatically checked if you change a custom alias to something that matches what the automated/Pathauto alias would be. The bug/effect from #91, as I attempted to explain in #93, is the same sort of thing (at least, it seems like the same sort of thing to non-programmer-me).
  • I appreciated that I was subtly "alerted" of the fact that the alias of the node did not match the Automated alias settings (anymore). Otherwise, I may not have realized that the URL alias was quietly being changed/updated. Are there other ways to deal with this issue? Certainly -- but, I liked this effect, and if it's determined not to be a "buggy" effect, and if it's clearly documented, I'd vote for keeping it! :)
SpikeX’s picture

I just applied the patch in #34 (I'm using Pathauto 1.1 for D6), and nothing happened.

Someone want to point me in the right direction for patching my Pathauto 1.1 on D6 to fix this issue? It's been bugging me for a long time now.

Jax’s picture

subscribe

featherbelly’s picture

Subscribing

HnLn’s picture

subscribe

alison’s picture

Were you able to get this working? If not, could you tell us what you did, and maybe we can tell you what to do to get it working?

John Pitcairn’s picture

Re #55 - I understand the maintainers' wish to not alter the node table, or to add a pathauto_node table, but yes, something like this seems to me to be a non-hacky and more site-developer-friendly solution. Auto-alias-generation should be a node property, and manually unchecking that checkbox an explicitly remembered action. Subscribing.

gpk’s picture

@91-96: this behavior (i.e. you change the pattern and then when you edit affected objects the "automatic alias" checkbox is unchecked) is what I would expect (but note I am coming at this question from the perspective of someone who has partly written a custom module to do the same as the patches here, before I found this thread). Whether it is desirable .. well that's harder. Pathauto doesn't comment on what happens if you change your pattern, and doesn't AFAIK provide an option to "update" your aliases to the new patterns (though you can bulk generate patterns for all unaliased objects of any or each type). If you wanted to do that you would have to edit/save all existing objects of the relevant type.

On a long-standing site you might not want existing aliases to be updated when you change a pattern. So the current patch would fit that model well, whereas #102 (as at present) would ensure that the relevant objects get new paths when the object is re-saved. OK you can get round changes in aliases by using the options to leave the old alias intact, or to have a path redirect from old to new. But those options are less helpful when creating new content, when you might e.g. edit the page title a few times to get it how you want. If you are basing the alias on the title do you then want multiple aliases/redirects to be set up? Or maybe it doesn't really matter?

As someone for whom websites/Drupal is a part-time activity I sometimes wonder what I am trying to achieve with URL aliases. I'm not sure how best to use great modules like pathauto and path redirect, or how much I should care about this! Does it really matter? Sometimes it seems it would be easiest just to go back to node/xxx, which would certainly avoid problems with broken links!!

tim.plunkett’s picture

subscribing

easp’s picture

subscribing

jeffleeismyhero’s picture

subscribing

John Pitcairn’s picture

Re #103: But those options are less helpful when creating new content, when you might e.g. edit the page title a few times to get it how you want.

Yeah, this is the sort of behaviour I'm anticipating from content creators/editors in a revisioning workflow. In that case I'd probably want something like: Generate an alias for the current published revision of the title. Optionally keep, delete, or (most likely) 301 redirect, older aliases. Ideally, don't generate new aliases for unpublished revisions.

So far so good, it's mostly do-able. The node's pathauto checkbox has effectively been checked throughout the revisioning process, because I don't want editors to have to think about this - they won't even see the checkbox and path alias field. And I don't want it to become unchecked if admin changes the pathauto pattern, I want a new alias generated (this is where the patch really doesn't help me).

But I do want admins to be able to manually uncheck that box and have the node permanently associated with one specific URL, ie all the internal link paths remain consistent even if the editors are monkeying with the node title. If editors want the path changed they'll have to escalate to admin. And I don't want that alias removed or altered by a bulk-regenerate for a new pattern if that checkbox is unchecked.

So I don't think I can do what I want at present, with or without this patch. I'd definitely need that checkbox state associated with the node?

Any suggestions are very welcome.

sjhobson’s picture

subscribing

cerup’s picture

subscribing.

This is a necessity.

alison’s picture

@92 (me)

Following up...

In case anyone's curious, I was trying some other things out, unrelated to this, and discovered that the patch (#34) also works in the following situation (now that I think of it, I'm not sure we haven't already covered this... but just in case...):

- create content item
- convert content item to different content type, which happens to have a different Automatic Alias settings (converted using Node Convert module)
- edit content item --> checkbox is unchecked (because... the auto-alias settings, if applied, would generate an alias different than the current/existing alias!)
(- save content item without checking box, no path alias change)

just felt the need to share :) please forgive any unwarranted enthusiasm.

grendzy’s picture

subscribe

mattyoung’s picture

.

grendzy’s picture

Status: Needs work » Needs review

I've tested #54, and it seems to work for basic patterns like [title-raw]. However it doesn't work for any patterns that rely on data that changes external to the node form (e.g., author name). Essentially, this problem is impossible to solve correctly without persistent, per-node storage of the auto/manual setting. The comparison method doesn't work because when there's a mismatch, there's no way to tell if the alias was manually changed, or if the underlying data changed.

It also doesn't preserve manual aliases on /admin/content/node -> Update path alias.

Here's the core tokens that won't work with this comparison method. Note that it's broken for about 60% of the available tokens. Granted, not all these tokens are widely used. Still, #54 is not a "99%" solution.

[type-name]
[author-name]
[author-name-raw]
[author-mail]
[author-mail-raw]
[term]
[term-raw]
[vocab]
[vocab-raw]
[mod-yyyy]
[mod-yy]
[mod-month]
[mod-mon]
[mod-mm]
[mod-m]
[mod-ww]
[mod-date]
[mod-day]
[mod-ddd]
[mod-dd]
[mod-d]
[menu]
[menu-raw]
[menupath]
[menupath-raw]
[menu-link-title]
[menu-link-title-raw]
[termpath]
[termpath-raw]
[termalias]

In #180440: If an alias is manually created, don't automatically replace it on edit, greggles wrote

In terms of performance - I built a test site with ~8,000 nodes and didn't see any statistically significant differences in performance. This patch gets involved on object insert, bulk generate, and node edit. So, those pages are already things that people are used to having as slow operations and they are already scalable. I don't see this as a huge deal.

So while I realize that adding a new table is distasteful to some, it appears to be the only correct solution.

grendzy’s picture

Status: Needs review » Needs work
Dave Reid’s picture

Status: Needs review » Needs work

@grendzy: The other viewpoint is that if the URL alias is going to have a major change like could happen with those tokens, then having that URL alias field unchecked so you can review the change and approve it.

I'll be rolling a patch tomorrow that implements a database column solution.

greggles’s picture

@Dave - no need, I don't like that solution. I just need some more round tuits to finish reviewing this and commit it, but it will happen within this week. I'd rather you save your effort for the 6.x-1.x backport.

The list from #113 seems overly inclusive (how do type-name or author-name fail with this method?) and is not representative of true use - 60% of nominal tokens != 60% of the most commonly used tokens for path alliases.

grendzy’s picture

how do type-name or author-name fail with this method?

  • Create node
  • Change author name (Not user id, but change author name on /user/NN/edit)
  • Edit node — path will fail to update

[menupath-raw] is probably the best example of a token that's both useful, and volatile. In other words, it's pretty easy to imagine someone wanting to use this token, then moving a menu item, and expecting the path to update when the node is saved.

I think greggles was right in #11; if we commit this we'll never get the right fix.

Dave Reid: I see your point about some users wanting to review certain changes. On the whole though, I think the comparison method is more likely to confuse users. Pathauto will be guessing the state of the checkbox, and sometimes will guess wrong. Compare
"Sometimes my pathauto nodes get unchecked, and I don't understand why"
vs
"I know a node set on auto will always be on autopilot, and will get the most current path every time I update it. I also know that paths I set manually will stay that way".

jrabeemer’s picture

+1 Eagerly awaiting the patch for 6.x-1.x port!!

xjm’s picture

Tracking the 6.x patch too.

DamienMcKenna’s picture

Could someone please explain:

  • If any of the proposed patches are under consideration,
  • If so, are there specific things that need to be considered,
  • If not, what the gaps are that need to be covered in order for something to be considered?

I'm really eager to get this done as it's the only remaining issue before a new release can be rolled out (#535988: Bugs to fix before a Release 6.x-1.2).

Thank you.

DamienMcKenna’s picture

Note: Due to D7 being in active development there is still possibly time to add core APIs to handle this, so this is focused on D6.

Per discussion on IRC (#drupal) with greggles and grendzy, then afterwards with dave_reid, grendzy and tha_sun (#drupal and #drupal-seo), the following seemed to be the preferred plan:

  • Track for all nodes whether the checkbox is selected.
  • hook_schema():
    • Define the {url_alias_pathauto} table:
    • nid - the node's nid
    • enabled - boolean (tinyint) to control whether the checkbox is enabled.
  • hook_install() and hook_update_8():
    • Create the {url_alias_pathauto} table.
  • hook_uninstall():
    • Remove the {url_alias_pathauto} table.
  • hook_nodeapi():
    • $op == 'prepare': load the corresponding record from {url_alias_pathauto}. If it does not exist, use a default loaded through a variable ("pathauto_automatic_path") with the default set to TRUE.
    • $op == 'insert', 'update: store the value of the checkbox.

Scenarios that were brought up during the extended discussion:

  1. Paths can be manually added through the APIs. Due to a lack of appropriate hooks in D6 we will have to be able to control the default for this scenario; this is a rationale to not just track the nodes which are overridden.
  2. In order to give flexibility to controlling the defaults, we could have a system-wide default and then allow it to be overridden per content type, e.g.:
        $automatic_default = variable_get('pathauto_automatic_default', TRUE);
        $automatic = variable_get("pathauto_automatic_{$node->type}", $automatic_default);
        $node->pathauto_perform_alias = $automatic;
    

    or more simply:

        $node->pathauto_perform_alias = variable_get("pathauto_automatic_{$node->type}", variable_get('pathauto_automatic_default', TRUE));
    

Regarding the data structure, the following were discussed:

  1. Just store the node nid. In D6 only nodes can have their paths manually assigned so there isn't much benefit to be gained over tracking other object types.
  2. Track the object ID and object type, e.g. obj_id (integer) and obj_type (varchar), to allow future expansion for Drupal 7 and beyond.
  3. Just track the internal path, e.g. "node/123", "taxonomy/term/123", to tie it back to the fact this is for the path system.

Will be going with option 1.

Scenarios left to be decided:

  1. What happens with legacy data? Should it all default to TRUE to replicate the existing workflow?

Please provide feedback ASAP.

Dave Reid’s picture

1. We pathauto has exposed the fact that we *desperately* need proper URL alias APIs (#332333: Add a real API to path.module). Unfortunately, I haven't been able to put any kind of priority on this until now and I wouldn't have time in the next two days to push it in. Sun did write #603870: Inform modules about path alias changes to at *least* get hooks executed in the mess that is path_set_alias(). But I think we'll need to plan that whatever solution we use now, it going to be used in D7 pathauto as well.

2. We need to consider our solution to be able to adapt to the fact that taxonomy terms and users have exposed url alias fields in D7.

3. Let's name our table {url_alias_pathauto} since we're "extending" URL alias capability with pathauto.

4. I think the best start is to store {url_alias_pathauto}.src with the 'base path' of the entity (e.g. node/1, taxonomy/term/5, user/2, etc.). This will allow us to do easy joins on {url_alias} if we needed to.

5. We should probably look at storing entity_type (e.g. 'node'), entity_subtype (e.g. 'story'), and entity_id (e.g. node ID) so that if we need to we can update/delete on any of those three values.

greggles’s picture

fwiw, I spent a lot of time working toward this kind of solution and ultimately failed for a variety of reasons, mainly the lack of those hooks. That's why I like Dave's simple solution in this issue - it works most of the time without diving into the madness of trying to track alias changes in a world without hooks. I'm still open to both solutions, though.

Dave Reid’s picture

Yeah, the more I thought about this issue last night, the more my gut feeling tells me that without proper APIs to support us, putting effort into any type of db-storage issue is just a waste. Why lock ourselves into a solution that we'll only be able to fully leverage until D8? Using a simple return & compare to current alias works for the majority of cases out there. It's easy and it gets the job done.

azinck’s picture

I'm coming in to this discussion late and ill-informed of all the issues at hand. But here's an idea:

In the database we store the automatic-alias (true or false) setting on a per-entity basis in {url_alias_pathauto}.

On the node edit form we ditch the automatic-alias checkbox in favor of 3 radio buttons:

  • Always update -- (maps to "true" in {url_alias_pathauto})
  • Update once, then never update -- (maps to "false" in {url_alias_pathauto})
  • Never update (manual override) -- (maps to "false" in {url_alias_pathauto})

To be clear, option 2 would be an opportunity to do a "reset" of the alias to match the pattern but would then give you a stable alias without having to open the node back up to turn off automatic aliasing.

On loading the node edit form the following logic would be used to determine the state of the radio buttons:
- if there's no record for the entity in {url_alias_pathauto} use option 2
- else obey the setting in {url_alias_pathauto} (option 1 if true, option 3 if false)

If {url_alias_pathauto} indicates a setting of "true" for automatic aliasing for the entity, then we can run the current alias through Dave's algorithm and give a little notice to the user if their current alias appears to be a manual alias. The wording's going to be tricky, but something like "Notice: the current path alias for this node does not appear to match the normal pattern. Set automatic alias to 'never update' if you wish to keep the existing alias." You could even show users the automatically generated alias for comparison to help inform their decision (though that might be confusing since it won't change on-the-fly as they edit the node).

Again...I'm probably missing something big here but in my mind this takes care of most use cases.

DamienMcKenna’s picture

Further discussion went back to using the patch from comment #54 for D6 and keeping this functionality for D7, then working to build out a proper set of APIs for D8.

Here's Dave's patch rerolled for v6.x-1.x (not tested yet).

azinck’s picture

There's a typo on line 48 of the patch in 126. The line should start with 2 slashes instead of 1.

grendzy’s picture

Maybe we should drop the dependency on path.module, and write path-ng. (mostly kidding :-)

samhassell’s picture

Looking forwards to a resolution for this one.

DamienMcKenna’s picture

The typo has been fixed.

DamienMcKenna’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

FYI, due to awesome work from dmitrig01, Drupal 7 has a full API for interacting with paths:

That'll greatly help with properly fixing this in D7, though D6 will have to deal with hacks.

echoz’s picture

subscribing

greggles’s picture

Title: Fix the automatic alias checkbox on node form » If a user changes the automatic path, try to remember that in the future
Status: Needs review » Fixed

Now committed #54 to HEAD and #130 to 6.x-1.x

http://drupal.org/cvs?commit=276094
and http://drupal.org/cvs?commit=276096

Thanks very much for your assistance, everyone. Let's use a new issue for 7.x once #606062: Pathauto for Drupal 7 has moved along.

alison’s picture

Based on the release notes for Pathauto 6.x-1.2, I think I know the answer to this, but just to be safe...

Does the latest (non-devel) release of Pathauto 6.x include the patch at #34 (or at least the functionality of the patch at #34)? If not, will the patch at #34 work on this latest release, or...?

Thank you!

Dave Reid’s picture

@alisonjo2786: It is included in the latest release, 6.x-1.2.

alison’s picture

Thank you, David!

Status: Fixed » Closed (fixed)

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

a_c_m’s picture

Is this getting back ported to 5?

greggles’s picture

I don't plan to work on that. I don't know if anyone else is, but I doubt it.