Blog module not implementing hook_view() correctly

The node being called needs to be returned not passed by reference.

it's got a patch

i'd love some reviews...

I think this may be a misdiagnosis of the problem. Some filters (e.g. the basic html filter, url-to-link maker) are applied, although BBcode changes are not in evidence. However, patch doesn't make bbcode work for me. I think the issue may be with how bbcode module does its translations.

Here's my test node:

I [b]need[/b] this to work.

And I need <em>this</em> to work, too.

What about my site? http://www.outlandishjosh.com

Viewing this node with the base "filtered html" setting + bbcode gives the expected result. Viewing source, here's what's being output to the browser:

<!-- begin content -->
<div id="node-1" class="node">



      <span class="submitted">Thu, 12/28/2006 - 00:06 — <a href="/~joshk/drupal5/?q=user/1" title="View user profile.">joshk</a></span>
  
  <div class="content">
    <p><p>I <span style="font-weight:bold">need</span> this to work.</p>

<p>And I need <em>this</em> to work, too.</p>
<p>What about my site? <a href="http://www.outlandishjosh.com" title="http://www.outlandishjosh.com"><a href="http://www.outlandishjosh.com">http://www.outlandishjosh.com</a></a></p>
</p>  </div>

  <div class="clear-block clear">
    <div class="meta">
        </div>

          <div class="links"><ul class="links inline"><li class="first comment_add"><a href="/~joshk/drupal5/?q=comment/reply/1#comment_form" title="Share your thoughts and opinions related to this posting." class="comment_add">Add new comment</a></li>
</ul></div>
      </div>

</div><div id="comments"></div><!-- end content --> 

Notice that the [b] has turned into a <span> declaration with inline CSS style. I think that may be the issue. Here's the source from the rss feed:

<?xml version="1.0" encoding="utf-8"?>
<rss version="2.0" xml:base="http://vitus.chn.comcast.net/~joshk/drupal5" xmlns:dc="http://purl.org/dc/elements/1.1/">
<channel>
 <title>Drupal - </title>
 <link>http://vitus.chn.comcast.net/~joshk/drupal5</link>
 <description></description>
 <language>en</language>
<item>
 <title>Test</title>
 <link>http://vitus.chn.comcast.net/~joshk/drupal5/?q=node/1</link>
 <description>&lt;p&gt;&lt;p&gt;I &lt;span style=&quot;font-weight:bold&quot;&gt;need&lt;/span&gt; this to work.&lt;/p&gt;

&lt;p&gt;And I need &lt;em&gt;this&lt;/em&gt; to work, too.&lt;/p&gt;
&lt;p&gt;What about my site? &lt;a href=&quot;http://www.outlandishjosh.com&quot; title=&quot;http://www.outlandishjosh.com&quot;&gt;&lt;a href=&quot;http://www.outlandishjosh.com&quot;&gt;http://www.outlandishjosh.com&lt;/a&gt;&lt;/a&gt;&lt;/p&gt;

&lt;/p&gt;</description>
 <comments>http://vitus.chn.comcast.net/~joshk/drupal5/?q=node/1#comments</comments>
 <pubDate>Wed, 27 Dec 2006 16:06:29 -0800</pubDate>
 <dc:creator>joshk</dc:creator>
 <guid isPermaLink="FALSE">1 at http://vitus.chn.comcast.net/~joshk/drupal5</guid>
</item>
</channel>
</rss>

This is the same with or without the patch. It seems that the data is being put into the feed, and it's really a matter of the receiving end of the rss being able to parse the span with inline style data. Firefox certainly doesn't.

Yeah, I just looked into it and hook_view says that you should modify the passed-by-reference node AND return that node. It's a bit odd, but this issue should be against blog_view() and maybe should look like this patch. Is this only with blog.module? Do page taxonomy feeds work okay? Please recatgorize if this patch doesn't help or you feel it's not the right approach.

With that said, I cannot reproduce this error on a fresh copy of 5.x-dev using PHP 4.4.4, so I'll try and do a test on PHP 5 later if this patch doesn't resolve the problem.

See http://api.drupal.org/api/HEAD/function/hook_view to see how the example does it.

Is chunk required?

-  return node_prepare($node, $teaser);
+  $node = node_prepare($node, $teaser);
+  return $node;

It's prone to error, IMO, as people will tend to optimize that code to 'return node_prepare()'.

Dries, I agree it does seem really awkward. Since this is a change from 4.7 it really ought to be documented in the module update guide as such.

Patch without the chunk. Aint it beautiful? One changed byte.

forngren, that won't work because node_prepare() doesn't take $node by reference.

this fixes the poll module which also doesn't take $node by reference. it also adds a comment to explain the purpose of the "extra" operation.

please pardon the DOS line endings.

@forngren/Dries - This chunk is definitely intentional and required because of the explanation drewish gave above. Look at the http://api.drupal.org/api/HEAD/function/hook_view example and docs. We need to set $node to change the referenced $node and then return it to be consistent with what the docs say, so IMO my patch still stands.

-  return node_prepare($node, $teaser);
+  $node = node_prepare($node, $teaser);
+  return $node;

i hope what robroy meant was his patch with my poll module addition stands ;) i'd really rather not have to try to re-explain it in another issue.

@drewish, Yeah, that poll inclusion is good. In theory it should be in another issue, but it's pretty small so it's probably alright here. Small nit: there needs to be a period at the end of the blog_view comment you added.

Oh and you can download Notepad++ to easily convert patches to Unix line endings.

robroy, i'd almost added a period but the bread crumb comment above didn't have one and i couldn't remember what the "official" style was.

usually i use dos2unix to convert line endings but i hadn't had a chance to dig up a copy on this computer.

@drewish - The official style is to have a period at the end, so any added comments should conform to this style IMO, even if surrounding comments do not.

@earnie - Please see http://api.drupal.org/api/HEAD/function/hook_view as I stated above. "The passed-by-reference $node parameter should be modified as necessary and returned so it can be properly presented by theme('node', $node)."

earnie, it is used. take a look at node_build_content(). my original proposal in comment #3 was to just have node_feed() work the same way.

@earnie - Yes, I understand what you're saying, but this change is to get existing view hooks to work as described on the api docs. Your issues for hook_view and hook_prepare would be two separate API changing issues for D6 as the current implementation seems to require both pass by reference and returning that node for certain direct calls to hook_view as stated in the docs.

We need to get this straight as ambiguity can lead to nasty XSS problems on PHP 4.

Either we fix the hook_view docs and apply drewish's patch in #3, making a return $node mandatory for hook_view (my preference) or we rewrite node_build_content and node_prepare to work with the a reference to $node.

First of all, there is no easy solution here, we are late in cycle and need to change something. My 2c is on http://drupal.org/files/issues/node_rss.patch and fixing hook_view documentation. Less references are always better.

I agree with chx (#26). Let's go with that patch, and make sure the documentation gets fixed. We're in a code freeze, but it is not too late to fix critical bugs elegantly.

I agree with that. I did a quick search to make sure that no other invoke calls to hook_view expect the referenced node to be changed, and they don't. I'll update the docs now.

HEAD docs for hook_view and node_example_view have been updated.

I committed the patch agreed upon by Dries and chx since the documentation seems to be moving.

@earnie - Those are separate issues and are probably best suited for 6.x-dev.