I'm just now trying to use this feature, so I could be doing something wrong or looking in the wrong place, but it appears that the documentation for the 'fields' op in nodeapi has no functionality that ever calls it. Looking at node_load in node.module, I can see that the fields that are SELECTed from the node table are hard-coded, and no attempt is made to see what fields other modules want loaded.

If this has been deprecated, please make sure that the documentation reflects this. Otherwise, it wouldn't be too dificult to invoke the nodeapi fields hook and implode them into the sql query.

CommentFileSizeAuthor
#12 hook_nodeapi_fields.patch6.48 KBjose reyero
#7 fields.patch910 byteskilles@www.drop.org
#5 node.module.fields_patch3.35 KBjavanaut

Comments

javanaut’s picture

javanaut commented

I can now see that this functionality was abandoned quite a while back. There are so many segments of code that would prevent this from working, I would like to propose a change in the functionality.

Instead of the previous format:

  case 'fields':
    return ('foo', 'bar');

I would like to propose the format:

  case 'fields':
    return ('foo' => '%d', 'bar' => "'%s'");

This would allow node_save to actuall INSERT/UPDATE as needed. If anyone thinks this is worth pursuing, I'll submit a patch.

killes@www.drop.org’s picture

killes@www.drop.org commented

I guess it is my fault that I didn't document it. I removed this when implementing the revisions patch. Where would your changed code be called?

javanaut’s picture

javanaut commented

It would be called from node_load and node_save.

killes@www.drop.org’s picture

killes@www.drop.org commented

I am sceptical about it. This i sjust another way of harcoding the values (which is done now).

javanaut’s picture

javanaut commented
StatusFileSize
new node.module.fields_patch3.35 KB

This is a start on it. Currently, the comment module still defines the 'fields' op in comment_nodeapi(), so things will be ugly for you until it's updated or removed. I noticed that you hard-coded the comment parameter in node.module.

This patch doesn't touch the revisions table, but it would be trivial to do so in the same way that the node table is updated. My only concern is that developers wouldn't know about having to add the same columns to the node_revisions table that they do in the node table.

Please give feedback.

javanaut’s picture

javanaut commented
Status: Active » Needs review

updating status

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new fields.patch910 bytes

Here is an alternative patch that simply removes the fields part from comment.module.

javanaut’s picture

javanaut commented

I'm not sure what you mean by this. Are you suggesting that the 'fields' feature is removed from drupal?

killes@www.drop.org’s picture

killes@www.drop.org commented

Yes. Did you need it somewhere? It was used to gather the fields that shoudl be saved in tzhe node table. Those are now hardcoded into node.module. thi smakes sense, because the number of fields was fixed anyway.

javanaut’s picture

javanaut commented

My understanding of this feature was as a higher performance version of what's done using the load/insert/update nodeapis. This performance comes at the expense of having to alter the node table's structure (thus losing forward compatibility), but spares extra queries for each node load/save operation. It also spares a lot of code writing.

I used this a long time ago, and got a lot done in very short time. I'd like to see it stay.

moshe weitzman’s picture

moshe weitzman commented
Title: nodeapi 'fields' op no longer works » nodeapi 'fields' op should be revived
Status: Needs review » Active

i'd like to see this feature revived one day.

jose reyero’s picture

jose reyero commented
Status: Active » Needs work
StatusFileSize
new hook_nodeapi_fields.patch6.48 KB

Ohh, I've just seen this one and I do need that 'fields' op again :-(

This is really more complex with all that revisions stuff, and I know this patch needs still some work but the idea is as follows:

- In node_save:
We build a $tables array hat has all the information to be saved for node and node_revisions table. This one will be passed by reference on hook_nodeapi($node, 'fields', $data) so any module can add/remove/modify fields or field values for these two tables.

- In node_load (this needs some more work)
Select node.* and then, when loading a revision we throw a second query for that revision data that will overwrite the node data.

This way modules can add fields in node table and they may be into revisions table too.

David Lesieur’s picture

David Lesieur commented

I did a quick test of Jose's patch and it seems to work well. I was able to load and save extra node fields. The fields op is certainly required to avoid additional queries in i18n just to retrieve a node's language.

The patch requires the hook to be defined as: 

function hook_nodeapi(&$node, $op, &$arg)

This means $arg cannot have a default value, but I'm not sure whether this is a problem.

Mirrorball’s picture

Mirrorball commented

If 'fields' no longer works, it should be removed from the documentation. It's confusing.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Needs work » Active

I've applied my patch from #7, Jose's patch was not ready yet and it is too late in the release cycle. Mirrorball, where is that docs, you speak about? Marking active till this is sorted out.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Active » Fixed

docs found and removed.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
chx’s picture

chx commented
Title: nodeapi 'fields' op should be revived » nodeapi 'fields' op got lost
Version: x.y.z » 6.x-dev
Assigned: Unassigned » chx
Status: Closed (fixed) » Postponed (maintainer needs more info)

My 4.5 site was performing quite well with 'views' field in node table. I have even written a very nice quickstats module (fireangel is working on releasing it) which collects views in a small table and rolls them back to node table in one UPDATE statement per cron run so there is no big performance loss by updateing node all the time.

As this functionality is no longer possible, this is a bug, a performance bug. A patch is forthcoming. I am actually itching to set it to critical because we lost functionality, a very useful one.

chx’s picture

chx commented
Status: Postponed (maintainer needs more info) » Active
chx’s picture

chx commented

Regarding forward compatibility, I just updated from 4.5 to 5.0 and aside from update 175 which tried to urldecode 300 000 aliases, I had no problems and my custom fields stayed intact. I consider that argument is moot. Of course, there is always a small risk, but I am perfectly happy to review my codebase if core node table also gets a node.views field. I am not going to do a very costly join or extra query though every time I want to display a short version of node so having my own field(s) in node (there are more than one, actually) is crucial.

moshe weitzman’s picture

moshe weitzman commented
Status: Active » Closed (duplicate)

there is now a patch for node_load and node_save to work off of schema api which would bring back this feature