The current code for [node-body] token doesn't handle cck fields well (nor other similar content that is added in with normal node rendering).

Forthcoming patch does two things:

  1. First, it sets the teaser and body-raw for situations where those are important.
  2. Then it does a more complete node_load (it seems the object passed in is not a full node) and it renders it using code from node_view.

This provides much better notification content for sites.

This has the drawback of being somewhat slower. Fortunately, token module caches the tokens per object and notifications are sent in batches which can take advantage of that caching.

Comments

greggles’s picture

Status: Active » Needs review
StatusFileSize
new1.5 KB
jody lynn’s picture

Version: 6.x-2.x-dev » 6.x-4.x-dev
StatusFileSize
new1.56 KB

Here's a new patch that just uses node_view, for both node-body and node-teaser. Still needs some testing.

We actually need this patch for groups.drupal.org. Currently when someone posts an event, you get notifications that don't contain the date of the event (which is a cck date field). Makes it hard to know when meetups are happening.

jose reyero’s picture

Status: Needs review » Postponed (maintainer needs more info)

Really, none of the patches look very good to me. Some issues:

- If the node is not fully loaded, maybe we should see why
- The 'node-body' token has some uses too, I wouldn't like to drop it
- The node_alter hook is only invoked from node_view(), which means modules can add all kind of HTML crap there, looks like 'poluting' the token.
- Using node_view() is not adequate, as it formats the whole thing with links, etc... I rather use our own theming function, or none, for nodes in messages.

So what I've done for now is adding a new 'node-content' token, which is just the rendered built node content, only in 4.x branch for now.

@greggles,
If this is any use for g.d.o. we can backport this token for 2.x (current stable)
Optionally we could add our own theme function so you can do any aditional formatting there

greggles’s picture

- If the node is not fully loaded, maybe we should see why

I'm not sure. My original reason was 100% related to its use in messaging/notifications so the problem is somewhere in these two modules. Jody's case is related to signup.

- The 'node-body' token has some uses too, I wouldn't like to drop it

OK, so we should call these "node-body-full" and "node-teaser-full" or something similar.

- The node_alter hook is only invoked from node_view(), which means modules can add all kind of HTML crap there, looks like 'poluting' the token.

As the old saying goes, one man's trash is another's treasure. We need the HTML crap (and it gets stripped later with html_to_text).

- Using node_view() is not adequate, as it formats the whole thing with links, etc... I rather use our own theming function, or none, for nodes in messages.

I'm not sure I understand this.

jody lynn’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new1.19 KB

@jose
The new token looks ok except that when I tested it (with pathauto also enabled) your new node-content token was empty. It seemed that it was not that the $node was not fully loaded after all, but that it contained an incomplete $node->content array (perhaps because the build_mode for the $node is 'token' CCK loaded content differently). Unsetting $node->content (patch attached) before node_build_content did the trick.

I do think backporting it for g.d.o. would be great.

@greggles
The g.d.o. case is not actually related to signup, that was just Alex's wrong theory. It's purely the need to show the CCK date field on notifications emails coming from g.d.o.

roderik’s picture

Priority: Normal » Major
StatusFileSize
new11.68 KB

Messing with node bodies is hairy. So whatever you do, you get hairy code that needs to tiptoe around stuff.

A couple of things:
1)
The reason that the $node->content array is incomplete, is that CCK's implementation of the hook (i.e. content_token_values()) throws it away. (Something which yched has just fixed in the to-be-rolled-soon CCK -dev version, by the way.)

2)
One must never call node_prepare() (or node_build_content()) twice on the same node, because the body contents can end up in a bad state from that. (Search for 'node_prepare twice' for explanation.)
One must also never call check_markup() on $node->body again, after node_prepare() has already run, for the same reason.
(This is my reason for setting Priority = major. You have a potential security hole here, though I'm not reporting it as such.)

3)
lf token_get_values() was called while $node is still being assembled (i.e. in a hook_nodeapi($node, 'view') implementation), you get 'fun' recursion issues by calling node_build_content() again from here.

I am not aware of any issue #3 in official contrib code yet, though it was the reason I got to looking into this issue. (I patched webform to work with tokens, which prompted #3. All my webforms were being submitted twice when notifications_content is enabled.)

By the time I'd finished untangling above issues, I'd come up with the attached patch. (The supporting function may be a little more generic than needed, but that's the result of re-testing my assumptions 3 times :)) It works for me in several scenarios, with and without the newest CCK-dev patch.

I think this may actually warrant a separate little module, decoupled from notifications, because:
- more people want these tokens: #181546: Token for body of node. Actually, the messaging_template module screams for this token too.
- there are bound to be more bug reports related to odd behavior which is triggered by certain configurations. ("My ... is missing from the 'node-content' token!")
- the variable is needed; it needs to be set to 0 to prevent recursion & nasty resulting issues on some systems (i.e. with webform-tokens enabled); but it needs to be set to >=1 for 'node-content' to actually be useful (i.e. different from the node-body token. It needs to be >=1 for CCK fields to be present in the content.)
- it needs documentation. With lots of warnings.

I can pursue that if you want to.

Unless, of course, you tell me I am way 'overengineering' this. Or you don't want to require that separate module.

virtualdrupal’s picture

Can anyone give an example of how to utilize this patch to get imagecache+imagefield's included in notification emails?

roderik’s picture

StatusFileSize
new12.34 KB

@virtualroberts: bug me if still necessary. (Work doesn't enable me to spend as much time in issue queues as I'd like, keeping up with the patches that never get committed.)

Included a patch with an extra one-line fix and an extra comment block (as compared to #6).

roderik’s picture

StatusFileSize
new12.46 KB

Eh - by now it's better to change some references in the comments. Changed.