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.

CommentFileSizeAuthor
#319 interdiff.txt763 bytesDave Reid
#319 9636222-pathauto-persist.patch13.89 KBDave Reid
#317 interdiff.txt17.72 KBDave Reid
#317 9636222-pathauto-persist.patch13.25 KBDave Reid
#315 pathauto-persists-936222-315.patch15.74 KBemanaton
#299 interdiff.txt975 bytesjoelpittet
#299 merge_in-936222-299.patch15.47 KBjoelpittet
#291 pathauto-persists-936222-291.patch15.74 KBiamEAP
#291 pathauto-persists-936222-291.interdiff.txt1.27 KBiamEAP
#283 pathauto-persists-936222-281.patch15.45 KBjoelpittet
#283 interdiff.txt735 bytesjoelpittet
#281 interdiff.txt1.85 KBjoelpittet
#281 pathauto-persists-936222-281.patch15.49 KBjoelpittet
#278 interdiff.txt3 KBjoelpittet
#278 pathauto-persists-936222-278.patch15.18 KBjoelpittet
#242 pathauto-persist_pathauto_state-936222-242.patch14.69 KBmrmikedewolf
#235 pathauto.module.patch535 bytespinoniq
#234 pathauto-persist-936222-234-pathauto-state.patch12.79 KBbibishani
#213 pathauto-persist-936222-213-pathauto-state.patch14.69 KBponies
#211 pathauto-persist-936222-211-pathauto-state.patch14.64 KBponies
#209 pathauto-persist-936222-209-pathauto-state.patch14.66 KBRoloDMonkey
#209 interdiff-pathauto-persist-936222-195-209.txt2.35 KBRoloDMonkey
#208 pathauto-persist-936222-208-pathauto-state.patch14.49 KBRoloDMonkey
#208 interdiff-pathauto-persist-936222-195-208.txt2.18 KBRoloDMonkey
#195 pathauto-persist-936222-195-pathauto-state.patch14.87 KBsammarks15
#193 interdiff-pathauto-persist-936222-178-193.patch.txt1.38 KBsammarks15
#193 pathauto-persist-936222-193-pathauto-state.patch15.15 KBsammarks15
#182 interdiff-936222-130-178-pathauto-state.txt554 bytessammarks15
#178 pathauto-persist-936222-178-pathauto-state.patch14.39 KBsammarks15
#142 pathauto-persist-936222-141-detect-pathauto-upon-node-save.patch1.29 KBbvanmeurs
#140 pathauto-persist-936222-140-detect-pathauto-upon-node-save.patch1.28 KBbvanmeurs
#132 pathauto-persist-936222-interdiff-120-130-pathauto-state.txt1.18 KBmvc
#132 pathauto-persist-936222-interdiff-120-130-pathauto-regenerate.txt4.69 KBmvc
#130 pathauto-persist-936222-130-pathauto-state.patch14.27 KBmvc
#130 pathauto-persist-936222-130-pathauto-regenerate.patch14.37 KBmvc
#120 pathauto-persist-936222-120.patch13.7 KBPancho
#120 pathauto-persist-936222-interdiff-108-120.txt800 bytesPancho
#115 pathauto-persist-936222-115.patch13.67 KBbrianV
#114 stacktrace.txt1.64 KBsylus
#108 pathauto-persist-936222-108.patch13.7 KBmvc
#103 pathauto-persist-936222-103.patch11.89 KBbrianV
#99 pathauto-persist-936222-99.patch11.19 KBmvc
#95 pathauto-persist-936222-95.patch11.16 KBbrianV
#90 pathauto-persist-936222-90.patch10.48 KBmvc
#80 936222-80-pathauto-persist.patch8.09 KBlathan
#79 936222-pathauto-persist.patch8.09 KBlathan
#77 936222-pathauto-persist.patch8.04 KBpopulist
#66 pathauto_persist.patch8.04 KBIsland Usurper
#66 pathauto_persist-d6.patch9.48 KBIsland Usurper
#40 pathauto_persist-d6.patch9.48 KBquicksketch
#40 pathauto_persist-d7.patch8.04 KBquicksketch
#30 pathauto_persist_merge.patch6.99 KBquicksketch
#29 pathauto_persist_merge-d7.patch6.99 KBquicksketch
#29 pathauto_persist_merge-d6.patch8.45 KBquicksketch
#16 936222-pathauto-persist-16.patch3.67 KBeffulgentsia
#13 empty-pathauto.patch1.79 KBGábor Hojtsy
#8 936222-pathauto-node-update-logic-D7A2.patch680 byteseffulgentsia
#4 936222-pathauto-node-update-logic-D7.patch647 bytesDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

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

Dave Reid’s picture

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.

Dave Reid’s picture

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.

Dave Reid’s picture

Status: Active » Needs review
FileSize
647 bytes

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

Dave Reid’s picture

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

klonos’s picture

...subscribing

Dave Reid’s picture

Priority: Normal » Major

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

effulgentsia’s picture

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.

quicksketch’s picture

Title: Rethink if we should interfere on node_save() calls » Pathauto 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:

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

Dave Reid’s picture

Title: Pathauto reverts to default patterns on all node_save() calls » Merge 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.

quicksketch’s picture

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

klonos’s picture

Great news!

Gábor Hojtsy’s picture

FileSize
1.79 KB

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.

JacobSingh’s picture

Status: Active » Needs work

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

effulgentsia’s picture

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.

effulgentsia’s picture

Status: Active » Needs review
FileSize
3.67 KB

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.

YK85’s picture

subscribing

JacobSingh’s picture

This looks good to me.

quicksketch’s picture

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.

Dave Reid’s picture

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

klonos’s picture

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.

effulgentsia’s picture

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.

David_Rothstein’s picture

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

klonos’s picture

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

Dave Reid’s picture

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.

klonos’s picture

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

vinmassaro’s picture

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.

klonos’s picture

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

quicksketch’s picture

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.

quicksketch’s picture

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

Dave Reid’s picture

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.

Dave Reid’s picture

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.

quicksketch’s picture

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.

Dave Reid’s picture

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.

quicksketch’s picture

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.

Dave Reid’s picture

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

klonos’s picture

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.

Cyberwolf’s picture

Subscribing.

stackpr’s picture

Subscribing.

quicksketch’s picture

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.

dimitriseng’s picture

Subscribing.

lpalgarvio’s picture

looking forward to this :)

Taxoman’s picture

Subscribing

klonos’s picture

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.

klonos’s picture

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

yannisc’s picture

subscribing

klonos’s picture

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?

klonos’s picture

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

klonos’s picture

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

effulgentsia’s picture

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.

klonos’s picture

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

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

ja09’s picture

subscribing

dynamicdan’s picture

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?

rickmanelius’s picture

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?

aacraig’s picture

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.

klonos’s picture

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.

rickmanelius’s picture

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

aacraig’s picture

Thanks for that Rick.

klonos’s picture

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

rickmanelius’s picture

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

dema502’s picture

subscribing

klonos’s picture

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

klonos’s picture

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

quicksketch’s picture

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.

klonos’s picture

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.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
8.04 KB

Rerolled both patches in #40 to latest devs.

Status: Needs review » Needs work

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

stacysimpson’s picture

Subscribing

quicksketch’s picture

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

blainelang’s picture

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.

theagonizer’s picture

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.

blainelang’s picture

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

nodecode’s picture

nevermind.

klonos’s picture

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

mikeryan’s picture

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

populist’s picture

Status: Needs work » Needs review
FileSize
8.04 KB

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.

lathan’s picture

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.

lathan’s picture

My bad here is the correct patch for this issue.

Jim Kirkpatrick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

klonos’s picture

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

Fabianx’s picture

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

pjcdawkins’s picture

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

Fabianx’s picture

Thanks for the explanation. That is fine then.

This issue still needs updated tests.

dimitriseng’s picture

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.

nodecode’s picture

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

aendra’s picture

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

mvc’s picture

Status: Needs work » Needs review
FileSize
10.48 KB

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.

ajFernandez’s picture

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.

mvc’s picture

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.

brianV’s picture

Status: Needs work » Needs review
FileSize
11.16 KB

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


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


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:

    // 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);
d.clarke’s picture

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.

Fabianx’s picture

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

mvc’s picture

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?

mvc’s picture

minor UX change from #98

brianV’s picture

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

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.

mvc’s picture

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

quicksketch’s picture

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.

brianV’s picture

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

mvc’s picture

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.

klonos’s picture

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.

Dave Reid’s picture

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.

mvc’s picture

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.

mvc’s picture

Status: Needs work » Needs review
FileSize
13.7 KB

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

ericras’s picture

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?

mvc’s picture

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

klonos’s picture

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

ericras’s picture

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

acbramley’s picture

sylus’s picture

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

brianV’s picture

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

mvc’s picture

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

Pancho’s picture

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

mvc’s picture

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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I have waited for this for a long time.

Lets get this finally in!

Pancho’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
800 bytes
13.7 KB

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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

mvc’s picture

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.

Pancho’s picture

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

Pancho’s picture

'autocreate' would be another naming option.

John Pitcairn’s picture

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

acbramley’s picture

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

Pancho’s picture

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

quicksketch’s picture

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.

mvc’s picture

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.

mvc’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.37 KB
14.27 KB

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.

DamienMcKenna’s picture

Status: Needs work » Needs review

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

mvc’s picture

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

rooby’s picture

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

quicksketch’s picture

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.

dsnopek’s picture

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?

klonos’s picture

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.

acbramley’s picture

steveoliver’s picture

+1 for RTBC. Nice work, all.

pwaterz’s picture

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

bvanmeurs’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.28 KB

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.

bvanmeurs’s picture

bvanmeurs’s picture

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

/**
 * 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);
  }
}
bvanmeurs’s picture

Status: Needs work » Needs review
JamesOakley’s picture

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.

bvanmeurs’s picture

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.

dsnopek’s picture

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

klonos’s picture

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

Please commit.

bvanmeurs’s picture

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

greggles’s picture

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

mvc’s picture

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

greggles’s picture

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.

dsnopek’s picture

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

deanflory’s picture

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.

pwaterz’s picture

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

dsnopek’s picture

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

deanflory’s picture

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.

mattlt’s picture

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

Danny_Joris’s picture

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

Danny_Joris’s picture

Issue summary: View changes

Added XREF to pathauto_persist

slv_’s picture

aalamaki’s picture

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

gauravvdeshpande’s picture

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.

mvc’s picture

Issue summary: View changes
Issue tags: -Needs tests

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

Dave Reid’s picture

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.

klonos’s picture

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

dsnopek’s picture

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

klonos’s picture

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

klonos’s picture

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.

klonos’s picture

barraponto’s picture

+++ 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
barraponto’s picture

Status: Needs work » Reviewed & tested by the community

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

jenlampton’s picture

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.

jenlampton’s picture

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.

Clemens Sahs’s picture

#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
)
sammarks15’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.39 KB

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.

caspervoogt’s picture

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!

dsnopek’s picture

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

Clemens Sahs’s picture

@sammarks15

can you write a interdiff?

sammarks15’s picture

Oops, sorry about that. I've attached the interdiff.

mvc’s picture

Status: Needs review » Reviewed & tested by the community

@sammarks15: thanks for the interdiff. your patch works for me, and it seems like a safe test.

edxxu’s picture

The patch from #178 works for me. Thanks @sammarks15.

BarisW’s picture

Status: Reviewed & tested by the community » Needs work

When 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 :(

dsnopek’s picture

@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 in pathauth_entity_state_load_multiple().

sammarks15’s picture

And 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()?

dsnopek’s picture

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

sammarks15’s picture

What 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 to drupal_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).

dsnopek’s picture

Sure, I think a default value makes sense.

quicksketch’s picture

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

dsnopek’s picture

@quicksketch: That sounds brilliant! I really like the try/catch vs checking if the table exists.

sammarks15’s picture

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

Status: Needs review » Needs work

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

sammarks15’s picture

I guess my PHPStorm-generated patch doesn't work well with Drupal's CI tests. This one is generated by git instead.

sammarks15’s picture

Status: Needs work » Needs review

Updating status and hiding old files. The interdiff in #193 still applies to the patch in #195.

dsnopek’s picture

@sammarks15: Thanks for the new patch! In the next couple days, I hope to do some testing with Panopoly. :-)

dsnopek’s picture

Status: Needs review » Reviewed & tested by the community

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

Pere Orga’s picture

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

hefox’s picture

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

RoloDMonkey’s picture

hefox,

If I understand, what you are describing sounds like it should be in a separate issue.

Fabianx’s picture

RTBC + 1

hefox’s picture

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

DamienMcKenna’s picture

FYI 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

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

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

DamienMcKenna’s picture

I suggest including this in the 1.3 release.

DamienMcKenna’s picture

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

RoloDMonkey’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
14.49 KB

Here is a new patch, per Dave's request in #205.

RoloDMonkey’s picture

Oops, I missed the change in #2047811-6: Make compatible with non-integer entity IDs. Uploading the new files. Hiding the old ones.

DrCord’s picture

this latest patch from #209 worked perfectly with the latest pathauto (7.x-1.2+21-dev) and Drupal 7.31.

ponies’s picture

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

m.stenta’s picture

Status: Needs review » Needs work

Small nit-pick (but necessary for Drupal coding standards):

+++ b/pathauto.module
@@ -384,6 +405,121 @@ function pathauto_entity_presave($entity, $type) {
+	// filter out entity_ids that are not integers
+	$entity_ids = array_filter($entity_ids, 'is_numeric');
+	// if everything was filtered out, return an empty array
+	if(empty($entity_ids)) {
+		return array();
+	}

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

ponies’s picture

Here it is again, this time with fewer tabs.

m.stenta’s picture

Status: Needs work » Needs review
JeebsUK’s picture

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

RoloDMonkey’s picture

Patches are almost always done against the dev branch.

acbramley’s picture

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

acbramley’s picture

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

function mymodule_init() {
  $node = node_load(1);
  $node->title = 'My new title';
  node_save($node);
}

3) Reload the node page

Outcome:
The title is correctly updated and the node alias remains the same.

RoloDMonkey’s picture

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

pbuyle’s picture

Patch in #213 (applied release 1.2) solved the issue in my case.

mvc’s picture

Status: Needs review » Reviewed & tested by the community

two positive reviews; changing status

JeebsUK’s picture

It solved our issues too so another +1 for RTBC.

dustinleblanc’s picture

Chiming in on another success for #213

glass.dimly’s picture

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

jrabeemer’s picture

Patch #213 works for me. Ship it!

Leeteq’s picture

How about committing #213 and then set back to add some tests?
Would be great to have it in -dev meanwhile.

loziju’s picture

Status: Reviewed & tested by the community » Needs work

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

pbuyle’s picture

IMHO, #48 should be addressed in a separated, follow-up, issue and should not prevent completion of this issue. Just like #224.

Leeteq’s picture

Status: Needs work » Reviewed & tested by the community
swirt’s picture

#213 works for me with workbench_moderation and no previous instance of pathauto_persist.
+1 RTBC

jstoller’s picture

#213 appears to solve all my problems. +1 RTBC.

fictionindustries’s picture

#213 Works like a charm

scott.whittaker’s picture

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

bibishani’s picture

Patch #233 works great, but I have conflict with other update hook, so this is a patch without the update hook

pinoniq’s picture

FileSize
535 bytes

These 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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 235: pathauto.module.patch, failed testing.

JamesOakley’s picture

Status: Needs work » Reviewed & tested by the community

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

pinoniq’s picture

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

JamesOakley’s picture

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

dsnopek’s picture

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

firfin’s picture

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

mrmikedewolf’s picture

Patch #213 is great. Many thanks to ponies. I've renamed it to follow drupal conventions. If this is committed, please credit the original author.

RyanPrice’s picture

Applied patch #242 to our environment and it solved the issue. Let's get this committed and closed.

klonos’s picture

All I want for Christmas is this to get in. I'll settle for New Year, but please please Santa!

Aron Novak’s picture

+1 , we use this patch in production environments without any issues to solve this bug

Leeteq’s picture

Happy 2015. I know Santa does not exist, but there are strong indications that his helpers do...

s.perilhou’s picture

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

JamesOakley’s picture

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

s.perilhou’s picture

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

JamesOakley’s picture

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

alcroito’s picture

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

jstoller’s picture

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

s.perilhou’s picture

Sure.

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

alcroito’s picture

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

wOOge’s picture

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

kreatIL’s picture

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

wOOge’s picture

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

MiroslavBanov’s picture

This doesn't sound very promising. If publishcontent or rules can create problems with this patch, then what about vbo, workflow, workbench moderation, scheduler?

Edit: This was rebuted in following comments.

klonos’s picture

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

MiroslavBanov’s picture

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

kreatIL’s picture

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

dsnopek’s picture

This doesn't sound very promising. If publishcontent or rules can create problems with this patch, then what about vbo, workflow, workbench moderation, scheduler?

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

MiroslavBanov’s picture

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

dkendrickmbll’s picture

This patch resolved our issue of the disabling of the Generate automatic URL alias and entering a manual URL alias not saving. Thanks for this!

jstoller’s picture

@Dave Reid, do you see any issues stopping this from being committed now? Pretty please. :-)

webservant316’s picture

yes, please commit.

Steven Jones’s picture

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

Fabianx’s picture

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

MiroslavBanov’s picture

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

Steven Jones’s picture

I'd be happy to see this patch get in, and then a fix for the performance in a follow up.

ladybug_3777’s picture

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

ladybug_3777’s picture

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

alauzon’s picture

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

jstoller’s picture

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

JeebsUK’s picture

I would have to concur with the above #274. Please can we take the multilingual scenario into a new separate issue.

ladybug_3777’s picture

I agree, this patch is ready, let's not complicate it with the multilingual issue.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

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

    rules_config => (1 call) ...(Array, 3 elements)
    commerce_order => (5 calls) ... (Array, 1 element)
    country => 1 call (Array, 250 elements)
    commerce_line_item (42 calls) ... (Array, 1 element)
    commerce_discount => 1 call ... (Array, 3 elements)
    commerce_discount_offer => 2 calls ... (Array, 1 element)
    mailchimp_signup => 1 call ... (Array, 2 elements)
    search_api_index => 1 call ... (Array, 2 elements)
    search_api_server => 1 call ... (Array, 1 element)

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_types in {pathauto_state}. Then a simple check before making the SQL other SQL calls if that entity type exists in the table?

function pathauto_entity_state_load_multiple($entity_type, $entity_ids) {
  static $path_auto_entity_types;

  if (!isset($path_auto_entity_types)) {
    $path_auto_entity_types = db_query("SELECT entity_type FROM {pathauto_state} GROUP BY entity_type")->fetchAllKeyed(0, 0);
  }

  // We aren't storing state for this entity_type at the moment.
  if (!isset($path_auto_entity_types[$entity_type])) {
    return array();
  }
  ...

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.

  1. +++ b/pathauto.install
    @@ -169,6 +220,55 @@ function pathauto_update_7005() {
    +    if (db_table_exists('pathauto_persist')) {
    +      // Rename pathauto_persist's table, then create a new empty one just so
    +      // that we can cleanly disable that module.
    +      db_rename_table('pathauto_persist', 'pathauto_state');
    

    May want to do this check first if someone had this other module installed

  2. +++ b/pathauto.module
    @@ -384,6 +405,121 @@ function pathauto_entity_presave($entity, $type) {
    +function pathauto_entity_state_load_multiple($entity_type, $entity_ids) {
    +  try {
    +       // filter out entity_ids that are not integers
    +       $entity_ids = array_filter($entity_ids, 'is_numeric');
    +       // if everything was filtered out, return an empty array
    +       if(empty($entity_ids)) {
    +               return array();
    +       }
    +    $pathauto_state = db_query("SELECT entity_id, pathauto FROM {pathauto_state} WHERE entity_type = :entity_type AND entity_id IN (:entity_ids)",
    +      array(':entity_type' => $entity_type, ':entity_ids' => $entity_ids))->fetchAllKeyed();
    +
    +    return $pathauto_state;
    +  } catch (Exception $ex) {
    +    if (!defined('MAINTENANCE_MODE')) {
    +      $message = 'The <code>pathauto_state

    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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
15.18 KB
3 KB

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

Status: Needs review » Needs work

The last submitted patch, 278: pathauto-persists-936222-278.patch, failed testing.

joelpittet’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
15.49 KB
1.85 KB

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

joelpittet’s picture

Sweet, Joël 1, testbot 0!

joelpittet’s picture

That works but it clears the list too often, I found a better place for the static cache reset. Hope testbot agrees.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Joel, 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?

joelpittet’s picture

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

Fabianx’s picture

I misread that part, RTBC for #283. Checking for existing entity_types is fine ...

joelpittet’s picture

Fewf, I was sweating there for a second;) Thanks @Fabianx

MiroslavBanov’s picture

This should be a very well optimized query, right?
'primary key' => array('entity_type', 'entity_id')

$path_auto_entity_types = db_query("SELECT entity_type FROM {pathauto_state} GROUP BY entity_type")->fetchAllKeyed(0, 0);

It looks optimized to me. If so we don't need to introduce performance-related changes as mentioned in #267.

Fabianx’s picture

Yes, at least #269 is addressed for this and yes this query is well optimized by the database.

Steven Jones’s picture

Status: Reviewed & tested by the community » Needs work

Sorry @joelpittet but it's not quite 100% yet.

+++ b/pathauto.module
@@ -359,9 +359,47 @@ function pathauto_field_attach_form($entity_type, $entity, &$form, &$form_state,
+  if (!isset($drupal_static_fast)) {
+    $drupal_static_fast = &drupal_static(__FUNCTION__);
+  }
+
+  $path_auto_entity_types = &$drupal_static_fast;

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:

  if (!isset($drupal_static_fast)) {
    $drupal_static_fast['types'] = &drupal_static(__FUNCTION__);
  }

  $path_auto_entity_types = &$drupal_static_fast['types'];

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.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
15.74 KB

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

caspervoogt’s picture

I just tried the patch from #291 and it works.

osopolar’s picture

Patch in #291 works for me too.

joelpittet’s picture

@Steven Jones sorry for the late reply (honestly I thought someone would step up to bat for me) why does :

It needs to be like this

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

joelpittet’s picture

Also I stepped through it numerous times while creating the patch to ensure it worked as I expected.

iamEAP’s picture

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

MiroslavBanov’s picture

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

joelpittet’s picture

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

joelpittet’s picture

FileSize
15.47 KB
975 bytes

@iamEAP So here's your changes minus the try/catch. Is that ok? It sounded like that from #296

joelpittet’s picture

Thanks for fixing my typo on 'unnessasary' I unnecessarily spell that wrong all the time!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC ...

iamEAP’s picture

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

Clemens Sahs’s picture

works great for me

cleaver’s picture

I 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

webservant316’s picture

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

cleaver’s picture

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

MiroslavBanov’s picture

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

webservant316’s picture

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

osopolar’s picture

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

gapple’s picture

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

iamEAP’s picture

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

gapple’s picture

Yes, patch #291 resolved the issue and allowed the necessary update to be run.

cleaver’s picture

@iamEAP, is there an interdiff between 291 and 299? I took a quick look, but did not see one.

gapple’s picture

It looks like #291 and #299 both have interdiffs from #281, but not from each other.
Here is the relevant change:

+++ b/pathauto.module
@@ -365,14 +365,22 @@ function pathauto_entity_load($entities, $type) {
-    $path_auto_entity_types = db_query("SELECT entity_type FROM {pathauto_state} GROUP BY entity_type")->fetchAllKeyed(0, 0);
+    try {
+      $path_auto_entity_types = db_query("SELECT entity_type FROM {pathauto_state} GROUP BY entity_type")->fetchAllKeyed(0, 0);
+    }
+    catch (PDOException $e) {
+      if ($e->getCode() === '42S02') {
+        watchdog('pathauto', 'The pathauto_state table was not found. Be sure that all database updates have been run.', array(), WATCHDOG_ALERT);
+        return;
+      }
+    }
emanaton’s picture

Reroll of #291 for branch 1.2; resolves minor issue of hunk offsets.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/pathauto.module
    @@ -342,9 +342,55 @@ function pathauto_field_attach_form($entity_type, $entity, &$form, &$form_state,
    +  // Save the entity types used statically to avoid unnecessary queries later.
    +  if (!isset($path_auto_entity_types)) {
    +    try {
    +      $path_auto_entity_types = db_query("SELECT entity_type FROM {pathauto_state} GROUP BY entity_type")->fetchAllKeyed(0, 0);
    +    }
    +    catch (PDOException $e) {
    +      if ($e->getCode() === '42S02') {
    +        watchdog('pathauto', 'The pathauto_state table was not found. Be sure that all database updates have been run.', array(), WATCHDOG_ALERT);
    +        return;
    +      }
    +    }
    +  }
    

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

  2. +++ b/pathauto.module
    @@ -342,9 +342,55 @@ function pathauto_field_attach_form($entity_type, $entity, &$form, &$form_state,
    +      if (!isset($entities[$id]->path) || !is_array($entities[$id]->path)) {
    +        $entities[$id]->path = array();
    +      }
    

    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.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
13.25 KB
17.72 KB

Re-rolled based on Pathauto Persist's 7.x-1.2 release and including fixes for #316 and documentation fixes.

Status: Needs review » Needs work

The last submitted patch, 317: 9636222-pathauto-persist.patch, failed testing.

Dave Reid’s picture

Dave Reid’s picture

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

Dave Reid’s picture

Category: Bug report » Task
Issue tags: +Needs change record

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

Dave Reid’s picture

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

joelpittet’s picture

Using it in production since yesterday... fingers crossed but no problems so far.

mattlt’s picture

Using it also. No problems.

Dave Reid’s picture

rv0’s picture

great, working fine here too!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Works fine here, back to RTBC

Dave Reid’s picture

Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

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

  • Dave Reid committed 88db3ee on 7.x-1.x
    Issue #936222 by mvc, quicksketch, joelpittet, Dave Reid, sammarks15,...
dsnopek’s picture

Huzzah! 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! :-)

webservant316’s picture

Concerning #308, can I safely uninstall pathauto_persist and begin to use pathauto-7.x-1.x-dev?

klausi’s picture

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

Dave Reid’s picture

Issue tags: -Needs change record

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

lpalgarvio’s picture

great!!! fantastic!!! thanks :)

webservant316’s picture

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

DamienMcKenna’s picture

@webservant316: Yes. Yes.

jstoller’s picture

@webservant316: Make sure you update Pathauto before uninstalling Pathauto Persist, if you want to maintain your current settings.

Status: Fixed » Closed (fixed)

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

azinck’s picture

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

crystaldawn’s picture

Status: Closed (fixed) » Needs review

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

DamienMcKenna’s picture

Status: Needs review » Closed (fixed)

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

crystaldawn’s picture

For 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