NodeAPI view horribly broken for some modules

A slightly modified version. Since nodeapi 'alter' already went in, I've preserved it. This *only* adds the additional hook_node_content_alter() function. If there's a better name for it I'm all ears.

My apologies -- looking at an earlier piece of code, I had mistakenly thought that freelinking used the nodeapi view operation, rather than the filter operation. :-) The problem is that other modules like inline *do* require that context, and without this patch, they would become crippled in the same way that filter operations are -- they have no context.

The reasons lie in the relatively complicated inner workings of nodeapi. Only the first parameter of a nodeapi call -- the node itself -- is passed in by reference. Thus, it would be impossible to pass in $node, $op, then $text as the $text is what needs to be altered.

We COULD pass in $text as the first parameter and node as one of the later ones, but that would be counter to the defined interfaces of nodeapi and cause a lot of needless confusion.

We COULD define a third nodeapi operation for node rendering, one that lets modules alter $node->body or $node->teaser after all the rendering is done. That, though, runs counter to the new rendering model wherein the $node->body and $node->teaser are used just to hold the post-rendering html.

If we were to do that, it might make sense to ditch the nodeapi 'alter' call's current purpose -- rearranging elements, etc -- and use it purely for this purpose. However, that removes the secondary phrase of the rendering that we'd added so that modules could modify eahc others' additions. This option, of the above, is probably the cleanest, although it trades alteration-of-the-content-array for alteration-of-the-rendered-body.

This solution doesn't break the already-defined features in 5.0's APIs, and solves a second problem for themes while we're at it. I can understand if the solution in the curent version of the pathc is not favored for that reason, but I'm hoping and praying that it gets in. I got sucked into FormAPI work shortly after doing the nodeapi refactoring, and didn't spot this issue until actually porting a module.

Fixed an error on priview with submit.module.

In Drupal 4.7, $node->body was assembled as pure text, the way forms were in 4.6. In Drupal 5.0, $node->content is assembled as a structured array, and modules implementing nodeapi op view can add their elements to $node->content. Later, that's rendered into the text that node_view() returns.

Modules like inline need access to the entire post-render text, not just the array of data, to do their substitution work. They *can* use the #after_render callback via the drupal_render() method, but that only gives them the text, not the node object that contains context for the substitution (like the nid, the file array, and so on).

To make it operate like 4.7, we can do one of two things: roll back the node rendering patch, or apply this patch.

If other modules want to pull out a sub-chunk of the node for display in a sidebar block (for example), they would need to call that function to properly process the chunk for display. That was the only real reason for putting it into a separate function.

previews/$previews typo fixed.

Moshe,

If you think that would be sufficient, I'd be willingto roll up a patch that includes $node, $teaser, and $page in flags on the main $node->content[] array. While it would still leave most themes locked out of the game as far as printing subsets of the content array, you're correct that it would make it possible for modules to work around the problem.

In retrospect, removing nodeapi's 'view' and 'alter' operations, and removing #after_render, in favor of two operations (alter the content array, or alter the rendered node) would have been a much better course of action. I really, really regret that wasn't something that I realized before the freeze.

It just occurred to me, though. Wouldn't that mean that we're setting $node->content['#node'] to $node? Does PHP handle those sorts of circular references properly..?

Well, this definitely changes APIs more than the previous one did, but it does so in a 'reasonable' fashion. It's untested, but an example of what Moshe's suggestion might yield:

The node rendering pipeline, with this patch, goes:

1. hook_view()
2. nodeapi 'alter' (where the raw content array can be altered)
3. node body/teaser is rendered
4. nodeapi 'view' (where the post-rendering text can be altered, just like the 4.7 era nodeapi 'view')

This will still force some updates on the part of other modules, but it also makes a relatively clear distinction betwen 'the operation you should use to do post-render filtering,' and 'the operation you should use to add to or modify the node's contents.'

Thoughts?

Here's a slightly updated version of the patch that keeps things simpler and fixes this problem. Please -- really, please! consider the importance of this issue as we go forward with conversions and updates of contrib.

Changes:

1) nodeapi view operates in the standard 'HEAD' way -- modules can add chunks to the $node->content array.
2) nodeapi 'alter' is now fired after the node is rendered from that content array. thus, 'alter' serves as a last-step chance for modules to directly alter a node's text. It makes sense and is much more useful there than as a duplicate of the 'view' op.
3) The unecessary 'after_render' marker in drupal_render() is removed. This just slowed down other rendering operations unecessarily and added another 'magic secret flag' to our already complex structure.

Fixed a minor array/object bug that was keeping upload.module from recognizing files during previews.

At chx's request, a very simple nutshell explanation of why the patch is necessary:

In Drupal 5.0, node content is built as a structured array, then rendered into text after all modules have contributed their chunks to that array. This works great, except for the few modules that actually DO need to modify the entire fully-rendered contents of the node (like inline.module, premium.module, and upload.module). 5.0 currently includes a rudimentary workaround for it (the #after_render flag), but this only gives modules access to the *text* of the node, cutting them off from important contextual information in the node object.

This patch fixes that by removing the relatively useless #after_render flag, and simplifying the node processing into into a 'view' op (for assembling portions of the node), and an 'alter' op (for modules that need to hack at the text after it's been completely rendered).

*poke poke*

Hoping this can make it in. The patch is much cleaner than previous iterations and will make life much, much, much easier.

As in, not broken. :-)

We could leave #after_render in, but it's only used in one location as a workaround for something that this patch fixes better. The modules that would be affected by the change -- those that might be using #after_render OR the 'alter' operation -- have not yet been ported to 5.0s APIs.

The 'alter' op moved so that it could be executed after the rendering rather than before. It was a do-nothing operation.

We could, in theory at least, avoid any changes by adding a FOURTH operation. But this is a very, very edge case. It's an important edge case, but one that the majority of modules (including all the modules that have been converted so far) don't care about. This is an API simplification.

If you think that there's no other alternative, I can look into changing it, but I feel strongly that this is a necessary simplification.

I've attached a new version that leavse #after_rendre intact. If anyone does end up using it, it's still there.

I'm tentatively marking this RTBC again, as there was 1 request (leave #after_render) and 1 question (is moving 'alter' necessary).

Oi, you're correct -- I'd forgotten about paging.module. Thanks for the reminder, chx. That one is very important.