I think the next big problem we have in Pathauto is solving what happens when code does something like this:

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

Files: 
CommentFileSizeAuthor
#178 pathauto-persist-936222-178-pathauto-state.patch14.39 KBsammarks15
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]
#132 pathauto-persist-936222-interdiff-120-130-pathauto-state.txt1.18 KBmvc
#130 pathauto-persist-936222-130-pathauto-state.patch14.27 KBmvc
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

Comments

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

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

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

Status:Active» Needs review
StatusFileSize
new647 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 936222-pathauto-node-update-logic-D7.patch.
[ View ]

And so this is all we'd need to change.

Status:Needs review» Needs work

(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

...subscribing

Priority:Normal» Major

Bumping to major as this needs to be addressed before 7.0.

StatusFileSize
new680 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 936222-pathauto-node-update-logic-D7A2.patch.
[ View ]

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

Title:Rethink if we should interfere on node_save() callsPathauto reverts to default patterns on all node_save() calls
Category:task» bug

Marked #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:

<?php
$node
= node_load($nid);
node_save($node);
?>

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.

Title:Pathauto reverts to default patterns on all node_save() callsMerge in pathauto_persist module functionality to prevent losing manual aliases with node_save() calls
Status:Needs work» Active

Yes this was the idea behind Pathauto persist which is going to be merged into Pathauto. Retitling issue to for the new direction.

Sweet, this has my support, for what it's worth.

Great news!

StatusFileSize
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

Status:Active» Needs work

Hmm... this patch causes test to fail in generating bulk aliases. Trying to grok it still, but NW for now.

Status:Needs work» Active

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

Trying to grok it still

Here's the problem:
Suppose node/1 has the pathauto checkbox set, and node/2 has a custom path, and you have this code:

$node1 = node_load(1);
$node1->title = 'My new title';
// $node1->path['pathauto'] = TRUE;
node_save($node1);
$node2 = node_load(2);
// $node2->path['pathauto'] = FALSE;
node_save($node2);

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.

Status:Active» Needs review
StatusFileSize
new3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

subscribing

This looks good to me.

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

@quicksketch: I agree. This issue is about merging in that functionality for good.

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

Status:Needs review» Active

AFAIK, #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.

Status:Active» Needs review

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

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

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

...drupal can wait. You go have a great wedding now ;)

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

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

Hey 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 ;)

StatusFileSize
new8.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pathauto_persist_merge-d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new6.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pathauto_persist_merge-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new6.99 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

D7 version (same as above) without suffix so it can be tested.

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

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

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.

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

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

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

Still, checking the table can make the upgrade for those 300 users significantly faster. :)

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

Subscribing.

Subscribing.

StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/pathauto/pathauto.install.
[ View ]
new9.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pathauto_persist-d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Subscribing.

looking forward to this :)

Subscribing

Status:Needs review» Needs work

Still haunted by this :/

...applied d7 patch in #40 (manually) and tried editing a node that already had a translation. I got this on save:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'my_drupal_db.pathauto' doesn't exist: SELECT 1 AS expression FROM {pathauto} pathauto WHERE ( (entity_type = :db_condition_placeholder_0) AND (entity_id = :db_condition_placeholder_1) ) FOR UPDATE; Array ( [:db_condition_placeholder_0] => node [:db_condition_placeholder_1] => 6 ) in pathauto_entity_state_save() (line 222 of /var/www/sites/all/modules/pathauto/pathauto.module).

...Table 'my_drupal_db.pathauto' doesn't exist...

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

Fatal error: Cannot redeclare pathauto_update_7005() (previously declared in /var/www/sites/all/modules/pathauto/pathauto.install:181) in /var/www/sites/all/modules/pathauto/pathauto.install on line 287

...Cannot redeclare pathauto_update_7005...

...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) to 7006. ...reload update.php and now we seem to be good to go... 1 pending update... run it...

The following updates returned messages
pathauto module
Update #7006
Failed: PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'pathauto' at row 1: INSERT INTO {pathauto} (entity_type, entity_id, pathauto) VALUES (:entity_type, :entity_id, :pathauto); Array ( [:entity_type] => node [:entity_id] => 3 [:pathauto] => ) in pathauto_update_7006() (line 277 of /var/www/sites/all/modules/pathauto/pathauto.install).

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

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

subscribing

While 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?

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

Actually, 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(?).

Am I the only one annoyed by this behavior?

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

...but I would expect #40 to fix that. If it doesn't,...

...it doesn't! I have the patch already applied.

subscribing

I 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?

I can confirm the issues in #40. I moved the update in the patch to 7006, but still get

Failed: PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'pathauto' at row 1: INSERT INTO {pathauto} (entity_type, entity_id, pathauto) VALUES (:entity_type, :entity_id, :pathauto); Array ( [:entity_type] => node [:entity_id] => 43 [:pathauto] => ) in pathauto_update_7006() (line 277 of /Library/WebServer/htdocs/JE2/trunk/public_html/sites/all/modules/pathauto/pathauto.install).

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?

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

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

From #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']))) {

Thanks for that Rick.

I'd have done that myself, but didn't want to steal Aaron's thunder ;)

I only did that so we didn't steal Aaron's time :)

subscribing

Just 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 the system 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)?

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

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

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

Status:Needs work» Needs review
StatusFileSize
new9.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pathauto_persist-d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] 260 pass(es), 20 fail(s), and 3 exception(s).
[ View ]

Rerolled both patches in #40 to latest devs.

Status:Needs review» Needs work

The last submitted patch, pathauto_persist.patch, failed testing.

Subscribing

@stacysimpson There is now a "Follow" button you can click at the top of the page instead of leaving a comment to subscribe.

Status:Needs work» Needs review

#66: pathauto_persist.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, pathauto_persist.patch, failed testing.

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

I think you mean install the module pathauto_persist http://drupal.org/project/pathauto_persist

nevermind.

It's been more than a year now. Can we please get some love in this issue?

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

Status:Needs work» Needs review
StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] 296 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 936222-pathauto-persist.patch, failed testing.

StatusFileSize
new8.09 KB
FAILED: [[SimpleTest]]: [MySQL] 296 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

small adjustment to this patch as we were getting

SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: '' for column 'pathauto' at row 1

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.

StatusFileSize
new8.09 KB
FAILED: [[SimpleTest]]: [MySQL] 296 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

My bad here is the correct patch for this issue.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 936222-80-pathauto-persist.patch, failed testing.

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

Issue tags:+Needs tests

+++ b/pathauto.installundefined
@@ -169,6 +202,93 @@ function pathauto_update_7005() {
+      $table = array(
+        'description' => 'Holds the state of Pathauto\'s setting on any individual entity.',
+        'fields' => array(
+          'entity_type' => array(
+            'type' => 'varchar',
+            'length' => 128,
+            'not null' => TRUE,
+            'description' => 'An entity type.',
+          ),
+          'entity_id' => array(
+            'type' => 'int',
+            'unsigned' => TRUE,
+            'not null' => TRUE,
+            'description' => 'An entity ID.',
+          ),
+          'pathauto' => array(
+            'type' => 'int',
+            'size' => 'tiny',
+            'not null' => TRUE,
+            'default' => 0,
+            'description' => 'The automatic alias status of the entity.',
+          ),
+        ),
+        'primary key' => array('entity_type', 'entity_id'),

Why is the schema duplicated here?

The tests need updating, too. Simpletest _must_ succeed.

Best,

Fabian

Fabian: update functions should duplicate the schema. See http://drupal.org/node/150220

Thanks for the explanation. That is fine then.

This issue still needs updated tests.

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

Any foreseeable movement on this issue which currently affects about a quarter of a million websites?

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

Status:Needs work» Needs review
StatusFileSize
new10.48 KB
FAILED: [[SimpleTest]]: [MySQL] 387 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Needs tests

The last submitted patch, pathauto-persist-936222-90.patch, failed testing.

Status:Needs work» Needs review

#90: pathauto-persist-936222-90.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests

The last submitted patch, pathauto-persist-936222-90.patch, failed testing.

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

Status:Needs work» Needs review
StatusFileSize
new11.16 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

I fixed the two failing tests. The code that was failing was:

<?php
// Add a new node.
$new_node = $this->drupalCreateNode(array('path' => array('alias' => '', 'pathauto' => FALSE)));
// Run the update again which should only run against the new node.
$this->drupalPost('admin/config/search/path/update_bulk', $edit, t('Update'));
$this->assertText('Generated 1 URL alias.'); // 1 node + 0 users
$this->assertEntityAliasExists('node', $new_node);
?>

However, pathauto_node_update_alias() starts with the following:

<?php
function pathauto_node_update_alias(stdClass $node, $op, array $options = array()) {
 
// Skip processing if the user has disabled pathauto for the node.
 
if (isset($node->path['pathauto']) && empty($node->path['pathauto'])) {
    return;
  }
 
// snipped rest...
}
?>

Therefore, the node as created by the test will not have an alias. The attached patch updates the test to reflect this:

<?php
   
// Add a new node.
   
$new_node = $this->drupalCreateNode(array('path' => array('alias' => '', 'pathauto' => FALSE)));
   
// Run the update again which should not run against any nodes.
   
$this->drupalPost('admin/config/search/path/update_bulk', $edit, t('Update'));
   
$this->assertText('No new URL aliases to generate.');
   
$this->assertNoEntityAliasExists('node', $new_node);
?>

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

Looks very close to RTBC to me. Anyone else up to a review?

thanks 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:

This version of Pathauto includes the functionality of Pathauto Persist, so that module can now be safely disabled.

however, on second thought, it would be better to say:

This version of Pathauto includes the functionality of Pathauto Persist, which can be safely disabled and removed. All settings have been copied.

thoughts?

StatusFileSize
new11.19 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

minor UX change from #98

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

<?php
function pathauto_requirements($phase) {
 
$t = get_t();
  if (
$phase == 'runtime' && module_exists('pathauto_persist')) {
   
$requirements['pathauto'] = array(
     
'title' => $t('Pathauto'),
     
'description' => $t('Pathauto Persist is installed and enabled. As Pathauto Persist has been merged into Pathauto, the Pathauto Persist module can be safely disabled and removed. All Pathauto Persist settings have been migrated to the Pathauto implementation.'),
     
'severity' => REQUIREMENT_INFO,
    );
  }
}
?>

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.

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

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)

You 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:

module_disable(array('pathauto_persist'));

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.

StatusFileSize
new11.89 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

Attached is a patch that adds the hook_requirements from #100 to mvc's patch from #99.

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

Thanks everyone for the great work on this so far!

+++ b/pathauto.installundefined
@@ -8,6 +8,40 @@
+  $schema['pathauto'] = array(
+    'description' => '',

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.

+++ b/pathauto.installundefined
@@ -169,6 +203,23 @@ function pathauto_update_7005() {
+    $schema = drupal_get_schema_unprocessed('pathauto', 'pathauto');

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.

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

Status:Needs work» Needs review
StatusFileSize
new13.7 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

* 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()

I'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?

@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"?

"OP" = Original Post(er). It refers to what we in d.o call an issue summary ;)

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

StatusFileSize
new1.64 KB

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

StatusFileSize
new13.67 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

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

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

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

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

Status:Needs review» Reviewed & tested by the community

I have waited for this for a long time.

Lets get this finally in!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new800 bytes
new13.7 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

I agree with #120, its green => RTBC again

Status:Reviewed & tested by the community» Needs work

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

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

'autocreate' would be another naming option.

pathauto_manual ? More descriptive and wouldn't require the boolean flip.

my $0.02, pathauto_locks is the most readable and understandable suggestion

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

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

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new14.37 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]
new14.27 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

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

Status:Needs work» Needs review

I second quicksketch's comments that the table should be simply named 'pathauto'.

oops, 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 :)

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

Is there possibly a chance that thetable pathauto might be needed in the future for something more fitting?

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

Status:Needs review» Reviewed & tested by the community

The pathauto-persist-936222-130-pathauto-state.patch from #130 works beautifully for me! What left to get this committed?

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

+1 for RTBC. Nice work, all.

I 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!!

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.28 KB
FAILED: [[SimpleTest]]: [MySQL] 310 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, pathauto-persist-936222-140-detect-pathauto-upon-node-save.patch, failed testing.

StatusFileSize
new1.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pathauto-persist-936222-141-detect-pathauto-upon-node-save.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Note that this solution can also be applied in a custom module as a work around:

<?php
/**
* Implements hook_node_presave().
* @param $node
*/
function YOURMODULENAME_node_presave($node) {
 
// Work-around for pathauto issue #936222
 
if (module_exists('pathauto') && !empty($node->nid) && (!isset($node->path) || !isset($node->path['pathauto']))) {
   
// This node was saved without specifying the desired pathauto options,
    // probably programmatically.
    // We will only create a new alias automatically if and only if the original
    // title matched the auto generated path alias.
   
$original = $node->original;
   
module_load_include('inc', 'pathauto');
   
$uri = entity_uri('node', $original);
   
$path = drupal_get_path_alias($uri['path'], $original->language);
   
$pathauto_alias = pathauto_create_alias('node', 'return', $uri['path'], array('node' => $original), $original->type, $original->language);
    if (!isset(
$node->path)) {
     
$node->path = array();
    }
   
$node->path['pathauto'] = ($path != $uri['path'] && $path == $pathauto_alias);
  }
}
?>

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

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

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

@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!

3rd birthday of this issue coming up in a week :(

Please commit.

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

Hi greggles,

There's an issue that's considered fixed for a while now and is causing people headaches. The issue has been open for 3 years and people are widely using patched 'unofficial' pathauto modules because of that. The disadvantages of leaving the project release cycle prevent others from doing the same and they have to live with the bug.

No action has been undertaken by any of the module maintainers (your or Dave Reid). I was wondering if you would consider committing the patch to the dev branch and/or release it, or would consider having another maintainer who is able to do this.

See https://drupal.org/node/936222

Kind regards,
Bas

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

Issue tags:-Needs tests

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

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

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

I'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:

Additional uncaught exception thrown while handling exception.

Original

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'dbnamehere.pathauto_state' doesn't exist: SELECT entity_id, pathauto FROM {pathauto_state} WHERE entity_type = :entity_type AND entity_id IN (:entity_ids_0); Array ( [:entity_type] => user [:entity_ids_0] => 1 ) in pathauto_entity_state_load_multiple() (line 443 of /.../sites/all/modules/pathauto/pathauto.module).

Additional

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'dbnamehere.pathauto_state' doesn't exist: SELECT entity_id, pathauto FROM {pathauto_state} WHERE entity_type = :entity_type AND entity_id IN (:entity_ids_0); Array ( [:entity_type] => user [:entity_ids_0] => 1 ) in pathauto_entity_state_load_multiple() (line 443 of /.../sites/all/modules/pathauto/pathauto.module).

I'll report back here if I can't get this resolved.

All you have to do is apply the patch and run update.php.

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

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

The -pathauto-state patch and update worked fine for me. I did not previously have the pathauto_persist module installed.

I 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

Issue summary:View changes

Added XREF to pathauto_persist

I 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

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

Issue summary:View changes
Issue tags:-Needs tests

Removing 'needs tests' tag again because the patch in #130 has them.

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

Can't wait for that dev! - I've been patching my installations for 3 years now with each update.

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

@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)?

Issue summary:View changes
Related issues:+#358722: Node aliases lost/changed when using i18n synchronize translations, +#845158: Design flaw: manual aliases won't work without storing auto alias setting in the DB, +#920702: Publishing Unpublished Content resets path, +#931740: If a user-profile alias changes, try to remember that in the future, +#937826: Merge into Pathauto ?

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

+++ b/pathauto.install
@@ -8,6 +8,40 @@
+      'entity_id' => array(
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => TRUE,
+        'description' => 'An entity ID.',
+      ),

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

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Reviewed & tested by the community

It's the patch #130 (state version) that is RTBC.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Reviewed & tested by the community

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

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

INSERT INTO pathauto_state (
SELECT b.* FROM (
SELECT
IF(LOCATE('node', source),'node',IF(LOCATE('user', source),'user',false)) as entity_type,
IF(0=LOCATE('node', source),IF(0=LOCATE('user', source),false,SUBSTRING(source,6)),SUBSTRING(source,6)) as entity_id,
IF(LOCATE('content', alias),1,IF(LOCATE('users', alias),1,0)) as pathauto
FROM  `url_alias`
) as b
LEFT JOIN pathauto_state AS p ON (b.entity_type = p.entity_type AND b.entity_id = p.entity_id)
WHERE p.entity_type IS NULL
)

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new14.39 KB
PASSED: [[SimpleTest]]: [MySQL] 389 pass(es).
[ View ]

I'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 the path property to an empty array if it doesn't already exists and then continues to update the pathauto key.

After updating that code the warnings disappeared.

I'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!

@plethoradesign: I've created an issue in the Panopoly queue for this #2239847: Update "state" 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!

@sammarks15

can you write a interdiff?