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 think the next big problem we have in Pathauto is solving what happens when code does something like this:
// Load a node that has had $node->path set to 'mymanualalias';
$node = node_load(1);
$node->title = 'My new title';
node_save($node);
// The node has now had it's alias reset to 'content/my-new-title';
I think this behavior is undesired and I think we need to not do anything if $node->pathauto_perform_alias is not defined (and use this same logic for other entities as well). This would likely solve the issues listed as related and would make Pathauto Persistent State obsolete.
Comment | File | Size | Author |
---|---|---|---|
#319 | interdiff.txt | 763 bytes | Dave Reid |
#319 | 9636222-pathauto-persist.patch | 13.89 KB | Dave Reid |
#317 | 9636222-pathauto-persist.patch | 13.25 KB | Dave Reid |
#315 | pathauto-persists-936222-315.patch | 15.74 KB | emanaton |
#299 | merge_in-936222-299.patch | 15.47 KB | joelpittet |
Comments
Comment #1
gregglesIf someone changes the title and Pathauto doesn't update the path that seems like really weird behavior. I get that this is tricky to do right, but I think Pathauto should work on node_save.
Does it help for those 4 issues if people use the API a bit more correctly (i.e. using node_object_prepare).
Comment #2
Dave ReidHrm, I don't initially see how implementing node_object_prepare() will help, since Pathauto doesn't do any logic in hook_nodeapi('prepare'), although we very likely could put our "automatic alias" comparison logic there.
Comment #3
Dave ReidGah, I just ran into this while working on webforms for the DrupalCon site. Everytime I would save the webform settings, it does a node_save() on an existing node, and it would reset my manual path alias. I think it makes a lot of sense to:
1. If $node->pathauto_perform_alias is undefined for a new node, that we should default it to TRUE and let the auto-alias run.
2. If $node->pathauto_perform_alias is undefined for an existing node, that we should not do anything. This doesn't affect when users submit the normal node form, as the property will always be defined then.
Comment #4
Dave ReidAnd so this is all we'd need to change.
Comment #5
Dave Reid(01:25:44 PM) davereid: greggles: So funny story. I'm working on the scholarship webform for chi2011....
(01:26:00 PM) davereid: And every time I save the settings under the 'Webform' tab
(01:26:06 PM) davereid: The URL alias gets reset to the automatic one
(01:26:21 PM) davereid: Because pathauto runs on node_save() when !isset($node->pathauto_perform_alias)
(01:26:54 PM) davereid: which is why I really think http://drupal.org/node/936222 needs to be considered. :)
(01:26:55 PM) Druplicon: http://drupal.org/node/936222 => Rethink if we should interfere on node_save() calls => Pathauto, Code, normal, active, 2 comments, 1 IRC mention
(01:28:01 PM) greggles: I think we should just move our logic to determine whether or not it needs to be build hook_nodeapi('prepare'
(01:28:21 PM) greggles: if "node_save($node)" during import stops setting aliases that would be a major wtf for a lot of folks
(01:28:23 PM) greggles: davereid:
(01:30:19 PM) davereid: Hrm, that's going to add additional time since we need to run the comparison that we do currently when displaying the node form?
(01:34:02 PM) davereid: If they're new nodes, it should be easy to detect that logic since $node->nid is not defined yet before save.
(01:34:28 PM) davereid: But for existing nodes, I don't think we should do anything if $node->pathauto_perform_alias() is undefined (aka was not displayed on the form)
(01:34:50 PM) davereid: I agree we should perform aliases on new nodes by default
(01:43:52 PM) greggles: I think on new and updated
(01:44:01 PM) greggles: programmatically we should behave just like the form
(01:46:29 PM) greggles: davereid: I think the additional time is an acceptable tradeoff
(01:46:57 PM) davereid: the only way we can effectively do that then is on hook_node_load
(01:49:04 PM) davereid: ok, so what you're saying is that other modules should be doing $node = node_load(1); node_object_perpare($node); $node->title = 'changed title'; node_save($node); ?
(01:49:43 PM) davereid: because core doesn't
(01:49:44 PM) davereid: :(
(01:49:48 PM) davereid: which is the big flaw
(01:50:40 PM) davereid: So I can see us adding the comparison logic in hook_node_prepare() which will catch preventable cases in contrib
(01:51:35 PM) davereid: But for cases like using the operations on admin/content/node to unpublish/publish a node should still not change a node's alias, which is currently does. And the only way to avoid that is to either put the comparison in hook_node_load() or to skip if $node->pathauto_perform_alias is undefined
(01:51:59 PM) davereid: Anyway, just a little frustrated after having to revent it like 10 times on the chicago site. I'll put this in the issue
(01:52:03 PM) davereid: revent/revert
(01:53:05 PM) greggles: yeah
(01:53:13 PM) greggles: davereid: I appreciate we should be doing something different
(01:54:03 PM) greggles: but I want to preserve consistency in the API and the UI
(01:59:58 PM) davereid: I agree to some extend. I think if we get to using hook_node_load() to run comparisons then we just need to merge in pathauto_persist at that point.
(02:00:09 PM) davereid: I'll play with this and see what I can come up with.
(02:00:36 PM) greggles: so, 3 years ago freso and I were trying to do fundraising to pay for pathauto_persist since that felt like the better long term solution
(02:00:43 PM) greggles: ;)
(02:01:18 PM) greggles: I'm fine with the idea of replacing the current "comparison check" code with pathauto_persist
(02:02:37 PM) davereid: And it would be fairly easy to write a batch update function that fills in the data for that table if we wanted to as well.
(02:03:24 PM) ***davereid is fine with this not touching the 6.x-1.x branch as well
Comment #6
klonos...subscribing
Comment #7
Dave ReidBumping to major as this needs to be addressed before 7.0.
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedSince #4 doesn't apply against HEAD anyway, here's the same thing applied against 7.x-1.0-alpha2. Just posting here in case it's useful to anyone running off point releases instead of dev snapshots. Leaving at "needs work", since this doesn't apply against HEAD.
Keep in mind that this patch does have side effects as discussed in this issue. But in the meantime, it's useful for allowing webform nodes to be saved with a custom (non-pathauto) alias. Looking forward to a proper solution.
Comment #9
quicksketchMarked #1024854: URL path settings bug duplicate.
I should note that you don't even need to change the node title at all. Simply loading the node and then saving it again will break previously custom aliases.
Steps:
1. Create a new node (doesn't have to be a Webform but it's easily reproduce-able with Webforms).
2. Give this node a custom alias.
3. Execute the following PHP code:
4. Your custom alias is lost and reverted to the default alias pattern.
Clearly this is not expected behavior. If a user did not mark an item with an "automatic" alias, Pathauto should never force an automatic behavior upon the node.
My suggestion here is that Pathauto should create a database table to keep track of which nodes have "automatic" alias and those that do not. Then it simply has to obey this "automatic" setting when things like the node title change.
Comment #10
Dave ReidYes this was the idea behind Pathauto persist which is going to be merged into Pathauto. Retitling issue to for the new direction.
Comment #11
quicksketchSweet, this has my support, for what it's worth.
Comment #12
klonosGreat news!
Comment #13
Gábor HojtsyThe code moved around in that area, so here is a quick reroll of the patch with test fixes included until the larger role of the issue is fulfilled.
Comment #14
JacobSingh CreditAttribution: JacobSingh commentedHmm... this patch causes test to fail in generating bulk aliases. Trying to grok it still, but NW for now.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedBack to active as per #10. #13 solves the OD, but see #1, #5, and #10 for why it's not viable, and why a completely new approach is needed.
Here's the problem:
Suppose node/1 has the pathauto checkbox set, and node/2 has a custom path, and you have this code:
In HEAD, $node2 gets its path reverted to a pathauto path. This surfaces when saving webform nodes from the UI, but can also surface anywhere node_save() is called outside the strict context of a particular node form submission process.
With #13, $node2 keeps its custom path, but $node1 fails to get its path updated to reflect the new title.
Depending on your site, you may prefer one situation over the other, but ultimately, we need both solved.
Note also that with both HEAD and with #13, both use-cases can work as expected by uncommenting the commented lines in the above snippet. What this issue needs to solve is for either the node_load() or the node_save() to automatically know the current state of $node->path['pathauto'], without client code having to set it.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedHere's an approach that passes all existing tests, and adds new tests for the problem at hand. It's a little bit of ugly code duplication, but I think might be worth committing until pathauto_persist is worked out.
Comment #17
YK85 CreditAttribution: YK85 commentedsubscribing
Comment #18
JacobSingh CreditAttribution: JacobSingh commentedThis looks good to me.
Comment #19
quicksketchI'd advocate not including the latest patch into the module. Is it really too much to ask to add pathauto_persist into the module? How hard is this really? A database table an 80 lines of code? I'd like to see a real solution on this one (finally), not yet another band-aid.
Comment #20
Dave Reid@quicksketch: I agree. This issue is about merging in that functionality for good.
Comment #21
klonosI have started putting together a collection of issues + patch instructions here: #1067286: [META] core/i18n/pathauto: aliases, patches & issues havoc (both D6 & D7)
This is still work in progress and it actually is quite a handful may I add (since I need to first gather all needed/related data and organize it in a proper structure + test all of this as I go - in both D6 & D7). I really need an answer regarding the progress of this issue here before I move on. So,
@Dave/Greg/Frederik: Can you please update us on where we are at right now? Which patch needs to be applied here in order to test and report back? Are we actively working on this one? Is there an estimation of when it might be implemented? I am not asking for a promise/commitment - just a rough estimation. Will it be days?, weeks?, months?. Also can you please answer these few questions (a simple yes/no will do):
1. It is a final decision to add pathauto_persist functionality to pathauto. We are firm on that. Right?
2. It will be implemented in 7.x-1.x first (?)...
3. ...and then be backported to 6.x-2.x (?)...
4. ...but it will NOT be implemented in 6.x-1.x though (?).
5. Alex's approach (posts #15 & #16) do make sense and seem to address issues (?)...
6. ...but will not be considered before there's an initial merge of pathauto_persist features first (?).
Thanx beforehand for your time.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedAFAIK, #16 solves every bug that this issue started out trying to solve, and does not introduce any new bugs, unlike prior patches that replaced one bug for a different one, as described in #15.
However, merging in pathauto_persist would be a better way of solving the same bug that #16 solves. Given #19 and #20, setting the status to "active", since further review and work on #16 isn't desired by the maintainer.
My own opinion is a slight disagreement from #19. If merging pathauto_persist can happen relatively quickly (not sure how to quantify that), then I agree that committing #16 is an unnecessary deviation from the project's real goals. OTOH, if the people who can make pathauto_persist happen are too busy, then I think committing #16 is a useful interim step. Its code can later be replaced by pathauto_persist, but in the meantime, it would solve the same problem. But even if #16 isn't committed, I hope it's useful to people running sites with the Webform module, or other use-cases where this bug surfaces, until this issue is solved the right way.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedWell if nothing else the tests in #16 look perfectly good.
Even if someone comes along and writes a perfect patch some day in the magical future, they still should incorporate those tests :)
Comment #24
klonos...any progress here? I guess most of us here agree in that we should incorporate the functionality of pathauto_persist asap (which is what this issue is about), but it is not clear to me if we have a firm decision on that being the path we are going to take.
No one had some time to clarify things for me by answering any of my questions in #21 (a simple yes/no would suffice), so I cannot go forward with my meta-issue #1067286: [META] core/i18n/pathauto: aliases, patches & issues havoc (both D6 & D7)
Comment #25
Dave ReidSorry I am very busy this month. Had DrupalCon last week and I am getting married on the 19th, so please be patient or feel free to start patching. I should have more free time to resume working on contrib in April once I return from my honeymoon.
Comment #26
klonos...drupal can wait. You go have a great wedding now ;)
Comment #27
vinmassaro CreditAttribution: vinmassaro commentedI ran into this issue today in Drupal 6 by:
1. Create a new node, uncheck "Automatic alias". Enter custom alias 'custom/alias'. Save node.
2. Change the workflow state. Hit submit.
3. Alias for the node has now been changed to its pathauto setting.
Adding the pathauto_persist module fixed this problem. I have the need to use this on a production site right now, but want to make sure when this is rolled into pathauto proper that upgrading to that won't break anything. Thoughts? Thanks.
Comment #28
klonosHey Vincent, nobody promises anything in open source world -especially when using dev versions-. So, I guess it is a risk you'll have to take with your production site. I am sure you'll find out how this evolves by following this issue here. So you'll know when this feature is actually merged in pathauto and that will mean it's time to uninstall the contrib pathauto_persist module. Besides, it will most likely be noted in bold letters in that module's page that its features have been merged and there is no need for it anymore when that happens ;)
Comment #29
quicksketchHere's a pair of patches to bring the functionality of pathauto_persist to Pathauto. The D6 branch seems to be an exact 1-to-1 match functionality-wise, though I avoided loading the $node->pathauto_perform_alias property on node_load(), instead it's loaded just before save or when displaying the node form, since this variable isn't otherwise helpful and saves one query per node-load.
The D7 branch it's not quite so equivalent. This patch only provides pathauto_persist functionality for nodes, not other entity types. The reason being Pathauto doesn't yet implement hook_entity_insert/update/delete(), instead it implements the specific node/user/term hooks. I'm not sure if this persist functionality should introduce these hooks, especially since there's no location to set a custom term or user path anyway in D7 (that I know of), so implementing the persist functionality for those types doesn't really make a difference anyway (does it?).
At the very least, these patches make the new "pathauto" table and work for nodes, where this problem is most prevalent.
Comment #30
quicksketchD7 version (same as above) without suffix so it can be tested.
Comment #31
Dave ReidI wonder if rather than running an batch node update from Pathauto, we should just check if the {pathauto_perist} table exists, copy over any data, and then uninstall pathauto_persist.module from the Pathauto update. That way we don't have to process all nodes.
Comment #32
Dave ReidBTW, the testbot can only check if the patch applies or not, since we require dependencies that PIFR doesn't download for us (token.module) - so tests have to be run and verified locally.
Comment #33
quicksketchI'd probably say that it would be nice to pull in pathauto_persist data if it exists, but really we should generate this information even if pathauto_persist did not exist previously. Then any nodes that don't currently have their alias and pathauto's alias match will be impervious to the path-reset issue, otherwise the issue will still exist for old nodes but not new ones. Even pathauto_persist doesn't solve anything for existing nodes.
Comment #34
Dave ReidHrm, I just worry about the large amount of data to process but I can see that doing both - copying over the data & uninstalling pathauto_persist and then processing any remaining nodes would probably be good. We could probably mix around some of the checks like comparing drupal_get_path_alias() to $uri['path'] before fetching the node's pattern and generating the pathauto alias because if those match we already know that the {pathauto}.pathauto value should be 0.
Comment #35
quicksketchAfter looking at pathauto_persist I realize that I'm not sure pathauto should accommodate it really. It's a module with 300 users compared with 203,000. That's less than 0.2% of users (even if we're part of that 0.2%). I'd probably say those users should just uninstall the module or rename their table if they want to keep their old data.
Comment #36
Dave ReidStill, checking the table can make the upgrade for those 300 users significantly faster. :)
Comment #37
klonosI don't use the pathauto_persist module, so I can't test the 'migration' thing right now, but I want to help get this done. Care to explain what needs to be 'manually' tested?
PS: If you also provide a list of things that need to be tested with the 'migration' check (once implemented), then I'll try to find some time and install pathauto_persist and test that as well. Let me know.
Comment #38
Cyberwolf CreditAttribution: Cyberwolf commentedSubscribing.
Comment #39
stackpr CreditAttribution: stackpr commentedSubscribing.
Comment #40
quicksketchOkay, here's a reroll that utilizes the pathauto_persist table if available. I also made some other optimizations by checking if a node even has the potential to have an auto-alias before calling node_load(). Because this will likely decrease the number of node_load() calls necessary I've also increased the number of nodes processed at once from 50 to 100.
@klonos: A good way to test these patches is to set up a site with Pathauto, enable aliases for multiple node types. Create a few nodes, making some of them use the automatic alias and others using a manually specified alias that alters the automatic one. Then, apply this patch. Run update.php. Things to check:
- Ensure that those nodes whose path's you manually created have a "pathauto" column of 0 in the new "pathauto" table, those nodes that use the automatic alias should have a pathauto column of 1.
- Edit your nodes and toggle the automatic alias option on and off and make sure that its value is saved in the database.
- Try deleting nodes and make sure that the record in the pathauto table is removed.
Comment #41
dimitriseng CreditAttribution: dimitriseng commentedSubscribing.
Comment #42
lpalgarvio CreditAttribution: lpalgarvio commentedlooking forward to this :)
Comment #43
Taxoman CreditAttribution: Taxoman commentedSubscribing
Comment #44
klonosStill haunted by this :/
...applied d7 patch in #40 (manually) and tried editing a node that already had a translation. I got this on save:
...ooops!!! Of course there is no table. I forgot to run update.php. Stupid me! But now all I get is WSOD. Let's try being cunning here and "manually" point the browser to http://my.site.net/update.php...
...oookay then. So, it seems that the update number we meant to use for this patch is already taken by another update that got committed in the meantime. Fine!, then let's try to be cunning again... edit pathauto.install and change the
7005
of the function that creates the pathauto table for storing path alias status (that is in line #207 in the latest 7.x dev once patch in post #40 is applied) to7006
. ...reload update.php and now we seem to be good to go... 1 pending update... run it......oh well, at least now I don't get WSOD anymore. Let's give it a final try...
- Disable pathauto
- Uninstall pathauto
- Re-enable pathauto
This time it did well. Thank God!
So... setting back to NW at the very least because the number of the update function needs to be updated (perhaps a re-roll is also required + maybe the errors I got actually point to things we might have overlooked).
Note: I only tested the D7 version of the patch in #40.
Comment #45
klonos...PS: just clearing up that the above is for a use case scenario where one tries to apply the patch in #40 to a site that already has pathauto enabled and some multilanguage/translated content.
PPS: What a wonderful mess I got myself into in order to get the desired functionality, huh?
Comment #46
yannisc CreditAttribution: yannisc commentedsubscribing
Comment #47
klonosWhile this is in NW, I've noticed this:
- create en (source) node.
- leave the auto-generate alias checkbox checked in order to get an initial one.
- edit the node again and disable the "Automatic alias" check box (in order to set it to not auto-generate again).
- save node.
- edit node again -> "Automatic alias" check box unchecked state not remembered.
That happens because -as it is now- in order to have the state being remembered, one needs to manually edit the alias. Am I the only one annoyed by this behavior?
Comment #48
klonos...also, bulk delete of aliases should not delete node aliases when "Automatic alias" checkbox in node is unchecked and its state remembered ...or it should at the very least be optional.
You see, bulk update doesn't (by design of this feature) generate aliases for them. I believe that the bulk delete action was meant to be used prior to bulk update anyways (this pair of actions is actually a two-step procedure in order to achieve updating/re-creation of already created aliases). While this was fine back in the days, now that we are trying to achieve optionally persistent paths this needs to be handled differently.
Comment #49
klonosActually, the issue in #47 above might as well be a "byproduct" of #369840: If a user changes the automatic path, try to remember that in the future, but I am not sure(?).
Comment #50
effulgentsia CreditAttribution: effulgentsia commentedI agree that #47 is annoying from a UX standpoint. It's the current functionality of Pathauto, but I would expect #40 to fix that. If it doesn't, then I think that's a bug that the next reroll of #40 should address.
Comment #51
klonos...it doesn't! I have the patch already applied.
Comment #52
ja09 CreditAttribution: ja09 commentedsubscribing
Comment #53
dynamicdan CreditAttribution: dynamicdan commentedI need this..
I have like 15 aliases I need to keep from 1000... There is no bulk delete option on the list page or delete by node type... so I'm screwed!
BTW, this is 1 year in the making... can't be that hard right?
Comment #54
rickmanelius CreditAttribution: rickmanelius commentedI can confirm the issues in #40. I moved the update in the patch to 7006, but still get
I can also confirm that even if I ignore the update warning, I can't seem to set the alias in the node edit form. It shows the custom alias, but always defaults to the automatic one.
It seems we already had a lot of great work on this. Is it a quick fix at this point or a larger rewrite?
Comment #55
aacraig CreditAttribution: aacraig commentedI have posted a hack fix to this here: http://drupal.org/node/1342762
It looks to me like there's simply an expectation that $node->path will always be set in pathauto_node_update_alias(), when this doesn't seem to always be the case.
At any rate, checking to make sure that $node->path is set before checking it's value has sorted out the problem for me.
Comment #56
klonosHey Aaron, the other issue was closed as a duplicate of this one here. Do you mind copy-pasting your findings and the fix from that issue here so it won't get lost over time. Thanx.
Comment #57
rickmanelius CreditAttribution: rickmanelius commentedFrom #1342762: Manually set URL aliases get discarded and replaced by an automatic alias when the derived translation node is saved for also taking translation into account.
Old code:
if (isset($node->path['pathauto']) && empty($node->path['pathauto'])) {
The "fix":
if (!isset($node->path) || (isset($node->path['pathauto']) && empty($node->path['pathauto']))) {
Comment #58
aacraig CreditAttribution: aacraig commentedThanks for that Rick.
Comment #59
klonosI'd have done that myself, but didn't want to steal Aaron's thunder ;)
Comment #60
rickmanelius CreditAttribution: rickmanelius commentedI only did that so we didn't steal Aaron's time :)
Comment #61
dema502 CreditAttribution: dema502 commentedsubscribing
Comment #62
klonosJust to recap what needs to be done/addressed here:
1. The previous state of the "Generate automatic URL alias" checkbox needs to be remembered across successive node edits. Please see #47 for reproduction steps. Alex claims in #50 that the patch intents to do that, but as I've already said, it doesn't. Can someone please verify??
2. Bulk update needs to be adjusted to not update URL aliases for nodes that have their "Generate automatic URL alias" checkbox disabled (unchecked). See #48.
3. The mess with update version numbers (7005/7006) from #44.
@rickmanelius (and all others that fail to get the update even when moving to 7006): Hey Rick, sorry for the late reply. My current D7 installations with latest pathauto 7.x-1.x-dev all seem to already be in 7006 (I guess a lot of other things got committed in the meantime), so perhaps one might need to change the update number in the patch to 7007 instead of 7006 to get out of the WSOD mess. In order to be sure about what number will work for you, please check the
schema_version
field in thesystem
table in your db before altering the update # in the patch to see what version you are at. If still at 7005, then change to 7006. If at 7006, then change to 7007.Be warned though:
Any "manual" change in these update numbers might cause future updates to fail. Do all these only in order to help testing here in a non-production site.
Point is that the d7 patch in #40 needs to be rerolled against latest dev and at the same time attention needs to be paid to the version number used for the
pathauto_update_#
function that creates the pathauto table for storing path alias status.Perhaps the d6 patch in #40 needs a reroll too. Can someone please check the latest 6.x dev and tell us what the db update number for pathauto is in their system table (the patch uses 6202)?
Comment #63
klonos@quicksketch: Hey Nathan, do you have time to work on this again so we can finally get it in? I'll help with testing and however else I can. Drop a line if you find the time so to let us know. Thanx in advance.
Comment #64
quicksketchHi @klonos. Sure I can give this an attempt at a reroll. I don't think much has really happened in this space so it should just be a matter of changing the update number and including the fix in #57.
Comment #65
klonosThanx Nathan for even taking the time to reply so promptly. Yes, that would be great. If it is too much trouble to address #47 and #48, then we can leave those for later on and perhaps move them to separate issues.
Comment #66
Island Usurper CreditAttribution: Island Usurper commentedRerolled both patches in #40 to latest devs.
Comment #68
stacysimpson CreditAttribution: stacysimpson commentedSubscribing
Comment #69
quicksketch@stacysimpson There is now a "Follow" button you can click at the top of the page instead of leaving a comment to subscribe.
Comment #70
blainelang CreditAttribution: blainelang commented#66: pathauto_persist.patch queued for re-testing.
Comment #72
theagonizer CreditAttribution: theagonizer commentedI'de just like to note for future searches that land on this page that the solution is solved now by doing the following:
install Pathauto 6.x-2.0 the problem still exists in earlier versions
Intall Pathauto_presist
I beat my head against my screen for a while because I had an earlier version of pathauto installed.
Comment #73
blainelang CreditAttribution: blainelang commentedI think you mean install the module pathauto_persist http://drupal.org/project/pathauto_persist
Comment #74
nodecode CreditAttribution: nodecode commentednevermind.
Comment #75
klonosIt's been more than a year now. Can we please get some love in this issue?
Comment #76
mikeryan+1: Just got burned by this on a migration project: #1630624: When preserving old aliases, attachment processing overwrites them with pathauto enabled. I'll work around it in wordpress_migrate, but it'd be nice if existing aliases were respected unless pathauto was explicitly requested to generate an alias.
Comment #77
populist CreditAttribution: populist commentedHere is a rerolled patch that implements this functionality. This causes an issue with Panelizer when used to panelize nodes with custom URLs and it would be nice to see this rolled into the main release.
Comment #79
lathansmall adjustment to this patch as we were getting
on this update. Added a check to make sure the node we are obtaining values off is not empty (I assume node_load is the correct approach here and not entity_load.
Comment #80
lathanMy bad here is the correct patch for this issue.
Comment #81
Jim Kirkpatrick CreditAttribution: Jim Kirkpatrick commentedComment #83
klonos@jucallme, @populist, @Island Usurper @quicksketch: Hey Lathan, Matt, Lyle & Nathan! I'm pinging you guys because you are the ones that have worked on this recently. Can we get this moving please? Do you have the time/will to work on it again and get over with this issue that's been tormenting us for 2 years now?
Besides the fact that we need to get the latest patch to pass green I believe that all we have left is what's mentioned in #62 and #64. Would it help if I updated the issue summary with all this info so we can have it in a single place?
Also @Dave Reid: would you have the time/energy to also be involved in this?
Thanx to everybody in advance.
Comment #84
Fabianx CreditAttribution: Fabianx commentedWhy is the schema duplicated here?
The tests need updating, too. Simpletest _must_ succeed.
Best,
Fabian
Comment #85
pjcdawkins CreditAttribution: pjcdawkins commentedFabian: update functions should duplicate the schema. See http://drupal.org/node/150220
Comment #86
Fabianx CreditAttribution: Fabianx commentedThanks for the explanation. That is fine then.
This issue still needs updated tests.
Comment #87
dimitriseng CreditAttribution: dimitriseng commentedAny progress on this? The pathauto_persist module seems to be working ok but would be great to fix this in pathauto. There is an open issue in that module's queue as well to do this #937826: Merge into Pathauto ?. Thank you.
Comment #88
nodecode CreditAttribution: nodecode commentedAny foreseeable movement on this issue which currently affects about a quarter of a million websites?
Comment #89
aendra CreditAttribution: aendra commented+1 on nodecode's point. This issue is nuts.
Here's an idea: since both pathauto and pathauto_persist are maintained by the same developer, why have it as a separate project? While we're waiting on it to be merged, why not just include it by default with pathauto?
Honestly, I might try my hand at this over the weekend. We've just had to tell a client to be careful when updating Panelizer content ("If you are on an existing node, go to the Edit page and hit save before you go to the Panelizer content page. Why? We're not entirely sure ourselves...") simply because this issue isn't resolved and we didn't enable pathauto_persist when we first installed everything. If you could apply "DrupalWTF" tags to contrib modules, I would to this issue.
Comment #90
mvcHere's a first stab at merging in this functionality, which works for me. Settings are correctly copied over from pathauto_persist to pathauto, if that module was installed. pathauto_persist can safely be disabled & uninstalled after running update.php (although it's harmless to leave it in place).
Comment #92
ajFernandez CreditAttribution: ajFernandez commented#90: pathauto-persist-936222-90.patch queued for re-testing.
Comment #94
mvcCurrent status: After run some tests on a local site, I believe my patch in #90 works correctly, but that the test suite for this module needs to be updated. I gave it a shot but I don't know simpletest well enough so that part needs work from someone else. In the meantime, reports from patch reviewers would be very welcome; I'll respond to problem reports if I can.
Comment #95
brianV CreditAttribution: brianV commentedI fixed the two failing tests. The code that was failing was:
However, pathauto_node_update_alias() starts with the following:
Therefore, the node as created by the test will not have an alias. The attached patch updates the test to reflect this:
Comment #96
d.clarke CreditAttribution: d.clarke commentedThe patch from #95 applies against dev and works as expected and resolves my issue (panelizer related). The updates to the unit test makes sense to me and all test pass on my machine.
Side note: I noticed while testing if I create a new node, uncheck the "Generate automatic URL alias" checkbox, leave the URL alias field blank, save the node, and then select it from the content list and choose Update URL Alias from the update options drop down and click the update button, it'll show "Updated URL alias for 1 node." I was expecting this to say "No new URL aliases to generate." but this is how it works with the current stable release so it is a separate issue if an issue at all.
Comment #97
Fabianx CreditAttribution: Fabianx commentedLooks very close to RTBC to me. Anyone else up to a review?
Comment #98
mvcthanks for fixing the tests, brianV.
looking at this again, my only remaining question is how to phrase the message to the user when this module is upgraded. i originally wrote:
however, on second thought, it would be better to say:
thoughts?
Comment #99
mvcminor UX change from #98
Comment #100
brianV CreditAttribution: brianV commentedThat is a definite improvement in the text.
I was going to suggest that pathauto should disable pathauto_persist in order to prevent having two modules doing the process in duplicate as it's quite possible that people upgrading via Drush etc. may not actually see the upgrade message.
Unfortunately, there's always the possibility that someone has custom code that looks at the pathauto_persist table, so perhaps not really a good solution.
Perhaps a better method is to use hook_requirements() to detect if both pathauto and pathauto_persist are both enabled, and display an 'Info'- or 'Warning'-level notice on the status report for better visibility?
eg.,
If this idea is worth pursuing, I'll roll it into MVC's latest patch; that said, I think that one is RTBC-worthy as is, so I don't want to add new elements and delay getting this in.
Comment #101
mvci think your patch makes a lot of sense, brianV. i did actually try disabling pathauto_persist in the update hook but got lots of strange errors (i don't think you're supposed to be able to do that there). plus, on the principle of least surprise, it would be better to simply inform users rather than disable that module automatically.
fwiw drush will display that message, but as you say users might not notice it. with 467,164 users it makes sense to take the time to make this module as easy to use as possible :)
Comment #102
quicksketchYou should be able to disable other modules in update hooks. I've done this several times in my own modules and even used it to *install* other modules when a single module splits into two. Perhaps the oddity was caused by module_disable() taking an array of modules, though its name seems to indicate it would take string?
It should look like this:
Uninstalling the module probably won't work since we've actually renamed its existing table.
Great work on carrying this patch guys. I'll be thrilled to see it fixed once and for all.
Comment #103
brianV CreditAttribution: brianV commentedAttached is a patch that adds the hook_requirements from #100 to mvc's patch from #99.
Comment #104
mvcthanks, brianV. your changes looks good to me, and since various people have reviewed the parts i contributed, i'll mark this whole thing as RTBC.
Comment #105
klonosIt feels like we're really close now! It's been 3 years that I'm tormented by this and I'd really love to be rid of this bug once and for all.
I cannot do actual code review, but manual testing works in my use cases. Plus, I've been using pathauto_persist for quite a long time without any issues and this patch is mostly based on its logic/code. So, +1 RTBC from me. Can we please get this in and file separate issues for minor tweaking here and there as needed? Lets just fix the major issue at hand first and then improve on it.
Comment #106
Dave ReidThanks everyone for the great work on this so far!
Our APIs are calling this 'pathauto_state', so likely the table should match that rather than just 'pathauto'. Also missing empty description. I likely didn't have one in pathauto_persist, but we should ensure one is described here.
We cannot reference stuff in hook_schema() in update functions. We *must* copy the schema, even though it may be large, but change in schema will then break future update functions.
Overall, we're missing conditions for hook_entity_delete() and an API for pathauto_entity_state_delete(). I'm also wishing this actually disabled the pathauto_persist module in the update hook and copied data, rather than renaming the table. Renaming kind of weirded me out, but if it's the best thing we can do, then I guess it's OK.
As a best practice, we should probably ensure that the schema for the pathauto_persist table when re-created is run from the actual schema from pathauto_persist, and not using pathauto's schema.
Comment #107
mvcthanks for the comments, dave reid. i'll try to address them later this week if no-one else has a chance to do that first.
for the record, i renamed the table instead of copying the data because it would be a faster operation on very large sites. and sure, i can accept that pathauto_persist should be disabled during the upgrade of this module, although we would still need to check for it to cover the case where someone installs new, recent versions of both modules, unaware that pathauto_persist has become redundant.
Comment #108
mvc* added table description to schema
* copied schema to hook_update_N()
* added hook_entity_delete() and pathauto_entity_state_delete()
* disabled pathauto_persist in the update hook
* fixed bug in hook_requirements()
Comment #109
ericras CreditAttribution: ericras commentedI'm testing this along side the Scanner Search and Replace module due to #1766076: Custom alias overwritten on node_save() with alias generated from Path Auto pattern Specifically, with work I'm doing at https://github.com/ericras/scanner to update that module.
Patch #103 worked, patch #108 does not and the unwanted behavior in OP occurs.(I am not, nor have been, using Pathauto Persist.)Edit:
I'm mistaken. The reason being is that patch 103/108 only solves the OP's problem for aliases created after the patch is applied. (I was testing 103 with aliases created after applying the patch, 108 with aliases created before.)
This seems like a problem as outlined in #33. The patch doesn't "fix" existing custom aliases, only ones created in the future. Any module with OP's behavior will still wipe out aliases made before this module is updated. I guess the solution would be to "prime" the new pathauto_state table?
Comment #110
mvc@ericras: Well, that's the problem we're trying to solve: pathauto isn't storing enough information to know whether or not a path was set manually, particularly in the case where it happens to currently have the same value it would have had if generated automatically. With this patch, we'll start collecting that information so we know exactly what the user intends. You can manually fill in pathauto_state for older nodes if you know what was intended for your particular site, but there's no way to know that in general right now.
I'm not sure this patch should try to guess the correct initial state for all users. We'd have to iterate over all entities in hook_update_N() and try to guess whether the current alias was set automatically, which would be extremely time consuming for large sites (equal to regenerating every single alias). The only other option would be to check each time a path is generated whether there's a corresponding record in the pathauto_state table, and try to guess the appropriate value then. But that would be unnecessary cruft for all aliases created after this patch lands.
ps: what's "OP"?
Comment #111
klonos"OP" = Original Post(er). It refers to what we in d.o call an issue summary ;)
Comment #112
ericras CreditAttribution: ericras commented@mvc: I should have read the entire thread closer, I see that bulk adding existing nodes was part of the patches up through #80 in accordance with @quicksketch's thoughts in #33.
Since bulk adding existing nodes to the new table was not done in Pathauto Persist, it probably should be its own issue after this one. Sorry for sidetracking.
The latest patch looks good for the issue at hand: merging pathauto_persist into pathauto.
Comment #113
acbramley CreditAttribution: acbramley commentedI believe this would also fix #1949728: Alias reverts to auto-generated when saving non-published revision (workbench moderation)
Comment #114
sylus CreditAttribution: sylus commentedThe latest dev of pathauto + pathauto-persist-936222-108.patch works great for me in mySQL but is causing problems for me with PostgreSQL. Confirmed by removing the patch and just trying a drush si without the patch and everything works.
The error I am getting is: PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer and the issue for this can be found here: http://drupal.org/node/1932612
Checking the call stack it looks like the new function pathauto_entity_load is not retrieving just numeric id's?
I have attached my stack trace with the error when running drush si (latest dev of pathauto + patch 108).
Comment #115
brianV CreditAttribution: brianV commented#114 is right - we can't count on entity ids to be numeric. Some, (actions, for example) are varchars.
Patch is updated, should 'just work'.
Comment #116
mvc#115 doesn't seem right to me; many other modules use int for the entity_id column (examples: feeds_item.entity_id, field_data_body.entity_id, field_data_comment_body.entity_id). if actions use non-numeric entity_id's, then the proposed core patch at #1932612-5: PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer will break that module.
if there's a core postgres bug here, it's not specific to this module. at most this patch should add a cast to int or a call to is_numeric() in hook_entity_load() as a temporary measure, with a comment pointing to the core bug, and a TODO comment saying to take out that workaround once the core issue is resolved.
Comment #117
Pancho@mvc:
I marked the issue you mentioned as a duplicate of #1823494: Field API assumes serial/integer entity IDs, but the entity system does not which tries to go just the other route in D8.
For now, brianV seems to be right and we can't count on entity IDs to be numeric, but I'm really curious which way we will go in D8 (which will then be backported to D7). It's going to be a flexibility vs. performance decision.
So atm we should IMHO take the #115 route, but can also postpone to the D8 core issue.
Comment #118
mvc*sigh* so core doesn't yet know if it wants entity IDs to be numeric? alright, but i personally propose accepting #115 and closing this 2.5 year old issue using #108. if and when core changes its mind new issues can be opened against this and the hundreds of other modules that use integer IDs. at that time this new update hook can be changed to alter the table after the call to db_rename_table() to convert the existing int column to whatever size varchar core decides to use. this patch has languished long enough and waiting on core architectural issues means it will never happen. but, it was my patch, so i'll ask someone else to mark this RTBC.
Comment #119
Fabianx CreditAttribution: Fabianx commentedI have waited for this for a long time.
Lets get this finally in!
Comment #120
PanchoIf my point had been that core might change its mind in D8, then it would certainly make no sense to wait, as this can be covered by a followup.
But I refered to the Core issue because it confirms that core in an inconsistent state now, as #114 and #115 say: some core subsystems are expecting numeric IDs, others aren't and actions even produces non-numeric IDs. And I refered to it to show that non-numeric IDs are not just an implementation bug but an area of architectural debate.
We can't be stricter about IDs than core is, so for now we need to cover both numeric and non-numeric IDs.
Fixing some indentation codestyle issues from #115 and providing an interdiff between #108 and #120.
Comment #121
Fabianx CreditAttribution: Fabianx commentedI agree with #120, its green => RTBC again
Comment #122
mvc@Pancho: alright, but then pathauto_update_7006() needs to alter the column type after renaming pathauto_persist.module's table, as i mentioned in #118. ideally there would also be a comment here referencing #1823494: Field API assumes serial/integer entity IDs, but the entity system does not to explain why we can't use INT.
Comment #123
PanchoI believe that merging-in pathauto_persist warrants creating a new 7.x-2.x branch. This would also help admins with both modules enabled understand that this is a major new version, and that they should take a closer look at what's changed. And it would help custom modules reading from the pathauto_persist table keep requiring the 1.x branch for some time.
I have quite some more major and minor nitpicks, until this really is RTBC.
But first of all, I'd like to ask you how you think about these records being "pathauto_locks" not "pathauto_states". 'State' is such a generic term that it doesn't give insight.
Indeed, these are really 'locks': We're keeping the path locked to a given or no alias vs. we're allowing it to be recreated automatically according to title (or other) updates.
If we call it 'pathauto_locks', the concept would be immediately understood from the table name or the code.
The only disadvantage is that we'd need to switch around the boolean:
state = 1 means 'recreate' and would correspond to lock = 0
state = 0 means 'lock' and would correspond to lock = 1.
Don't think that is too much of a problem, though.
As another alternative, we could call them "autopath"s, which might sound a bit funny, given that the module is called pathauto. But it seems okay that 'pathauto' allows a path to be an 'autopath' or not. In any case, it would be just as intuitive. And conceptually it would be just as clean as the 'locks'.
Comment #124
Pancho'autocreate' would be another naming option.
Comment #125
John Pitcairn CreditAttribution: John Pitcairn commentedpathauto_manual ? More descriptive and wouldn't require the boolean flip.
Comment #126
acbramley CreditAttribution: acbramley commentedmy $0.02, pathauto_locks is the most readable and understandable suggestion
Comment #127
Pancho@John: 'pathauto_manual' unfortunately needs the flip just as well. :(
Names that don't need a flip are: 'pathauto_autopath', 'pathauto_autocreate', 'pathauto_recreate' or 'pathauto_regenerate'. The last two of them, especially the last one, sound like good alternatives to 'pathauto_locks'.
Comment #128
quicksketchIf you're going to get into renaming the table you'll need to rename the existing API in the module. pathauto_state was recommended by DaveReid in #106, previously the table name was just "pathauto", which I thought was the best but honestly it didn't matter to me. However if we're talking about renaming the table *again* because of some other nitpicks, then maybe we should just put it back to "pathauto" and be done with it and we don't need to change other existing code. There's no need for a longer table name if it's the only table owned by pathauto anyway.
So... if we're going to change it again, make it just be "pathauto". Though in honesty, the table name makes no impact whatsoever, so don't bother changing it again at all.
Comment #129
mvcI believe that merging-in pathauto_persist warrants creating a new 7.x-2.x branch. This would also help admins with both modules enabled understand that this is a major new version, and that they should take a closer look at what's changed. And it would help custom modules reading from the pathauto_persist table keep requiring the 1.x branch for some time.
@Pancho: For what it's worth, I would be very surprised if any other module is using pathauto_persist's database table, as opposed to just letting pathauto handle alias creation. If Dave Reid wants to create a new branch here I have no objections but I don't think that particular argument applies in this case.
I have no preferences for the table name as I personally don't see it as very important.
Comment #130
mvcAlright, here's a fix for patch #120 which alters the database column type, available in two flavours: pathauto_state & pathauto_regenerate. I propose we let Dave Reid choose which he prefers.
edit: see #132 for the interdiff files.
Comment #131
DamienMcKennaI second quicksketch's comments that the table should be simply named 'pathauto'.
Comment #132
mvcoops, forgot the interdiff files, and i can't add them to my last comment. here you go.
again, i really have no strong preference regarding the table name, but for those with an opinion on this matter please note Dave Reid's comments in #106, in which he asked that the name be changed from pathauto to pathauto_state :)
Comment #133
rooby CreditAttribution: rooby commentedIs there possibly a chance that thetable pathauto might be needed in the future for something more fitting?
I think pathauto_state makes sense based on what the actual data in the table is (even though the column name is strangely pathauto, I would expect it to be state, or even pathauto_state).
Comment #134
quicksketchWhat's more fitting than the only data pathauto needs to store? The "what if" scenario hasn't been encountered in the 8 years of this module's existence, so I don't think it's worth worrying about. Even if it were renamed later, we can use an update hook to accomodate.
In any case, I don't feel strongly about leaving it as-is in the current patch. Let's not block this issue over naming. I'm just saying the IF we decide to rename it AGAIN, we should just stick with "pathauto". It was named "pathauto_state" at the maintainer's request, so let's just accept judgement and stick with what we have.
Comment #135
dsnopekThe pathauto-persist-936222-130-pathauto-state.patch from #130 works beautifully for me! What left to get this committed?
Comment #136
klonosYes pretty please! This has been going on for nearly 3 years now :/
...lets just commit the actual feature now and perhaps bikeshed on the trivial mater of the name of the table in a follow-up issue.
Comment #137
acbramley CreditAttribution: acbramley commentedMarked #1408576: [menupath-*] alias generation/update should be triggered when menu settings of a node or any of its parents are changed. as a dup as it will be fixed with persisting states.
Comment #138
steveoliver CreditAttribution: steveoliver commented+1 for RTBC. Nice work, all.
Comment #139
pwaterz CreditAttribution: pwaterz commentedI was running into this issue when save a panalized node https://drupal.org/node/1424066. Patch https://drupal.org/files/pathauto-persist-936222-interdiff-120-130-patha... worked great! Thanks for everyones hard work!!
Comment #140
bvanmeurs CreditAttribution: bvanmeurs commentedFor some reason this seems to take ages. I don't know what the problem is, but I would like to post an alternative patch for now that doesn't require the creation of a new database table, which will cause problems when trying to update this module. Maybe the maintainer will also be more willing to commit my patch more quickly because basically it uses the same method to determine whether to use automatic path creation when saving a node in the same way as is done when building up a node form.
Comment #142
bvanmeurs CreditAttribution: bvanmeurs commentedComment #143
bvanmeurs CreditAttribution: bvanmeurs commentedNote that this solution can also be applied in a custom module as a work around:
Comment #144
bvanmeurs CreditAttribution: bvanmeurs commentedComment #145
JamesOakleyHi bvanmeurs
I'll put this back to RTBC, because the patch in #130 has been tested by several people and found both to work and to be what's needed.
I'm sure the maintainers would be grateful to look at your alternative implementation. However to propose a brand new solution and to set the status back to "needs review" will make committing this take even longer - the new patch would need several members of the community to review it before we can back at the RTBC stage.
That said, like you, I would love to know why this is so stuck at RTBC. Are the maintainers waiting for something else before committing the change? If so, it would be good to know what so the rest of us can help. If not, it would be great to get this fixed.
Comment #146
bvanmeurs CreditAttribution: bvanmeurs commentedAgreed James, I just had to put it on 'needs review' temporarily for my patch to be tested.
The original patch posted here (with the new db table) is still the best solution. But I think my workaround can help others especially because it is taking so long for the patch to be committed.
Comment #147
dsnopek@bvanmeurs: I agree completely with @JamesOakley - the patch in #130 has been tested extensively by many people, including on production systems. It's even included in Panopoly, OpenAtrium and all other Panopoly-based distributions. So all the users of those distributions have been testing this patch for a while (even if they didn't know it).
Since so many people are already using that patch, it be great to get it committed upstream!
Comment #148
klonos3rd birthday of this issue coming up in a week :(
Please commit.
Comment #149
bvanmeurs CreditAttribution: bvanmeurs commentedIt seems that none of the maintainers is aware of this issue, so I've sent the following message to greggles (who seems to have been comitting patches most recently):
Comment #150
greggles@bvanmeurs this issue is marked as "needs tests." Do you think it doesn't need tests? Your patch doesn't seem to include any.
@all the work in #130 does seem to have lots of tests, though I'm not sure that it has the right ones ;) I don't maintain this module any more so I won't comment on the various issues here. I do notice that in the many comments complaining about how long it has taken to get this committed nobody has actually done a full patch review. I suggest 1 or 2 people spending the time on a real patch review as the best way to move it forward.
Removing needs tests because the consensus solution in #130 seems to have them.
Comment #151
mvcActually removing 'needs tests' tag because the patch in #130 has them.
@greggles dave reid posted a comprehensive patch review in #106 (i believe my patch in #130 addresses each of the points he raised).
Comment #152
gregglesThanks, mvc.
It's great that 130 addresses that review. More reviews (as long as they don't bikeshed) are always helpful to finish off a patch.
Comment #153
dsnopekSince there are actually 2 patches in #130, I just wanted to point out that it's the "state" patch that's being used in Panopoly, OpenAtrium, and the like. That's the one I'd personally recommend get commited.
I just read through the patch again and nothing jumps out at me. Like I mentioned, I've been using this in production for some time and everything has worked fine including the upgrade path. Also, the test case looks pretty complete, so the fact that it's passing is great! However, it is missing a test for deleting a node. The code for
pathauto_entity_state_delete
looks correct and has worked in manual testing - not sure if it's necessary to test it?That's my review! I hope that helps. :-)
Comment #154
deanflory CreditAttribution: deanflory commentedI'm guessing from these WSOD errors after applying #130 "state" and attempting to run update.php, that I need to uninstall it first, then upload the patched version and then enable:
I'll report back here if I can't get this resolved.
Comment #155
pwaterz CreditAttribution: pwaterz commentedAll you have to do is apply the patch and run update.php.
Comment #156
dsnopekYeah, last time I upgraded I used 'drush updatedb' and didn't have problems. I don't see why pathauto_entity_state_load_multiple() would get called during update.php. But it's worth someone else testing one more time with update.php and making sure it works, though. Unfortunately, I won't have time to try it this weekend..
Comment #157
deanflory CreditAttribution: deanflory commentedDisabling, uninstalling, uploading patched version, then enabling got me around the errors reported in #154.
pwaterz, I was attempting to run update.php when I encountered those WSOD errors.
Comment #158
mattltThe -pathauto-state patch and update worked fine for me. I did not previously have the pathauto_persist module installed.
Comment #159
Danny_Joris CreditAttribution: Danny_Joris commentedI applied the pathauto-state patch in #130 because I'm hoping this issue might solve the one I'm having in #1949728: Alias reverts to auto-generated when saving non-published revision (workbench moderation).
This patch solves the problem where editing a node with custom path has it's 'Generate automatic URL alias' state enabled again. However, in my use case the one-liner patch in the mentioned queue (#7) has the exact outcome as this one. It solves the 'automatic URL state' problem, but when saving it to draft, the alias is also saved to the published version of the node. Maybe this isn't a pathauto issue, but the fact that I'm having the same outcome as the one-liner patch, I find interesting.
Edit: same result with non-patched version of pathauto in combination with pathauto_persist
Comment #159.0
Danny_Joris CreditAttribution: Danny_Joris commentedAdded XREF to pathauto_persist
Comment #160
slv_ CreditAttribution: slv_ commented130: pathauto-persist-936222-130-pathauto-state.patch queued for re-testing.
Comment #161
aalamaki CreditAttribution: aalamaki commentedI wonder why the schema name switched to pathauto_state in #115? I second quicksketch on that it should be simply "pathauto". The patch at #130 and earlier will generate issues in drush updb for anyone that has applied any of the earlier patches as there is no update path to move data from the pathauto table to the new pathauto_state. The latest update hook 7006 will just check for existence of the pathauto_persists table and moves data from it to the pathauto_state table, lack of the update path will result in data loss in this potential case.
Discovered the same issue when updating Panopoly to the latest version, their older rc3 version has an older patch applied but the latest rc5 version applies the patch at #130, the issue has been discussed in https://drupal.org/comment/8284361#comment-8284361
Comment #162
gauravvdeshpande CreditAttribution: gauravvdeshpande commentedI have faced similar issue while updating the panoply latest version.
Now another issue is "pathauto_state" table (and or db table rename) is not getting generated even after updating the pathauto_update_7006 hook and invoked couple of times.
Comment #164
mvcRemoving 'needs tests' tag again because the patch in #130 has them.
Comment #165
Dave ReidWanted to give everyone a quick update on status of committing this patch. I plan on releasing a maintenance release this week, then committing this after that release to 7.x-1.x-dev so that it can be QA'd a little bit more out in the wild.
Comment #166
klonosCan't wait for that dev! - I've been patching my installations for 3 years now with each update.
Comment #167
dsnopek@Dave Reid: That's amazing news, thanks! Of all the patches we use in Panopoly, this one is the one I'd most love to see get committed upstream (and the one I worry the most about it maybe NOT getting committed...)
Comment #168
klonos@Dave Reid: I hate to be the one to point it, but that week has passed. Just a friendly ping :)
Are there any withstanding issues preventing you from releasing a 7.x-1.3 (and the dev after that one)?
Comment #169
klonos...and just to be clear once again: it's the "state" version of the patch at #130 that's RTBC.
So, lets make it easier for people coming to this issue and those that are trying to test/review figure out which patch is the right one by hiding the other ones.
PS: also moving related issues to the dedicated metadata section.
Comment #170
klonosComment #171
klonos...sorry: #2132215: Validation for not allowing adding an issue relation to itself.
Comment #172
barraponto CreditAttribution: barraponto commentedEchoing @aalamaki in #1979558-22: Update pathauto patch: Why is entity_id a varchar? Isn't signed int enough? EDIT: Answer lies in comments #118 and #120.
Comment #174
barraponto CreditAttribution: barraponto commentedIt's the patch #130 (state version) that is RTBC.
Comment #175
jenlamptonI tested the patch in #130 and it did not work for me, entirely. I can confirm that the checkboxes stay checked and un-checked as expected (YAY) but the bulk update operations still don't work. :/
If I check three nodes I would like updated and use the "Update URL Alias" feature from the admin/content page, I get a happy green message that says
Updated URL aliases for 3 nodes.
but in fact, no aliases were updated.If it was intended that the aliases *not* be updated (because the checkboxes were "not checked" on those nodes after the update - indicating that their aliases were supposed to be manual) then the message should *not* say that the aliases were updated.
But if the aliases were supposed to be updated then they should have been updated to match the new pattern.
Comment #176
jenlamptonOn further investigation, I think the bulk update confusion may not be related to this patch. I'm going to set this back to RTBC because I have created another issue to fix the confusing bulk-update messaging: https://drupal.org/node/2235557
If you think that this patch should insert records for each node in the new database table to indicate the state of the "Generate automatic URL alias" checkbox then feel free to set back to needs work.
Comment #177
Clemens Sahs CreditAttribution: Clemens Sahs commented#130 fix this issue fine.
The only persisted problem is if your are updating this module. Because the update script don't fill the ```state``` table, so it will be empty. Here is a quick sql query that select the url_aliase and push them in to the pathauto_state table.
Comment #178
sammarks15 CreditAttribution: sammarks15 commentedI've just run into an issue with this patch that's causing warnings to be thrown when enabling or disabling menu links. Here's the warning:
Warning: Illegal string offset 'pathauto' in pathauto_entity_load() (line 358 of /.../pathauto/pathauto.module).
I've discovered that sometimes an entity doesn't have the
path
property which causes this warning. I've attached a patch (based on #130) that sets thepath
property to an empty array if it doesn't already exists and then continues to update thepathauto
key.After updating that code the warnings disappeared.
Comment #179
caspervoogt CreditAttribution: caspervoogt commentedI'm using Panopoly and was unaware that the patch from #130 was included. I was having an issue when customizing pages with the in-place editor, where it would reset the page's alias automatically afterwards. I tested the patch from #178 and that fixed it for me. Good work, Samuel!
Comment #180
dsnopek@plethoradesign: I've created an issue in the Panopoly queue for this #2239847: Update "persist" patch to pathauto
Could you please post the steps to create this problem on that issue? That'd definitely help with getting it committed quicker. :-) Thanks!
Comment #181
Clemens Sahs CreditAttribution: Clemens Sahs commented@sammarks15
can you write a interdiff?
Comment #182
sammarks15 CreditAttribution: sammarks15 commentedOops, sorry about that. I've attached the interdiff.
Comment #183
mvc@sammarks15: thanks for the interdiff. your patch works for me, and it seems like a safe test.
Comment #184
edxxu CreditAttribution: edxxu at INsReady commentedThe patch from #178 works for me. Thanks @sammarks15.
Comment #185
BarisW CreditAttribution: BarisW commentedWhen trying the patch, I cannot run the database updates that adds the pathauto_state table, as pathauto_entity_state_load_multiple() is called before the update has run. That function runs a query against the pathauto_state table, even when it doesn't exist yet.
So the update that adds the table crashes because the table does not exist :(
Comment #186
dsnopek@BarisW We've run into this in Panopoly, but worked around it by specifying
hook_update_dependencies()
to make sure hooks run in the right order. However, I think this is a losing battle. :-/ The pathauto patch should probably check for existance of the table before trying to query from it inpathauth_entity_state_load_multiple()
.Comment #187
sammarks15 CreditAttribution: sammarks15 commentedAnd what happens if the test to see if the table exists fails in
pathauto_entity_state_load_multiple()
? Should it fail silently somehow? I would suggest attempting to create the table at that point, but that seems like it'd be against Drupal best practices.@dsnopek, can you attach a patch of the changes you made with using
hook_update_dependencies()
?Comment #188
dsnopekI think "do nothing silently" is the only possible option.
We've had to make constant additions/changes to
hook_update_dependencies()
which are really Panopoly-specific. Basically, we say: "random update function that does entity_load() depends on pathauto_update_7006()" every time it comes up. The problem is that adding even an unrelated contrib module can change the order that the hook_update_N() function run and then we're back into trouble.Here is one of many examples:
http://drupalcode.org/project/panopoly_widgets.git/blob/refs/heads/7.x-1...
Note: we actually use panopoly_core_update_7002() as a proxy for pathauth_update_7006() because it depends on it (and will force it to run in certain conditions to handle a different issue).
Comment #189
sammarks15 CreditAttribution: sammarks15 commentedWhat if we implemented logic that said if the table doesn't exist, then it just returns a default value? For example, if
entity_load
was called and the table doesn't exist, we could just set some default values (ideally in line with the pathauto defaults) and then send a warning message todrupal_set_message
telling the user that the table doesn't exist and that a default value was provided.It seems to me that failing silently wouldn't be the best idea because it would result in issues later on down the line (for example, if someone wanted to set some pathauto state properties and the function failed silently, then we would get at least PHP warnings saying the property doesn't exist on the entity).
Comment #190
dsnopekSure, I think a default value makes sense.
Comment #191
quicksketchWe might also try using a try/catch statement, that way there's no performance cost to check the table after the update has been run. To prevent silent problems, I'd suggest a watchdog() and drupal_set_message() in the catch{}, but only if MAINTENANCE_MODE is not defined. That way the user won't get errors if they update the module, then visit update.php.
Comment #192
dsnopek@quicksketch: That sounds brilliant! I really like the try/catch vs checking if the table exists.
Comment #193
sammarks15 CreditAttribution: sammarks15 commentedI've uploaded the updated patch and interdiff to include the try/catch when querying the database. If the table doesn't exit, it returns a default of false. I thought false made the most sense because if something already had a custom path, that path wouldn't be overridden.
I also added a conditional so that if the site was not in maintenance mode, a warning would be sent to
drupal_set_message
andwatchdog
.Comment #195
sammarks15 CreditAttribution: sammarks15 commentedI guess my PHPStorm-generated patch doesn't work well with Drupal's CI tests. This one is generated by git instead.
Comment #196
sammarks15 CreditAttribution: sammarks15 commentedUpdating status and hiding old files. The interdiff in #193 still applies to the patch in #195.
Comment #197
dsnopek@sammarks15: Thanks for the new patch! In the next couple days, I hope to do some testing with Panopoly. :-)
Comment #198
dsnopekI just tested the new patch in #196, using an older revision of Panopoly which I knew had upgrade issues. The new patch worked perfectly! I also quickly did some manual testing to make sure that the patch still provides the functionality it's supposed to, and it does. Marking this RTBC! :-)
Thanks so much, @sammarks15!
Comment #199
Pere OrgaI would like to see this fixed too.
Just in case it's useful to others, I'm addressing this issue with Pathauto Enforce module instead.
Comment #200
hefox CreditAttribution: hefox commentedUsing patch from -130
So this situation encountered
1) Disabled editing of alias on node form (via code) for published nodes
2) accidentatly set up a bunch of nodes with wrong alias (via migration) and have them set to not use pathauto
Now, try and update those to use alias. Delete the aliases or use the action + views bulk operation -- can't.
Would need to remove 1) and then manually edit each to fix it.
Should this function
1) change it so action can configure whether to update those that have pathauto?
2) add a new action to purge/change pathauto state?
This is likely a feature request for after the patch lands, cause it's been a while in works!
Comment #201
RoloDMonkey CreditAttribution: RoloDMonkey commentedhefox,
If I understand, what you are describing sounds like it should be in a separate issue.
Comment #202
Fabianx CreditAttribution: Fabianx commentedRTBC + 1
Comment #203
hefox CreditAttribution: hefox commentedJust to add a reason why it might need to go with this patch: prepatch could delete alias (pathautoed or not) then regenerate all them. Now whatever you do via bulk operations don't take effect till manually edit each one to say to use pathauto.
Comment #204
DamienMcKennaFYI I added a note to the README.txt file for Panelizer as it was a common co-conspirator in this bug: #2295091: Document the problem with Pathauto loosing URL aliases
Comment #205
Dave ReidI don't agree with changing the entity_id column to a varchar. We should implement the same fix as #2047811: Make compatible with non-integer entity IDs. Unless there is a *valid* use-case for supporting pathauto state for enttities with non-numeric IDs.
Comment #206
DamienMcKennaI suggest including this in the 1.3 release.
Comment #207
DamienMcKennaOut of interest, are there any minor tweaks that have been made to the patch over the past *cough* four years relevant to the Pathauto Persist module (that the patch is based upon) which haven't already been added to it?
Comment #208
RoloDMonkey CreditAttribution: RoloDMonkey at FFW commentedHere is a new patch, per Dave's request in #205.
Comment #209
RoloDMonkey CreditAttribution: RoloDMonkey at FFW commentedOops, I missed the change in #2047811-6: Make compatible with non-integer entity IDs. Uploading the new files. Hiding the old ones.
Comment #210
DrCord CreditAttribution: DrCord commentedthis latest patch from #209 worked perfectly with the latest pathauto (7.x-1.2+21-dev) and Drupal 7.31.
Comment #211
poniesI had some whitespace issues applying #209 to 7.x-1.2+21-dev. It may just be my environment but I stripped them out anyway.
Comment #212
m.stentaSmall nit-pick (but necessary for Drupal coding standards):
Spaces should be used for indentation instead of tabs (https://www.drupal.org/coding-standards#indenting).
(Sorry for not submitting a new patch myself... not in front of my dev environment at the moment...)
Comment #213
poniesHere it is again, this time with fewer tabs.
Comment #214
m.stentaComment #215
JeebsUK CreditAttribution: JeebsUK commentedCan I just ask for clarification where this latest patch should be tested against? is it latest dev or 7.x-1.2? Looking to help out with this if possible because we're now suffering from it as well in a combination with Workbench and Workbench Moderation.
Comment #216
RoloDMonkey CreditAttribution: RoloDMonkey at FFW commentedPatches are almost always done against the dev branch.
Comment #217
acbramley CreditAttribution: acbramley commentedIt may be worth adding extra test cases to the description of the bug so this is easier to rapidly test, would be great to get this in soon. I am happy to do this given the authority :)
Comment #218
acbramley CreditAttribution: acbramley commentedThe following steps passed manual testing after applying the patch to 7.x-1.x:
1) Create node with "Generate automatic url alias" unticked, and "mycustomalias" in the URL Alias field
2) Implement a hook_init() with the following code inside:
3) Reload the node page
Outcome:
The title is correctly updated and the node alias remains the same.
Comment #219
RoloDMonkey CreditAttribution: RoloDMonkey at FFW commentedSorry about the whitespace issues with my patch. I was working on a new computer, and I didn't have my IDE completely set up yet.
Comment #220
pbuyle CreditAttribution: pbuyle commentedPatch in #213 (applied release 1.2) solved the issue in my case.
Comment #221
mvctwo positive reviews; changing status
Comment #222
JeebsUK CreditAttribution: JeebsUK commentedIt solved our issues too so another +1 for RTBC.
Comment #223
dustinleblancChiming in on another success for #213
Comment #224
glass.dimly CreditAttribution: glass.dimly commentedPatched with #213 and "generate automatic alias" no longer erroneously becomes checked again after saving the node with it unchecked.
However, I still have the problem from this thread that the old alias is preferred over the new, manually entered alias. Confirmed with both update actions, "Do nothing. Leave the old alias intact." and "Create a new alias. Leave the existing alias functioning." Also, strangely enough, with "Create a new alias. Delete the old alias." The old alias doesn't get deleted.
This is a bug reported on this thread and was previously marked a duplicate of this thread, but I've unmarked it as a duplicate of this thread. My reasoning and more details are in that thread, here.
I think #213 should be committed and work should continue on the other thread for a fix to that separate issue.
Comment #225
jrabeemer CreditAttribution: jrabeemer commentedPatch #213 works for me. Ship it!
Comment #226
Leeteq CreditAttribution: Leeteq commentedHow about committing #213 and then set back to add some tests?
Would be great to have it in -dev meanwhile.
Comment #227
loziju CreditAttribution: loziju commented@dave reid, @quicksketch, or anyone, has #48 been addressed? I tried the patch in #213 and #48 seems not addressed yet. I'm changing the status to needs work.
How to reproduce the issue:
1. Uncheck pathauto and add alias for a particular node / term
2. Bulk delete aliases and check that the custom alias for the node / term is deleted
3. Bulk generate aliases and check that pathauto is still unchecked and no aliases are generated (this is with patch #213)
I believe when we bulk delete aliases we should check if the state is 0, we don't delete the aliases. Is this correct? If yes, I'll see what I can do when I have some spare time.
Comment #228
pbuyle CreditAttribution: pbuyle commentedIMHO, #48 should be addressed in a separated, follow-up, issue and should not prevent completion of this issue. Just like #224.
Comment #229
Leeteq CreditAttribution: Leeteq commented#228: agreed.
Separate issue created here:
#2340217: bulk delete aliases should not delete aliases where the state is 0
Comment #230
swirt#213 works for me with workbench_moderation and no previous instance of pathauto_persist.
+1 RTBC
Comment #231
jstoller#213 appears to solve all my problems. +1 RTBC.
Comment #232
fictionindustries CreditAttribution: fictionindustries commented#213 Works like a charm
Comment #233
scott.whittaker CreditAttribution: scott.whittaker commentedForgive me if this is expected behaviour or a different issue altogether, but I've applied patch #213 and if I use the "Update node alias" feature of Views Bulk Operations on a node selection that includes a node with a custom alias, that custom alias is reverted to an automatic alias.
At least it does on custom aliases that were created before the patch was applied. I found that it does retain the custom alias if I create the custom alias after applying the patch and then do a VBO alias update.
This is what @hefox was referring to in comment #203. Is there a way to prime the table which marks custom aliases? I tried VBO's "Save content" action, but that also replaced the manual alias with a default one.
Comment #234
bibishani CreditAttribution: bibishani commentedPatch #233 works great, but I have conflict with other update hook, so this is a patch without the update hook
Comment #235
pinoniq CreditAttribution: pinoniq commentedThese patches seem a little large to me.
As far as I could determine, the problem occured when a node_save is triggered from outside the node/Edit. Thus the $node->path variable is null.
Simply adding a check for that should be enough. (See patch attached) This solved it for me
Comment #237
JamesOakley@pinoniq - Given how long it's taken to get this to RTBC, this probably isn't the time to propose a completely different approach and to start all over again. Is there anything specific wrong with the patch at #213 (other than being "a little large")?
Moving back to RTBC, as we're only back at Needs Work because the patch at #235 failed testing.
Question for the maintainers: Given this has been at RTBC for a couple of months, are you waiting for something specific before committing this?
Comment #238
pinoniq CreditAttribution: pinoniq commented@JamesOakley No idea, I didn't even test it for it seemed way to large. It also adds an extra table, extra db queries where a simple if should be enough. But maybe I missed some use-cases?
Comment #239
JamesOakleyI don't know if you read the issue history so far (there's a lot of it, I grant) - it goes back 4+ years.
You'll see a patch of similar length to yours in #4. Since then, it's got longer because it's had to deal with cases that weren't otherwise being handled correctly, and there's also the need to make sure there are proper automated tests defined for a module this popular.
Given lots of other people have tested this one nearly to death, I'd stick with the patch in #213 if I were you, and then come back here to report if you find it's not working properly.
Comment #240
dsnopek@pinoniq: It is necessary to have the extra DB table to tell the difference between a path created automatically or one manually created by the user.
We have been using several iterations of this patch in Panopoly for several years now (currently on #195 - need to find time to test the more recent ones) and the previous approach has been working great! If you're curious about the use case, please feel free to look at the Behat tests for Panopoly as they need this patch to succeed:
http://cgit.drupalcode.org/panopoly_test/tree/tests/features/pathauto_ad...
http://cgit.drupalcode.org/panopoly_test/tree/tests/features/pathauto_ed...
I just wish we could get this finally committed! @Dave Reid: Can you please let us know what is holding this patch back?
Comment #241
firfin CreditAttribution: firfin commentedJust tested from a Drupal which had pathauto persistant state module (v1.3) installed. Nobody mentioned this works fine also. But it does.
Time to commit and close this issue?
Comment #242
mrmikedewolf CreditAttribution: mrmikedewolf commentedPatch #213 is great. Many thanks to ponies. I've renamed it to follow drupal conventions. If this is committed, please credit the original author.
Comment #243
RyanPrice CreditAttribution: RyanPrice commentedApplied patch #242 to our environment and it solved the issue. Let's get this committed and closed.
Comment #244
klonosAll I want for Christmas is this to get in. I'll settle for New Year, but please please Santa!
Comment #245
Aron Novak+1 , we use this patch in production environments without any issues to solve this bug
Comment #246
Leeteq CreditAttribution: Leeteq commentedHappy 2015. I know Santa does not exist, but there are strong indications that his helpers do...
Comment #247
s.perilhou CreditAttribution: s.perilhou commentedHappy 2015!
I am currently testing #242 path and have a problem :
My site has thousands of URLs, some automatically generated, some not.
After the patch, if I run the code of the original post for an automatically-generated-url node, no problem. But for a non-automatically-generated-url node, it becames automatically generated and changes the URL.
As there is no information in pathauto_state table, pathauto considers that "automatically" is default.
This problem may apply to all production sites that use this patch.
Is there not a way in the 7006 patch, in the case of pathauto_persist does not exist, to generate entries in the pathauto_state table?
Comment #248
JamesOakley@s.perilhou
Did you run update.php after applying the patch?
The problem you're describing is exactly the problem that this patch is designed to fix. If it doesn't fix it at all, I'd expect someone else to have reported that by now, but instead we have lots of people saying it does work.
If you did run update.php, then maybe there's some other factor with your site that is preventing this from working as it should.
Comment #249
s.perilhou CreditAttribution: s.perilhou commented@JamesOakley Hi James
Sure I ran it :) The patch itselft is really great (it save my life !).
As I mention, my comment is for users (like me) who did not use pathauto_persist module, so there no information from the module table. As you see in #242 patch, there is nothing more than a create table in this case (end of 7006 update).
A proper test case (I guess) would be :
1/ Install Drupal
2/ Install Pathauto 7.x-1.2 (current stable)
3/ Make node with non-automatically-generated path
4/ Apply patch #242
5/ Test code from orignal post
In my specific case, I will make a script to populate pathauto_state table, but I wanted to escalate the issue.
Comment #250
JamesOakleyThen something isn't right. This patch is intended to bring all of the functionality from pathauto_persist into the pathauto module, making the former redundant. It shouldn't hinge on already having installed pathauto_persist for the patch to work.
Comment #251
alcroito CreditAttribution: alcroito commentedThe problem is that the patch does not contain a hook_update() that would populate the table with rows for all existing path aliases in the DB. A hook_update should be written that would somehow figure out which path aliases were generated, and which were manual, and put all that into the new table. At least that's what I think the issue is.
Comment #252
jstollerAnd with what data should the table be filled? It seems to me that Pathauto doesn't have any reliable way of determining which aliases should be automatically generated and which shouldn't. Isn't that the whole point of this patch in the first place?
Comment #253
s.perilhou CreditAttribution: s.perilhou commentedSure.
I made a little script this morning: for each node and taxonomy term, it compare current url and generated url to fill pathauto_state. But for me it's a dirty way...
Comment #254
alcroito CreditAttribution: alcroito commented@jstoller That's what I think as well, that there is no way to generate it in a proper way.
But it seems that people will lose their custom aliases for existing entities.
Comment #255
wOOge CreditAttribution: wOOge commentedPatch in #242 does *not* solve the OP issue.
1) Create node, save node.
2) Edit node, *deselect* auto generate path alias
3) Enter a custom alias "members/resources"
4) Save node (custom alias works)
5) Edit node again (changing nothing)
6) Save node again (custom alias is deleted)
This module should provide an auto path for all nodes that are not set to custom paths. If auto generate = 0 (not NULL), then custom paths should be left alone.
A possible (and brutal) workaround would be to add a custom field to the node, and then use pathauto auto generate to use the value from that custom field to "auto" generate the path using the manual entry... but then if left empty PA wouldn't have anything to work with.
EDIT:
I just tested on a vanilla Drupal with only pathauto and token installed and it works fine. There must be a conflict somewhere in my list of installed modules (groan) - but perhaps the conflicting module is common to us all?
Comment #256
kreatIL CreditAttribution: kreatIL commentedI guess the conflict comes from publishcontent. When I publish a node with custom path from within it's node edit form, the path remains untouched. But if I use the publish tab which is provided by the publishcontent module, the path gets overwritten by an auto generated version.
Comment #257
wOOge CreditAttribution: wOOge commentedFound (my) conflict:
I had a RULE that reacted on "Updating Existing Node" — but for some reason the "type" of node was not set. I deleted the offending event, and re added it, selecting the correct content type.
Strangely the rule didn't even do anything with paths... but perhaps the fault of a missing content type on the event was enough to throw off the path system on save.
Comment #258
MiroslavBanov CreditAttribution: MiroslavBanov commentedThis doesn't sound very promising. Ifpublishcontent
orrules
can create problems with this patch, then what aboutvbo
,workflow
,workbench moderation
,scheduler
?Edit: This was rebuted in following comments.
Comment #259
klonos@MiroslavBanov: We should initially focus on making sure that this feature work as expected in vanilla Drupal (+pathauto of course). The way this currently works is wrong and causes issues. That's why this issue here is categorized as a bug (and a major one in fact) rather than a feature request.
I'd like to present some facts here: pathauto is installed in ~60% of Drupal sites out there while publishcontent counts only ~1,25%. Now I am sure that you'll agree that fixing a major bug effecting 60% of Drupal sites is more important than the small percentage that publishcontent holds. Rules on the other hand might count a considerable ~24%, but from what I can tell from wOOge's comments above, it turned out to be a misconfigured rule and not a conflict after all.
As for the rest of the modules you mention, do you have any actual use cases that cause specific issues when applying this patch? If so, I would go ahead and file issues against their queues, warning about the imminent commit of this patch. If not, then you do realize that what you do is spread FUD in an issue that has been active since 2010 and is finally closing to being solved.
Bottom line is that we simply cannot account for every possible side effect with every other contrib module out there and every possible combination of these modules on top of that. Fair enough, the developers of these modules took for granted the way pathauto has worked for years. So, what we can do if this is deemed to be a major architectural change is to perhaps get this in a 2.x version of pathauto instead of the 1.x. This will help in communicating the fact that something major has changed and other module maintainers will simply have to take action in order to support it. End users will be warned and take caution too (as they do with evry other module releasing a new major version).
I hope all this makes sense.
Comment #260
MiroslavBanov CreditAttribution: MiroslavBanov commented@klonos: Sorry I did not mean to make a huge deal out of this. But I will explain:
I thought that if publishcontent is causing problems, than any module that is doing something with a node outside of a node edit form could cause problems.
I am not focused on the percentage use at all. I think that publishcontent is an innocuous little module. But maybe I'm wrong?
You are correct. I don't have evidence that other modules cause problems with this patch, so I am not going to argue.
Comment #261
kreatIL CreditAttribution: kreatIL commentedI've once again tested the publishcontent module against the patched (#242) version of pathauto 7.x-1.2+23-dev, which IS now working for me. I'm really sorry for the confusion.
Comment #262
dsnopekVarious versions of his patch have been in use in Panopoly and Open Atrium for several years now. On Panopoly and and Open Atrium projects, I have seen Rules, VBO, Workflow and Workflow Moderation used successfully, without additional problems relating to the functionality implemented here. I don't think there is any incompatibility with any of those modules (I can't speak to publishcontent).
Comment #263
MiroslavBanov CreditAttribution: MiroslavBanov commentedLatest versions of patch actually have no problem with publishcontent. See #261. I'm not really concerned with Rules module, as I have seen it misbehave in the past.
So, hurray, and thanks to the previous commenters for sorting this out.
Comment #264
dkendrickmbll CreditAttribution: dkendrickmbll commentedThis patch resolved our issue of the disabling of the Generate automatic URL alias and entering a manual URL alias not saving. Thanks for this!
Comment #265
jstoller@Dave Reid, do you see any issues stopping this from being committed now? Pretty please. :-)
Comment #266
webservant316 CreditAttribution: webservant316 commentedyes, please commit.
Comment #267
Steven Jones CreditAttribution: Steven Jones commentedSo I wonder if this would introduce a performance regression, because for every single entity load, a DB query will be performed. On some pages, where you might have a single node with a lot of field collections fields etc. and you render it, this might result in many, many DB queries for the field collections which are only necessary if field collection provided some pathauto integration (it doesn't afaik).
I don't think there would be a trivial way to solve this without introducing an additional API for pathauto that would require modules to declare what entity types they provide pathauto integration for, also I don't know if this is just because we tend to build sites where pages are made up of lots of field collections.
(Our site has pathauto_persist DB queries now accounting for 2.8% of total DB query time for the average node_view page, when it only really needs to get this data for a single node on the page)
Comment #268
Fabianx CreditAttribution: Fabianx commentedWe used db query cache by chx (https://www.drupal.org/node/2250301) to push those key-value queries to memcache.
But a getMultiple() or caching layer could be added here also directly.
I don't think however that performance should block correctness here ...
Comment #269
MiroslavBanov CreditAttribution: MiroslavBanov commentedMaybe some way can be implemented to define what entity types are being considered by pathauto, and this can be part of the entity_info for example, or some other cacheable and alterable way. But this will only help to make sure that field_collections, comments, fieldable_panels_panes, beans, and the like will not query the database needlessly. Anyway this is probably better to implement in a separate issue?
Comment #270
Steven Jones CreditAttribution: Steven Jones commentedI'd be happy to see this patch get in, and then a fix for the performance in a follow up.
Comment #271
ladybug_3777 CreditAttribution: ladybug_3777 commentedI noticed that with the newest stable release of pathauto (1.2), I no longer needed the pathauto_persist module. In fact it was acting very strange with both enabled.
I did not read through ALL the comments on this thread so perhaps this was already stated... but I found that I had to uninstall pathauto persist.
Comment #272
ladybug_3777 CreditAttribution: ladybug_3777 commentedJust stopping back to update with some findings of mine. I recently discovered that a new release of Workbench Moderation has modified a setting for pathauto. See this post: https://www.drupal.org/node/2308095 for the details.
This modification in WB Moderation lead me to believe that suddenly pathauto no longer needed pathauto persistent state, which is why I left the comment I did above. I wanted to leave this information in case anyone else is having a similar issue. Anyone that has upgraded workbench moderation to version 1.4 may find their way here in confusion as I did a few days ago.
Comment #273
alauzon CreditAttribution: alauzon commentedI have applied patch #242 and it is working wonders but for all but one use case.
In a multilingual context one could have nodes where automatic URLs are desired in some languages and not in other languages?
The solution for that use could be to add one column in the table when there are multiple languages and use that information where applicable? That way the info about is the URL translated or not would be for each language in use in the site.
Comment #274
jstoller@alauzon, re #273: I suggest creating a new issue for enhanced multilingual support, since that really is a new feature and goes beyond the existing pathauto_persist module functionality that is being merged in here. Also, somewhat selfishly, I would hate to see what momentum we have here stalled, given people have been waiting four and a half years for this to land and we've had a very well tested patch at RTBC for the last 4 months.
Comment #275
JeebsUK CreditAttribution: JeebsUK commentedI would have to concur with the above #274. Please can we take the multilingual scenario into a new separate issue.
Comment #276
ladybug_3777 CreditAttribution: ladybug_3777 commentedI agree, this patch is ready, let's not complicate it with the multilingual issue.
Comment #277
joelpittetOne thing I noticed while using this is that entity_load get's called a lot. For example. I have ~40 items in my cart on a commerce site.
pathauto_entity_state_load_multiple
get's called and does an unnecessary SQL query for each of these entities.This on the front page:
It's not the SQL Query that is of concern it's number of DB calls start to get large.
One suggestion, but may not be the best, is to do one call for all the
entity_type
s in{pathauto_state}
. Then a simple check before making the SQL other SQL calls if that entity type exists in the table?Also the coding standards on that patch need some love, indents are all over the place, likely don't need the try/catch block around the db_query in that function.
Mostly everything else looks great.
May want to do this check first if someone had this other module installed
table does not exist, so a default value was provided. Please make sure to run
update.php
';+ drupal_set_message($message, 'warning');
+ watchdog('pathauto', $message, array(), WATCHDOG_WARNING);
+ }
+ $result = array();
+ foreach ($entity_ids as $id) {
+ $result[$id] = FALSE;
+ }
+
+ return $result;
+ }
Coding standards.
Comment #278
joelpittetHere are the statics for performance added cleanup mentioned. I didn't do the install one because it's just an observation that i'm not 100% on.
Comment #280
joelpittetWell that's quick turn around on the tests:S. I wonder what use-case this broke... This patch save 60+ unnecessary SQL queries on my front page.
Comment #281
joelpittetAdding fast static pattern so that the types static can be reset. Not quite sure if I cleared it in the right place... testbot will tell.
Comment #282
joelpittetSweet, Joël 1, testbot 0!
Comment #283
joelpittetThat works but it clears the list too often, I found a better place for the static cache reset. Hope testbot agrees.
Comment #284
Fabianx CreditAttribution: Fabianx commentedJoel, if we go this direction there are 2 caveats:
1. What are the performance characteristics if you have 100, 1000, 10000 nodes with entries in pathauto_persist? (especially memory wise as that table grows ...)
2. Instead of using drupal_static_reset, you should just update the static cache on the write.
Overall:
Is the entity_load still a problem when e.g. using entitycache module? (it should mask that and as its internal entity state, should work correctly)
I think we should put this performance improvement to a follow-up and get #242 in for now.
If we don't do this, this might take another 4 years ...
Setting back to RTBC for #242. Can you open a follow-up?
Comment #285
joelpittet@Fabianx good questions but my change is only static the entity types not the entities so I think the memory should be very minimal at worst.
It's to shortcut out the out early to avoid the SQL queries trying to select types that aren't stored. In my case that saved 60 queries per page load. (Many entity types in commerce that don't care about paths)
Comment #286
Fabianx CreditAttribution: Fabianx commentedI misread that part, RTBC for #283. Checking for existing entity_types is fine ...
Comment #287
joelpittetFewf, I was sweating there for a second;) Thanks @Fabianx
Comment #288
MiroslavBanov CreditAttribution: MiroslavBanov commentedThis should be a very well optimized query, right?
'primary key' => array('entity_type', 'entity_id')
It looks optimized to me. If so we don't need to introduce performance-related changes as mentioned in #267.
Comment #289
Fabianx CreditAttribution: Fabianx commentedYes, at least #269 is addressed for this and yes this query is well optimized by the database.
Comment #290
Steven Jones CreditAttribution: Steven Jones commentedSorry @joelpittet but it's not quite 100% yet.
Sorry, but this isn't using the 'drupal_static_fast' pattern correctly.
In its current form,
drupal_static
is still being called on every invocation of this function. (the DB query would only be called once however, as desired).It needs to be like this:
As per the documentation here: https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup... This is just one of the lovely quirks of PHP and static variables! Stepping through with a breakpoint debugger will show this if you're curious.
Comment #291
iamEAP CreditAttribution: iamEAP commentedHere's #283 with the fixed drupal static fast pattern.
Additionally, I ran into issues applying this patch on a pre-existing Drupal site: every page would white screen because of the query against non-existent table {pathauto_state}. I was further unable to run
drush updb
(to fix the problem) because hook_entity_load() was being invoked somewhere during the drush bootstrap process.I've wrapped the DB query in a try/catch to alleviate this potential upgrade path issue.
Comment #292
caspervoogt CreditAttribution: caspervoogt at Plethora commentedI just tried the patch from #291 and it works.
Comment #293
osopolarPatch in #291 works for me too.
Comment #294
joelpittet@Steven Jones sorry for the late reply (honestly I thought someone would step up to bat for me) why does :
? It's a reference to the static variable. The variable doesn't need to be an array like the example shows, that's just if you want multiple keyed parts, FWIU.
As soon as $path_auto_entity_types has something assigned to, so does $drupal_static_fast, and the static is no longer NULL/undefined therefor the isset() doesn't need to call drupal_static().
Some proof with that pudding:
http://3v4l.org/1LUUR
Database call is only once for the types.
@iamEAP there should be no need for the try catch block if you run drush updb and haven't gotten to that module version yet.
Comment #295
joelpittetAlso I stepped through it numerous times while creating the patch to ensure it worked as I expected.
Comment #296
iamEAP CreditAttribution: iamEAP commented@joelpittet Re-applied the patch and did a backtrace. I found the source of the error with Redirect module. The specific scenario is: a) you have the Redirect module installed, and b) your homepage is some kind of entity.
Once you clear caches, the entity load (and thus
hook_entity_load()
, and thus this new implementation of that hook) will occur extremely often.The basic stack is:
_drupal_boostrap_full()
->drupal_path_initialize()
->drupal_get_normal_path()
->redirect_url_inbound_alter()
->redirect_load_entity_from_path()
-> (you get the gist).More critically, in this situation, it's impossible to run drush updb because drush seems to run in the context of the homepage and runs the exact same stack (originating at
_drush_bootstrap_drupal_full()
).Comment #297
MiroslavBanov CreditAttribution: MiroslavBanov commented@joelpittet
The idea of drupal_static_fast is to prevent the call to drupal_static().
Compare this: http://3v4l.org/Eje8H
with this: http://3v4l.org/vhYos
Not sure how much of a difference that would make, particularly with PHP-7, or HHVM, but this is supposed to be a solid optimization for PHP-5.
Comment #298
joelpittet@MiroslavBanov thanks for showing that, I was confused. Also still a bit baffled why using a key of an array would work but I guess that an empty array() is NOT NULL where as a NULL reference variable is still NULL and !isset() will be TRUE.
Thanks for the clarification.
Comment #299
joelpittet@iamEAP So here's your changes minus the try/catch. Is that ok? It sounded like that from #296
Comment #300
joelpittetThanks for fixing my typo on 'unnessasary' I unnecessarily spell that wrong all the time!
Comment #301
Fabianx CreditAttribution: Fabianx as a volunteer commentedBack to RTBC ...
Comment #302
iamEAP CreditAttribution: iamEAP commented@joelpittet I should've been more clear: without the try catch, applying the patch put my site in a broken and un-upgradeable state. I was not able to upgrade the module or fix my site until I added the try/catch.
I anticipate others will run into this, especially those in the case I described in #296 (those with Redirect module installed and some type of entity included on the homepage).
Comment #303
Clemens Sahs CreditAttribution: Clemens Sahs commentedworks great for me
Comment #304
cleaver CreditAttribution: cleaver as a volunteer commentedI tested #299 for a problem I was having with scheduler. When nodes with a custom URL were published on schedule they would revert to the automatic URL.
The patch fixes that problem and no side effects noted.
+1 to RTBC
Comment #305
webservant316 CreditAttribution: webservant316 commentedI see that #299 updates the database. Is that a concern if the patch doesn't get committed? I felt safer using this module https://www.drupal.org/project/pathauto_persist which also handled the problem for my use case. Though I would prefer not not use yet another module, so I will gladly use this patch when it is committed.
Comment #306
cleaver CreditAttribution: cleaver as a volunteer commented@webservant316, You will need to make sure you keep applying the patch if you update the module. I'd suggest drush make (http://drushcommands.com/drush-6x/make/make) to automate the process, or keep your own fork in a sandbox or on github. Hopefully, this will be committed soon. I think we're close if not ready to go now.
Comment #307
MiroslavBanov CreditAttribution: MiroslavBanov commented@webservant316
If you apply the patch, a different update hook could potentially land before this one, and then you could have a bit of a mess to untangle. I've had this happen to me before.
Comment #308
webservant316 CreditAttribution: webservant316 commentedI was planning to use https://www.drupal.org/project/pathauto_persist until this patch is committed, then I will uninstall pathauto_persist and update pathauto.
Comment #309
osopolarIt seems that the patches here are not in sync with the pathauto_persist module, where the latest changes where made at May 9, 2014 and its stable version dates back to 2012-Jul-06. Whereas patch in #299 is from May 31, 2015.
I opted to use the last patch from this issue, hoping/speculating that it gets committed soon – before another update implements this patches update hook (pathauto_update_7006). If this should happen, the only thing you need to be careful is to manually invoke pathauto_update_7006() from the update.
+1 to RTBC, and +1 to get it in before messing up with update-hooks.
Comment #310
gappleI also ran into the same non-upgradeable state that @iamEAP mentioned; redirect module is used and the homepage is a node (and has some views loading other nodes).
Due to an entity_load happening, drush exits with an exception before the updates can be loaded or run.
Comment #311
iamEAP CreditAttribution: iamEAP commented@gapple, did you try patch #291? That should be identical to #299, but with a try/catch to handle that case.
Given someone else hit this in the wild, I might recommend #291 over #299 be committed.
Comment #312
gappleYes, patch #291 resolved the issue and allowed the necessary update to be run.
Comment #313
cleaver CreditAttribution: cleaver as a volunteer commented@iamEAP, is there an interdiff between 291 and 299? I took a quick look, but did not see one.
Comment #314
gappleIt looks like #291 and #299 both have interdiffs from #281, but not from each other.
Here is the relevant change:
Comment #315
emanaton CreditAttribution: emanaton commentedReroll of #291 for branch 1.2; resolves minor issue of hunk offsets.
Comment #316
Dave ReidSo there is code here, and in pathauto_entity_state_load_multiple() to check if the table is available. We should just consolidate it here with a db_table_exists() and remove the check from pathauto_entity_state_load_multiple().
This needs to be reconciled with the changes to Pathauto Persist counting for $entity->path being a non-array value, we should not blow it away.
Comment #317
Dave ReidRe-rolled based on Pathauto Persist's 7.x-1.2 release and including fixes for #316 and documentation fixes.
Comment #319
Dave ReidFixing the tests by merging in the change to assertNoEntityAliasExists().
Comment #320
Dave ReidComment #321
Dave ReidComment #322
Dave ReidWe will also need a change record about this behavior change. And we'll need to update this page: https://www.drupal.org/node/1167612 (but that can be done post-commit).
Comment #323
Dave ReidIf I can't get any reviews on the latest patch within the next couple of days, I'm going to be forced to create the 7.x-1.3 release without this major addition.
Comment #324
joelpittetUsing it in production since yesterday... fingers crossed but no problems so far.
Comment #325
mattltUsing it also. No problems.
Comment #326
Dave ReidComment #327
rv0 CreditAttribution: rv0 commentedgreat, working fine here too!
Comment #328
Fabianx CreditAttribution: Fabianx as a volunteer commentedWorks fine here, back to RTBC
Comment #329
Dave ReidComment #330
Dave ReidCommitted and merged #319 to 7.x-1.x!
Thank you everyone for your work and persistence on this.
Will be releasing the 7.x-1.3 tag shortly now.
Comment #332
dsnopekHuzzah! Thanks so much, everyone! This patch has been weighing at the back of my mind for years now, as it's really the biggest patch we include in Panopoly. Now we can finally drop it! :-)
Comment #333
webservant316 CreditAttribution: webservant316 commentedConcerning #308, can I safely uninstall pathauto_persist and begin to use pathauto-7.x-1.x-dev?
Comment #334
klausiIf you see extremely slow queries against the pathauto_state table in your mysql slow query log then you can hack pathauto same as metatag module like this: #2546014: Disable metatags loading for large entity_load()s.
This problem can occur if you do large entity loads in background jobs for example (because you are too lazy to rewrite views_data_export to process batches properly for example :-D).
Comment #335
Dave Reid- [x] Edited the doc page at https://www.drupal.org/node/1167612 to note the change in behavior.
- [x] Added a change record at https://www.drupal.org/node/2581993
- [x] Updated the pathauto_persist project page at https://www.drupal.org/project/pathauto_persist
Comment #336
lpalgarvio CreditAttribution: lpalgarvio commentedgreat!!! fantastic!!! thanks :)
Comment #337
webservant316 CreditAttribution: webservant316 commentedTwo questions...
1. Does this fix eliminate the need for https://www.drupal.org/project/pathauto_persist?
2. Can I safely uninstall pathauto_persist and update to the latest pathauto version?
Comment #338
DamienMcKenna@webservant316: Yes. Yes.
Comment #339
jstoller@webservant316: Make sure you update Pathauto before uninstalling Pathauto Persist, if you want to maintain your current settings.
Comment #341
azinck CreditAttribution: azinck commentedIt should be noted that any modules that declare a dependency on Pathauto Persist are going to get disabled by this update. This could cause real mayhem on some sites. At least some note of this should be made in the release notes.
Comment #342
crystaldawn CreditAttribution: crystaldawn commentedRe-opening since this shouldnt be marked as closed. The problem still exists in D7 7.41 + Pathauth 1.3 which is current at this date. Not sure why, but the problem still persists.
Comment #343
DamienMcKenna@crystaldawn: The functionality is now in Pathauto 7.x-1.3 (it's the very first item listed in the changelog). Please open a new issue if you're experiencing a problem with it.
Comment #344
crystaldawn CreditAttribution: crystaldawn commentedFor the droves of people who are going to come to this thread looking for a fix, I opened a new issue here and it has a simple 1-line workaround for now but is by no means a "fix": https://www.drupal.org/node/2617716#comment-10579652