node_feed() seem to be still using the old node building API

So it's also the only place where we are calling hook_node_alter() (please see #367214: hook_node_alter almost disappeared ? for more details on that one) and Fields or any additions made in the node building hooks never make it to the feed items.

CommentFileSizeAuthor
#6 node_feed-449718.patch18.93 KBAmrMostafa
#4 node_feed-449718.patch16.05 KBAmrMostafa
#2 node_feed-449718.patch3.42 KBAmrMostafa
#1 node_feed-449718.patch3.61 KBAmrMostafa
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
Status: Active » Needs review
FileSize
node_feed-449718.patch3.61 KB

Patch attached. It depends on #449614: "read more" gone (node.module is still using hook_link for that) for the read more link.

This patch also changes something from the current (and also in D6) node_feed() behavior in that it includes the node links. This actually happens by default, since links are now part of the node content structure and so it gets included in the rendered content.

We could unset 'links' to retain the old behavior where links where not included in the feed, but why, this is a great feature because modules can easily check node->build_mode == NODE_BUILD_RSS and add only links they wish to be included with the node item in the feed. However, it's a bug that modules which provide links do not make this check, like taxonomy and comment modules who always set links with little or no checking for the build mode. I prefer that we handle these checks in other issues for those respective modules.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
FileSize
node_feed-449718.patch3.42 KB

Better patch (gotten rid of "read more" cruft, this was taken care of using a build_mode check in #449614: "read more" gone (node.module is still using hook_link for that))

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Needs work

Looks like a very nice cleanup. This brings fields back into RSS feeds as they are in D6 with CCK.

I think the existing hook_node_build_alter() suffices and we can deprecate $extra = module_invoke_all('node_rss_item', $node);. Modules that use that operation will use the alter hook along with the build mode.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
Status: Needs work » Needs review
FileSize
node_feed-449718.patch16.05 KB

New patch, gets rid of hook_node_rss_item() as suggested in #3 but uses hook_node_view() instead of hook_node_build_alter(), I hope that is Ok, Moshe.

Modules now have rss_elements and rss_namespaces to act on at NODE_BUILD_RSS.

I also added the needed checks for modules to not add links to nodes when $node->build_mode == NODE_BUILD_RSS which takes care of #1 (3rd paragraph).

Status: Needs review » Needs work

The last submitted patch failed testing.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
Status: Needs work » Needs review
FileSize
node_feed-449718.patch18.93 KB

Couple of minor fixes, plus tests! :)

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

The patch got much bigger since #2. Is that really needed? This patch is fine as an interim status, but I really would love if alienbrain or other would take on #409750: Overhaul and extend node build modes

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented

Here is what this patch currently does:

  1. Fixes node_feed() making it use the correct node building API which happens to also simplify it a lot. This is the most important and is the main focus of the patch.
  2. Adds the appropriate handling for NODE_BUILD_RSS in modules which currently do not do this.
  3. Removes hook_node_rss_item() in favor of NODE_BUILD_RSS in which modules can add extra RSS elements or namespaces, with documentation. This is also consuming a considerable space in the patch
  4. Adds tests.

2 and 3 are why the patch has grown bigger. But I think they are needed and are pretty simple in their logic.

I will try to get into #409750: Overhaul and extend node build modes when I get a chance. Build modes are definitely underutilized in core, but what's actually problematic is that they are not being checked in the node building hooks implemented by modules.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

@alienbrain - it is true that the build modes are not checked ... hav you used cck in drupal6 - specifically the display fields tab? that lets the *admin decide* what fields should show up for each build mode. thats where we have to get to in core, IMO.

i will take another look at this patch.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Ran through the code once again and this really is an admirable modernization of our rss feeds. For example, field information may now be included in the item body (cck UI gives admin a lot of control here).

Dries’s picture

Dries CreditAttribution: Dries commented

I'm delegating this patch to Moshe. I will commit this patch (after a quick review) when Moshe thinks it is ready.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

@Dries: Moshe said it's RTBC in #10. What more is there to delegate in #11, 25 hours later?

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Oops, I had opened this issue on Friday night, and didn't bother to reload it since. As a result, I missed the most recent updates.

Reviewed and committed. Thanks!

yched’s picture

yched CreditAttribution: yched commented
Status: Fixed » Active

Note that this patch effectively got rid of the last invocation of hook_node_alter().
From #367214: hook_node_alter almost disappeared ? :
"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() :-)"

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) {
}
dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Active » Fixed

yched: that's a job for #367214: hook_node_alter almost disappeared ? not here. ;)

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented

I updated the upgrade docs accordingly.

Status: Fixed » Closed (fixed)

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