In current HEAD it's only called from node_feed().
In D6 it was called for all node_view() - er, and apparently *not* from node_feed() :-)

CommentFileSizeAuthor
#26 367214-syntax_fix.patch521 bytesAmrMostafa
#22 367214-bye_hook_node_alter.patch2.08 KBAmrMostafa
#20 367214-bye_hook_node_alter.patch2.07 KBAmrMostafa

Comments

moshe weitzman’s picture

moshe weitzman commented 

// Allow modules to modify the structured node.
  drupal_alter('node_build', $node, $teaser);
AmrMostafa’s picture

AmrMostafa commented
Title: hook_node_alter almost disappeared ? » hook_nodeapi_alter almost disappeared ?

I traced it back to this issue #339929: Move node links into $node->content

Do we still need this hook at all?

I think that the proper way to do hook_node_alter()-like in the current system is using hook_preprocess_node:

function MODULE_preprocess_node(&$variables) {
  // $varaibles['content'] is the full rendered node.
}

Does that sound right?

We would need docs for that and more importantly a patch for node_feed (but I believe that needs its own issue).

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

hook_preprocess_node() seems to be thought for themes, while hook_DATA-ID_alter() seems for modules.

It's also true that some modules implement hook_preprocess_node() (or similar hooks), but they normally implement it to change the way something is rendered in the current theme.

Anonymous’s picture

Anonymous (not verified) commented
Title: hook_nodeapi_alter almost disappeared ? » hook_node_alter almost disappeared ?

@yched: Don't you mean hook_node_alter? I couldn't find hook_nodeapi_alter in the documentation or the functions you mention.

@Kiam: I also find no hook_preprocess_node but I find template_preprocess_node, is that what you meant?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: hook_nodeapi_alter almost disappeared ? » hook_node_alter almost disappeared ?

A module can define a function called <module_name>_preprocess_node(), and that function would be used the same.
There are two modules that use a <module_name>_preprocess_<section_to_render>(), with the purpose of changing what it shown in a theme template; one of the modules changes the page title used inside the <head> tag (to say the true, the module defines <module_name>_preprocess_page().

What I mean is that the mentioned hooks have different purposes, and uses.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Closed (duplicate)

This has already been discussed quite a bit here http://drupal.org/node/390774, and a patch there fixes up the doc to address these questions.

So I'm marking this as a duplicate.

AmrMostafa’s picture

AmrMostafa commented
Status: Closed (duplicate) » Active

This is not a documentation issue. The issue you refer to is merely concerned about docs, what we are trying to solve here is whether this hook is still needed or should be removed all together.

Anonymous’s picture

Anonymous (not verified) commented

And it is specific to hook_node_alter and not any other hook_${type}_alter function.

yched’s picture

yched commented

@yched: Don't you mean hook_node_alter? I couldn't find hook_nodeapi_alter in the documentation or the functions you mention.

hook_nodeapi_* were renamed to hook_node_* since the OP :-)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +DrupalWTF

It's definitely silly for there to be two ways to alter the content of a node; one for RSS feeds and one for regular operations.

Is changing the line in Moshe's #1 from node_build to node the only thing required?

AmrMostafa’s picture

AmrMostafa commented

@webchick actually I believe this is a bug in node_feed(), it's effectively broken. I worked on this today at #449718: node_feed() is using the old node building API

yched’s picture

yched commented

The patch committed in #449718: node_feed() is using the old node building API effectively got rid of the last invocation of hook_node_alter().

I don't know if the previous need for hook_node_alter() is now fully undertaken by some other new hook (hook_node_alter always confused me a little), but if so we should remove it from node.api.php :

/**
 * Fiter, substitute or otherwise alter the $node's raw text.
 *
 * The $node->content array has been rendered, so the node body or
 * teaser is filtered and now contains HTML. This hook should only be
 * used when text substitution, filtering, or other raw text operations
 * are necessary.
 *
 * @param $node
 *   The node the action is being performed on.
 * @param $teaser
 *   The $teaser parameter from node_view().
 * @return
 *   None.
 */
function hook_node_alter($node, $teaser) {
}
Anonymous’s picture

Anonymous (not verified) commented

My question would be how many modules port(ed|ing) to D6 use it? What other API could be used to achieve the same action?

brianV’s picture

brianV commented

Just spoke to catch on IRC about this, and he suggested that if it is still popular in contrib modules, it probably isn't worth killing it off. Is anyone able to grep the contrib archive?

AmrMostafa’s picture

AmrMostafa commented

I've went ahead and grepped/awked the contrib repos. Results follow..

25 modules use it.

ajax_select/ajax_select.module
artman2/artman2.module
asset_api/aapi.module
attachment/attachment.module
bingo/bingo_client/bingo_client.module
composite/composite.module
contemplate/contemplate.module
devinfo/devinfo.module
flashvideo/flashvideo.module
guitar/guitar_filter.module
hidden/hidden.module
htmLawed/htmLawed.module
ignore_user/ignore_user.module
inline/inline_upload.module
millennium/millennium.module
node_annotate/node_annotate.module
nodeauthor/nodeauthor.module
nodevote/nodevote.module
revisioning/revisioning.module
simplenews/simplenews.module
stage/stage.module
taxonomy_dss/taxonomy_dss.module
themesettings/themesettings.module
trackback/trackback.module
upload_group/upload_group.module

Many of these modules use hook_node_alter() because they would rather wait after all modules, not because they want to process the rendered node. Those modules will be much happier/better with D7's hook_node_build_alter(), and so the real usage of this hook is actually much less than 25.

bengtan’s picture

bengtan commented

Hi,

Thanks alienbrain for notifying me about this. I'm the maintainer of composite.module.

composite.module only uses 'hook_nodeapi: alter' in D6.x because it waits for other modules to inject their content through 'hook_nodeapi: view'. If there is another way to do this in D7 (ie. maybe hook_node_build_alter() as mentioned in the preceding comment), then composite.module will not need to use hook_node_alter().

However, I can't find any reference to hook_node_build_alter() on http://api.drupal.org/api/group/hooks/7. I presume it just hasn't been documented yet?

bengtan’s picture

bengtan commented

Just had a thought about a compromise approach if the situation about contrib modules is uncertain.

You could remove hook_node_alter() from D7, but do it in such a way that a D7-only contrib module (ie. hook_node_alter.module ) is written that adds this hook back in.

Then, if any contrib modules need hook_node_alter() in D7, they are marked as having a dependency on hook_node_alter.module.

Make the description of hook_node_alter.module clearly state that hook_node_alter() is deprecated and will be removed in D8.

[EDIT]

And/or, if sometime in the future, the usage stats of hook_node_alter.module drops down to zero, you can safely remove hook_node_alter(). Well, maybe.

AmrMostafa’s picture

AmrMostafa commented

Thanks for dropping by :)

Regarding #16, the hook definitely exists. It's not in http://api.drupal.org/api/group/hooks/7 but that's a documentation issue. I will try to quickly summarize the changes you need to be aware of.

In D6, "alter" was available as a way for module authors to work on the rendered node, that is, the final HTML of the node. Some modules also used it when they wanted to make sure that other modules have finished adding their additions to $node->content. In D7, "alter" was replaced by hook_node_build_alter(). However, it was also decided that the node should stay un-rendered (i.e. stay as a FAPI structure in $node->content) as long as possible. So while "alter" received a rendered node, hook_node_build_alter() operates on the same unrendered $node object (the same as in "view" hook"). Generally, that's much more flexible, unless you really want to operate on the final HTML. In that case, it was suggested that modules could use hook_preprocess_node(), which allows operating on the rendered node content.

It appears to me that for your case, hook_node_build_alter() definitely covers your need.

Regarding #17, I don't think we are that desperate :) I'm pretty sure what we currently cover "alter" pretty good. I've skimmed through most modules above and made sure they can do what they do using the new hooks, the ones I wasn't sure of, I e-mailed their authors asking for feedback.

moshe weitzman’s picture

moshe weitzman commented

Don't forget that there is #post_render and preprocess_node where modules can fiddle with fully rendered HTML.

AmrMostafa’s picture

AmrMostafa commented
Status: Active » Needs review
StatusFileSize
new 367214-bye_hook_node_alter.patch2.07 KB

Here is the patch we need. Removes docs for hook_node_alter() and adds docs for hook_node_build_alter().

Inspired by #19, in the sample implementation for hook_node_build_alter() I added an example of using #post_render to act on the rendered HTML.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

$node->content['#post_render'] = array('my_module_node_post_render');> should be

$node->content['#post_render'][] = 'my_module_node_post_render'; so as to not blow away other #post_render callbacks that might have been added.

Thanks for the nice docs.

AmrMostafa’s picture

AmrMostafa commented
Status: Needs work » Needs review
StatusFileSize
new 367214-bye_hook_node_alter.patch2.08 KB

Thanks for the review, updated patch attached.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented

Very nice.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good. Committed to CVS HEAD. Thanks!

AmrMostafa’s picture

AmrMostafa commented
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new 367214-syntax_fix.patch521 bytes

I managed to include a funny syntax error, very tiny patch attached. Thanks to Berdir for reporting this.

cburschka’s picture

cburschka
they
commented
Priority: Normal » Critical

This syntax error has already made PIFR reject at least one patch due to a broken head... best fix this ASAP.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed follow-up.

Status: Fixed » Closed (fixed)
Issue tags: -DrupalWTF

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +DrupalWTF

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.