Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#130 | pathauto-n369840-v6x-1.x.patch.txt | 5.91 KB | DamienMcKenna |
#126 | pathauto-n369840-v6x-1.x.patch | 5.91 KB | DamienMcKenna |
#54 | 369840-pathauto-fix-node-checkbox-D6-2.patch | 5.89 KB | Dave Reid |
#34 | 369840-pathauto-fix-node-checkbox-D6.patch | 5.89 KB | Dave Reid |
#26 | pathauto-auto-alias-fixed.zip | 11.23 KB | binhcan |
Comments
Comment #1
Dave ReidBasically 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.
Comment #2
Dave ReidQuick 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.
Comment #3
Dave ReidRemoved the cruft from the path_redirect API integration, sorry.
Comment #4
Dave ReidRemoves 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.Comment #5
Dave ReidI 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.Comment #6
Dave ReidEven more cleanups possible now that $node->pathauto_perform_alias is always set.
Comment #7
gregglesI 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 ).
Comment #8
Dave ReidOn intial thought, I don't see why it wouldn't work that way. I'll give it a try and report back.
Comment #9
Dave ReidI 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.
Comment #10
Dave Reidgreggles or anyone else, have you had a chance to test and review?
Comment #11
gregglesHere 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 alias2. 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.
Comment #12
Dave ReidHmm...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.Comment #13
greggles@DaveReid #12 - Yeah. I agree :p
Let's wait a little bit for Freso to provide his input.
Comment #14
akahn CreditAttribution: akahn commentedWhat 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.
Comment #15
gregglesAny 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.
Comment #16
akahn CreditAttribution: akahn commentedOk. 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.
Comment #17
japanitrat CreditAttribution: japanitrat commentedThats 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.
Comment #18
agentrickardPatch 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.
Comment #19
Manuel Garcia CreditAttribution: Manuel Garcia commentedI'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...
Comment #20
Dave Reid@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.
Comment #21
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK, thanks for explaining :)
Comment #22
Freso CreditAttribution: Freso commentedI'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.
Comment #23
keff CreditAttribution: keff commentedThank 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...
Comment #24
mrfelton CreditAttribution: mrfelton commentedThe 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!
Comment #25
HXn CreditAttribution: HXn commentedThanks Dave, works great with Pathauto 6.x-1.1.
Comment #26
binhcan CreditAttribution: binhcan commentedI'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.
Comment #27
greggles@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).
Comment #28
mikeker CreditAttribution: mikeker commented@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?
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedJust 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.
Comment #30
moonray CreditAttribution: moonray commentedsubscribing.
Comment #31
Freso CreditAttribution: Freso commentedThe patch doesn't apply to 6.x-2.x.
Comment #32
Jennifer_M CreditAttribution: Jennifer_M commentedNot 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.
Comment #33
AdrianB CreditAttribution: AdrianB commentedsubscribing
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?
Comment #34
Dave ReidRe-rolled for 6.x-1.x. Do we need this rerolled for 6.x-2.x as well?
Comment #35
Freso CreditAttribution: Freso commentedYep. 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.)
Comment #36
WesleyTx CreditAttribution: WesleyTx commentedsubscribing
Comment #37
z33k3r CreditAttribution: z33k3r commentedWhen can we see an official release including the required changes for this patch?
Comment #38
betamos CreditAttribution: betamos commentedI'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?
Comment #39
simePatch at #34 works for me. Is this already in the 1.x dev version?
Comment #40
Nelson Wu CreditAttribution: Nelson Wu commentedTrying 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.
Comment #41
apemantus CreditAttribution: apemantus commentedThis 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.
Comment #42
z33k3r CreditAttribution: z33k3r commented+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?
Comment #43
Dave ReidWe 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:
Comment #44
z33k3r CreditAttribution: z33k3r commentedRight, 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?
Comment #45
gregglesIf 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.
Comment #46
aangel CreditAttribution: aangel commentedWhen 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.
Comment #47
Freso CreditAttribution: Freso commentedSince 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.)
Comment #48
Poieo CreditAttribution: Poieo commentedSubscribing
Comment #49
doniking CreditAttribution: doniking commentedSubscribing
Comment #50
z33k3r CreditAttribution: z33k3r commentedAny luck on creating an official update to include the pre-mentioned patch?
Comment #51
Dave ReidToday is my recovery day from DrupalCampColorado, but I will be rolling this again tomorrow for 6.x-2.x and 6.x-1.x.
Comment #52
mr.j CreditAttribution: mr.j commentedI 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).
Comment #53
Jeff Burnz CreditAttribution: Jeff Burnz commentedSubscribe, really looking forward to this, would fix a long term pita:)
Comment #54
Dave ReidPatch re-rolled for Pathauto HEAD (6.x-2.x)! Tested and works as expected.
Comment #55
Dave ReidAn 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.
Comment #56
aangel CreditAttribution: aangel commentedThanks for that snippet mr.j. I've modified it slightly for 6.x and it's working well for me:
Comment #57
svogel CreditAttribution: svogel commentedSubscribe
Comment #58
darktygur-1 CreditAttribution: darktygur-1 commentedIs 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.
Comment #59
Freso CreditAttribution: Freso commented@ 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.
Comment #60
mikemccaffrey+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.
Comment #61
darktygur-1 CreditAttribution: darktygur-1 commented@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?
Comment #62
R.Hendel CreditAttribution: R.Hendel commentedSubscribing
Comment #63
askibinski CreditAttribution: askibinski commentedFYI:
It looks like the SEO friend module is also addressing this issue,
http://drupal.org/node/528836
Comment #64
Dave ReidMarked #532326: Pathauto Overwrites User's Custom Alias as a duplicate of this issue.
Comment #65
j0nathan CreditAttribution: j0nathan commentedsubscribing...
Comment #66
autopoietic CreditAttribution: autopoietic commentedThankyou Dave, I have patched and tested ok.
Comment #67
mr.j CreditAttribution: mr.j commentedThe "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.
Comment #68
alison(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.
Comment #69
Les Lim@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.
Comment #70
AdrianB CreditAttribution: AdrianB commentedI 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.)
Comment #71
J V CreditAttribution: J V commentedEdit: 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.
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)
Comment #72
okeedoak CreditAttribution: okeedoak commented+1 For storing the value of the checkbox in the database.
Comment #73
okeedoak CreditAttribution: okeedoak commentedOn 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.
Comment #74
alisonThank you! And thanks for the tip for the future.
Comment #75
greggles@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?
Comment #76
okeedoak CreditAttribution: okeedoak commented@greggles Tried the 6.x-1.x-dev version. No problems. Repatched same dev version, same problem.
Suggestions?
Comment #77
Dave ReidPatch 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.
Comment #78
okeedoak CreditAttribution: okeedoak commentedI 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?
Comment #79
greggles6.x-2.x is not suitable for production. It works ok, but may change drastically.
Comment #80
okeedoak CreditAttribution: okeedoak commentedPatch 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.
Comment #81
alisonYay for #34! Perfect!
Comment #82
okeedoak CreditAttribution: okeedoak commentedWonder why #34 worked for you and threw an error for me?
Comment #83
alisonWell, 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...?
Comment #84
okeedoak CreditAttribution: okeedoak commentedThe 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.
Comment #85
Les Lim#84: that would indicate that
_pathauto_include()
needs to get called inpathauto_form_alter()
beforepathauto_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.
Comment #86
Les LimThe 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?
Comment #87
gregglesInstalled and reviewed tonight.
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.
Comment #88
gpk CreditAttribution: gpk commentedVery 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.)
Comment #89
gpk CreditAttribution: gpk commented@87:
>changes a lot of the logic in the form_alter
Re. the change from:
to just
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 inpathauto_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.Comment #90
gpk CreditAttribution: gpk commentedSuggested 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).
Comment #91
alisonHere'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...)
Comment #92
alison@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!)
Comment #93
alison@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.)
Comment #94
greggles#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."
Comment #95
mxmilkiib CreditAttribution: mxmilkiib commentedsubscribe
Comment #96
alison@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:
Comment #97
SpikeX CreditAttribution: SpikeX commentedI 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.
Comment #98
Jax CreditAttribution: Jax commentedsubscribe
Comment #99
featherbellySubscribing
Comment #100
HnLn CreditAttribution: HnLn commentedsubscribe
Comment #101
alisonWere 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?
Comment #102
John Pitcairn CreditAttribution: John Pitcairn commentedRe #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.
Comment #103
gpk CreditAttribution: gpk commented@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!!
Comment #104
tim.plunkettsubscribing
Comment #105
easp CreditAttribution: easp commentedsubscribing
Comment #106
jeffleeismyhero CreditAttribution: jeffleeismyhero commentedsubscribing
Comment #107
John Pitcairn CreditAttribution: John Pitcairn commentedRe #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.
Comment #108
sjhobson CreditAttribution: sjhobson commentedsubscribing
Comment #109
cerup CreditAttribution: cerup commentedsubscribing.
This is a necessity.
Comment #110
alison@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.
Comment #111
grendzy CreditAttribution: grendzy commentedsubscribe
Comment #112
mattyoung CreditAttribution: mattyoung commented.
Comment #113
grendzy CreditAttribution: grendzy commentedI'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
So while I realize that adding a new table is distasteful to some, it appears to be the only correct solution.
Comment #114
grendzy CreditAttribution: grendzy commentedComment #115
Dave Reid@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.
Comment #116
greggles@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.
Comment #117
grendzy CreditAttribution: grendzy commented[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".
Comment #118
jrabeemer CreditAttribution: jrabeemer commented+1 Eagerly awaiting the patch for 6.x-1.x port!!
Comment #119
xjmTracking the 6.x patch too.
Comment #120
DamienMcKennaCould someone please explain:
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.
Comment #121
DamienMcKennaNote: 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:
Scenarios that were brought up during the extended discussion:
or more simply:
Regarding the data structure, the following were discussed:
Will be going with option 1.
Scenarios left to be decided:
Please provide feedback ASAP.
Comment #122
Dave Reid1. 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.
Comment #123
gregglesfwiw, 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.
Comment #124
Dave ReidYeah, 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.
Comment #125
azinck CreditAttribution: azinck commentedI'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:
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.
Comment #126
DamienMcKennaFurther 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).
Comment #127
azinck CreditAttribution: azinck commentedThere's a typo on line 48 of the patch in 126. The line should start with 2 slashes instead of 1.
Comment #128
grendzy CreditAttribution: grendzy commentedMaybe we should drop the dependency on path.module, and write path-ng. (mostly kidding :-)
Comment #129
samhassell CreditAttribution: samhassell commentedLooking forwards to a resolution for this one.
Comment #130
DamienMcKennaThe typo has been fixed.
Comment #131
DamienMcKennaComment #132
DamienMcKennaFYI, 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.
Comment #133
echoz CreditAttribution: echoz commentedsubscribing
Comment #134
gregglesNow 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.
Comment #135
alisonBased 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!
Comment #136
Dave Reid@alisonjo2786: It is included in the latest release, 6.x-1.2.
Comment #137
alisonThank you, David!
Comment #139
a_c_m CreditAttribution: a_c_m commentedIs this getting back ported to 5?
Comment #140
gregglesI don't plan to work on that. I don't know if anyone else is, but I doubt it.