Computed fields should compute on $op='presave' and/or $op='validate'

Interesting... while I think this change should be fine, I'd like to run a couple questions by yched first. So this one is pending a couple answers.

I got here because I have the same problem with Computed Field tokens and auto_nodetitle.
Thanks for the fix matt, even if it is not approved yet :)

Cheers !

I understand the fix but I cannot get it to work for me. After I change the code all I get is a blank page after hitting save.

I changed the following:
FROM

case 'insert':
case 'update':
  _computed_field_compute_value($node, $field, $node_field);
break;

TO

case 'presave':
  _computed_field_compute_value($node, $field, $node_field);
break;

Did I miss something? I am using computed_field 6.x-1.0-beta2 with token 6.x-1.12

Thanks

Just wanted to give some feedback about this approach: we applied the "patch" and everything works as expected. I have this on a live website.

@what4software, that is because you did it wrong :)
From

case 'insert':
case 'update':
  _computed_field_compute_value($node, $field, $node_field);
break;

It should be:

case 'insert':
case 'update':
case 'presave':
  _computed_field_compute_value($node, $field, $node_field);
break;

And here is the trivial patch against the latest beta3.

Updating the status so maybe we can get some maintainer feedback.

patch is good for me too.

Patch works for me, too (had the same problem with auto_nodetitle, token and one single computed field in the title)

Subscribing

Subscribing.

patch was successful for me as well - thanks for the great work!
i'm glad i came across this thread

May be this patch will solve the problems like described in this topic: http://drupal.org/node/350404

I have a strange thing happening. After applying the patch I get memory exhaust in the coming module (content.module - comes lexicographically after computed field). Memory is not really a problem - I have increased it and looks like there is a loop or recursive call. That is, because the needed memory is doubled whenever I double the memory limit. Please mind, however, that the code has run OK before applying the patch, but, of course, could be used after second saving (while editing) - the problem the patch comes to solve in the first place. Following is the code I am running; it is a double inner join query. It is used in order to get the title of an object of type org (organization). An og object refers to an org, and a wiki node is bound to an og node; I want to save the name of the org in each wiki node in order to use it in a pathaufo pattern.

if (!$node->nid) node_save(&$node);

$org_title = db_result(db_query("
SELECT rev.title FROM node_revisions as rev
INNER JOIN content_type_og as og
ON og.field_group_org_nid = rev.nid
INNER JOIN og_ancestry as ancestry
ON ancestry.group_nid = og.nid
WHERE ancestry.nid = %d",$node->nid));

$node_field[0]['value'] = $org_title;

Please note that I tried also "node_save($node);" instead of "node_save(&$node);" - no difference; system crashes.

Any help will be appreciated, thanks.

patch worked for me.

It's been waaaay too long. :/ While adding it to the "presave" op should work, it would also result in the computation happening twice per node save. (presave/insert or presave/update). Probably not a big deal for most people, but for those with "expensive" computations that pull from outside sources or DBs that might be rough.

I committed a change from insert/update to presave alone, which will show up in the next dev download on the project page. I think that should do it unless there is some sneaky time when presave isn't run. After I get confirmation this is working and look through the queue a bit more here we'll get this thing out of its eternal beta.

I have a similar problem as Amir in #36. Calling node_save($node) in the field computation code causes a segmentation fault. I haven't digged deeper on the issue, but this started to happen after upgrading Drupal from 6.17 to 6.19, Computed Field to 6.x-1.0-beta4 and dozens of other modules to their latest versions.

Edit:
This is my super-elegant workaround:

global $iAmAlreadySavingSoPleaseDoNotSaveMeAgainSir;
if (!$node->nid && !$iAmAlreadySavingSoPleaseDoNotSaveMeAgainSir) {
    $iAmAlreadySavingSoPleaseDoNotSaveMeAgainSir = true;
    node_save($node);
}
// and so on...

Same Issue for me. System crashes while I try to create a new node.

This is the computed code:

if (!$node->nid) {
node_save($node);
}
$node_field[0]['value'] = $node->nid;

And this the display format:

$display = l(t('Visualizza la mappa per questo evento'), 'mappa_evento/'.arg(1));

I use drupal 6.16 and computed-field 6.x-1.0-beta4.

I've the same code on a computed field in a site with drupal 6.13 and computed-field 6.x-1.0-beta3 without problems

this may be what caused #945712: upgrade from beta3 to beta4 causes Error 324 (net::ERR_EMPTY_RESPONSE): Unknown error.

@Moonshine

It's been waaaay too long. :/ While adding it to the "presave" op should work, it would also result in the computation happening twice per node save. (presave/insert or presave/update). Probably not a big deal for most people, but for those with "expensive" computations that pull from outside sources or DBs that might be rough.

Unfortunately leaving the insert hook out DOES create problems in some situations. To be specific, we might need the node id available. I actually spent more time than I like to admit with debugging a problem caused by the latest update (in beta-4 and the dev-version).

In my setup computed field is used to create a kind of unique field from the node id. This unique id is then used in the nodetitle (with token + autonodetitle) and the alias (token + patchauto). During the creation an email will also be sent.. and after beta-4 this link no longer worked (because the computed_field-token had not been specified in time). Updates to the node would then sent emails with the correct link, though.

Anyway, in my case I wasted so much time searching for the problematic module, as I first couldn't find when the problem started. I suspected token, messaging, notifications, team_notifications, pathauto, module weigths and what not, before I found the actual cause: computed_field no longer updating it's value during insert!

Problem in short:
Node id is not available during "presave", if the node is new. However, the nid IS available in the insert-hook of new nodes. In other words:

New node hooks
* .. other hooks..
* presave (no nid)
* insert (got nid!)

Old node update hooks
* .. other hooks..
* presave (got nid!)

Proposed solution
If we need to use the nid during code evaluation, and want to avoid duplicate runs, we could just alter a few lines in the field_hook:

-      _computed_field_compute_value($node, $field, $node_field);

+		if($node->nid) _computed_field_compute_value($node, $field, $node_field);
+    case 'insert':
+     _computed_field_compute_value($node, $field, $node_field);

I tested this, and for me it works without problems. Got nid, no duplicate execution and therefore there should also be no infinite looping. Please test the attached very simple patch againt the latest dev-version.

The first line could be skipped (=run during both insert and presave hooks might also have some justification?).

EDIT: set this to major, as this "new" logic breaks any all sites using $node->nid in the evaluated php (and need it to work during first insert).

@jarnoan

Edit:
This is my super-elegant workaround:
global $iAmAlreadySavingSoPleaseDoNotSaveMeAgainSir;
if (!$node->nid && !$iAmAlreadySavingSoPleaseDoNotSaveMeAgainSir) {
$iAmAlreadySavingSoPleaseDoNotSaveMeAgainSir = true;
node_save($node);
}
// and so on...

<smartass-mode>

A global variable would stop the execution of any node save during the page load, meaning that it would only work for the first node, and not during any subsequent node saves (for other nodes). IMO this would be a bit more "elegant":

if (!$node->nid && !$node->dont_loop) {
    $node->dont_loop = 1;
    node_save($node);
}

</smartass-mode>

Good point. also got a WSOD when trying to use node_save($node) in the computed field code.

Thanks bibo
I am use -beta4 and #44 solution was great for me.
I was not able to get NID when creating new node. Now it works like it should be.

Patch in #44 in testing on a dev site here: seems to work although I've not tested heavily to see if there are any other upshots/side-effects to the change. The computed field I'm working with uses $node->nid and a call to l() to add a link to the node itself (amongst other things). A simple version:

  $short_text = $somestuff;
  $node_field[0]['value'] = l($node->title, 'node/'. $node->nid) .'<br />'. $short_text;

Using the patch in #44 means the nid is now available for use in a link. The only issue I have now is that l() generates a simple /node/nid link, rather than an aliased path (which I assume pathauto generates AFTER computed field's hook_field/presave is called). Subsequently editing the node results in the correct URL alias returned from l(). Not really part of this issue, mind... but better than either producing a broken /node/ link or causing a 500 server error!

subscribing, hope beta5 will be out soon as beta4 corrupts the site

The "fix" committed in #38 caused many sites to break (including mine).

My simple patch in #44 seems to fix the problem. Setting to RTBC as per #46 and #47.

Subscribing

Hmmm, I wish this would have surfaced while it was sitting in dev. :( Sorry for the delay, but until days have 36 hours...

The real problem is that this original issue was created to get Computed Field running during "presave" so that it would return tokenized values for Automatic Nodetitles. "Presave" would be ideal as it's run early and is triggered before any "insert" or "update". However, as @bibo notes, "presave" is actually too early to have the the NID assigned -- which I'm sure is a problem for some computed fields.

I'm afraid there really isn't a magic solution that I can see that will satisfy both the original issue and still have NID available to the the computed code without redundant calls to the field computation, which I think we need to avoid.

The patch in #44 will allow computed code to always have the NID available (as it had with the prior update/insert method), however it won't use presave on initial node creation which doesn't help the original point of this issue.

In the end I think we should go back to the insert/update method as Computed Field traditionally had, at least for the Drupal 6 version, for a few reasons:

1. Having NID available, as it was for a long time, is going to be a dealbreaker for some computed fields.
2. The Automatic Nodetitle module actually allows for custom PHP code also, so there is still a way for people to accomplish whatever they want without using tokens from computed fields. (or potentially computed fields at all) I know it's not the "elegant" way but it is doable.
3. We want to avoid redundant calls to the field computation, which would be necessary for a "presave" call and a call with NID available on initial node creation.

I'll be rolling back this change in the dev branch and tagging a new beta 5 for now. If anyone has a magic answer that I'm overlooking, feel free to re-open.

When I get some time, I will look at how this works in the D7 version using the new Field API though. Perhaps there we'll be able to have it both ways.

I've moved things back to insert/update and tagged a beta5 release which should show up soon. Closing this issue for now, but as I mentioned, re-open if I'm overlooking something useful here.

I will be hitting the issue queue in the next couple weeks and pulling together a non-beta 6.x-1.0 release so that I can focus on 7.x a bit. Sorry for the delays everyone, I truly wish I had more time. At least CVS on drupal.org is going away this week, THAT is very exciting for me. I've never felt comfortable with it, and I can't wait for GIT! :)

I have upgraded Computed Field from 6.x-1.0-beta4 to 6.x-1.0-beta5, but I still have the $node->nid not available in the Display format code (#982038: $node not available in the Display format code).

just a comment - maybe other people coming to this issue will be in the same position:

i can see the reasoning behind undoing this, but the main reason i was using a computed field in the first place was to get code out of the Automatic Nodetitle and out of the web interface in general - i was using a computed_field_FIELDNAME_compute() function to generate the title. the workaround that fit better was to do the actual computation in a hook_nodeapi() function on 'presave', and then leave the computed_field function there but do nothing in it, to avoid redundant calculation of the field value.

but that only worked OK for me because i didn't need the $field or $node_field parameters. maybe a way to help people deal with this problem would be a presave hook - computed_field_FIELDNAME_presave() ? or a more general hook run by computed_field_field()? or if there is already a CCK hook that could be used to the same effect that i've overlooked.

Sorry to bring this one to life.

Started to try computed_field, which I generally avoided by creating my custom modules. I came to this issue because I wanted auto_nodetitle to work with the tokens as well, although I understand that most things can be achieved on the PHP side of nodetitle, there's bit which is also not handled on this great module which is field validation.

Say I set it to required, I want it to validate that the computed field has something in it, and that's not working. It's kind of a separate issue but related to this one.

I understand the need of having $node->nid on current computed tokens but can a change be considered to also include validation, and why not, presave.

I was thinking of exposing $op to the custom_field php code. By doing that, you could query it on your code and act accordingly, whether having nid or not, or on each action.

I know this may make the code a bit more complicated, but well, isn't flexibility the main thing for this module?

Another idea, which may be backward compatible, and not one I'd like, is to store a separate computed code for presaving/validation.

I am not sure if I will follow this or just jump into doing some custom module development, but if this can be considered, I don't mind submitting a patch into it.

Also, thinking of not breaking current sites, I am thinking that if something like this might be considered (having $op), an update routine to add if ($op =='insert' || $op == 'update') { current code } to all existing computed_fields would sort any backward compatibility issues.

Marking #1081952: Compute values during "insert" and "update" rather then "presave" as a duplicate.

So it turns out that in D7, even though hook_field_insert() and hook_field_update() are being used, the field tokens are available (with a workaround for now - see #1280100: computed_field_tokens() does not check if $type == 'node' and blindly replaces tokens for all token types for details). So no re-jigging the API is not necessary for that branch. (Or if it is, and I'm missing something, please tell me.)

For D6, however, I'd be more than happy to accept patches that change the API to include $op. That's a good idea.

Due to the Drupal core life cycle policy and security support advisery, Drupal 6 is no longer supported. So issues for Drupal 6 cannot be longer maintained. The project maintainer has asked for closing all D6 issues to clean up the issue queue. Feel free to reopen the issue if required or set it to "needs to be ported" and latest D8 dev version, if the issue discusses a still missing feature which can be implemented in the D8 version.