Needs review
Project:
Notifications
Version:
6.x-4.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
8 Mar 2010 at 19:17 UTC
Updated:
12 May 2012 at 22:44 UTC
Jump to comment: Most recent file
Comments
Comment #1
gregglesComment #2
jody lynnHere'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.
Comment #3
jose reyero commentedReally, 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
Comment #4
greggles- 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.
Comment #5
jody lynn@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.
Comment #6
roderikMessing 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.
Comment #7
virtualdrupal commentedCan anyone give an example of how to utilize this patch to get imagecache+imagefield's included in notification emails?
Comment #8
roderik@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).
Comment #9
roderikEh - by now it's better to change some references in the comments. Changed.