hook_link is called by the wrong parameters

This is an API change which could break modules. I think the best action is to:

1. Document what we are currently doing in 5.
2. Fix HEAD properly for 6.
3. If $teaser is needed, I would consider adding it as another argument after !$page.

This patch will affect the "$teaser" argument of any code calling the default
node_view($node, FALSE, FALSE, TRUE);
or
node_view($node, TRUE, TRUE, TRUE);

In core, this happens in comment module.

This should be considered; I would like to see someone else review this.

Right, the default arguments for node_view() cause this bug to appear.

i think drumm's plan in #3 is reasonable.

Any place where node_view is called as one of the following four cases will be effected:

// $teaser and $page are both FALSE
node_view(x);
node_view(x, FALSE);
node_view(x, FALSE, FALSE...

// $teaser and $page are both TRUE
node_view(x, TRUE, TRUE...

I checked the drupal-5.x-dev.tar.gz and drupal-6.x-dev.tar.gz. I found four instances where this change will have an effect(details below). In all four cases I believe that the change is actually for the better.

drupal-5.x-dev\modules\comment\comment.module(666):        $output .= node_view($node);
drupal-5.x-dev\modules\comment\comment.module(1623):    $form['#suffix'] = node_view(node_load($edit['nid']));
drupal-5.x-dev\modules\node\node.module(2223):    $output .= node_view($node, 0, FALSE, 0);
drupal-5.x-dev\modules\node\node.module(2226):    $output .= node_view($node, 0, FALSE, 0);

drupal-6.x-dev\modules\comment\comment.module(701):        $output .= node_view($node);
drupal-6.x-dev\modules\comment\comment.module(1673):    $form['#suffix'] = node_view($node);
drupal-6.x-dev\modules\node\node.module(2310):    $output .= node_view($node, 0, FALSE, 0);
drupal-6.x-dev\modules\node\node.module(2313):    $output .= node_view($node, 0, FALSE, 0);

I do not currently have CVS access. A check of all the contributed items should be conducted as well, and if any of these are effected in a negative way a bug should be opened against those projects preemptively before the change is committed.

I think that this change should be applied to both 5.x-dev and 6.x-dev since it contradicts the documentation.

IMHO, the bug is not where the finger points. There is a bug, but not where we're looking.

The problem is in how we interpret the $page parameter. This parameter went the way of $node->moderate: its true meaning has been forgotten.

First, let's look at the signature of node_view:

...
* @param $teaser
 *   Whether to display the teaser only, as on the main page.
 * @param $page
 *   Whether the node is being displayed by itself as a page.
...
 */
function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE)

Note, BTW, that $teaser and $page aren't mutualy exclusive. E.g., it's possible to have both FALSE. (Thats what the Views module does for the "full"-style listing.)

Now, let's look at hook_link:

* @param $teaser
 *   In case of node link: a 0/1 flag depending on whether the node is
 *   displayed with its teaser or its full form (on a node/nid page)
...
function hook_link($type, $node = NULL, $teaser = FALSE) {

This is where the chain of history has broken. First, the documenter assumed $teaser/$page are mutually exclusive. Second, he was not aware of the true meaning of $page. The $teaser parameter here is, or was, supposed to tell the programmer whether the node is displayed stand-alone on a page and therefore specialized links are to be displayed. $teaser is, of course, a misnomer, but the value it's assigned, !$pager, is the correct one --therefore the "fix" suggested here is wrong.

So, to sum it up:

- The problem is that the interaction bewtween $teaser and $page is vague and should be cleared up in discussions.

- Other issues, such as 134478 (Refactor node rendering), and related ones, are the ones to provide the correct solution.

Nancy, the initiator of this issue, wrote today: "$page is whether you want to use pager". AFAIK, this is not the meaning of $page.

Two comments earlier I wrote "Nancy wrote today '$page is whether...'"

I might have accidentally hurted somebody with my sloppy wording. Sorry. Nancy have a very valid point here in this issue, and I'm glad we're doing something to fix this.

I decided this is okay and put a note about the small API correction in the change log.

Committed to 5.x. Needs to go in 6.x.

Did anyone ever get a chance to scan contrib for effected code?

I got my CVS access so I did it. Here are the results. 47 projects in contributions are effected, plus whatever unpublished things there are out there. I left in nodefamily even though it was in the readme file since it was advising users to use this code and they will want to consider it there as well.

Well that was just the point, wasn't it? Any place where node_view is called with $teaser=$page was being handled incorrectly in the past (see comment #6 by drumm). Due to the default arguments there are actually 4 ways to call node_view that need to be searched for in the code (see comment #12 by me).

Your patch fixes this so that now the correct value is passes to the 'link' hook. This is a change in behavior though, so if any code was actually relying on this aberrant behavior it could cause problems there. In core I only found 2 modules which match one of these four cases (again, see comment #12) and they all look like they were expecting the correct behavior, so no big deal. I mentioned at that point, however, that someone should scan contributions for potential problem areas and alert the module maintainers to verify that the change does not effect them. Since nobody else had done so I finally got around to it and post the results. If may be that as with node and comment these locations expect the documented behavior, so this change fixes them as well. On the other hand it could be that something is now not working correctly since someone had worked around the problem.

So, it might be nice to contact these individuals and proactively tell them about this situation so that they can check their code. On the other hand I am sure that if there is a problem that at some point someone will open a bug report against those modules and it will get fixed. It is up to you what you do with all of this information ... I just like closing loops.

-zarko

needs reroll for D6. patch fails.

I have rerolled nancyw's patch as committed http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/node/node.module?r... for Drupal 6. Please do not credit me here. As it's from a released Drupal and because it's a one liner , I dare to RTBC.

Sucks ot upload the wrong patch.

As far as I see, mooffie's analysis of how the teaser and page parameters are formed is quite correct. This is flawed for a long time, there is a confusion is what teaser and page mean, and how do they interact. (At least I have been confused multiple times on this.) From the analysis, it seems that renaming $page to $standalone_display or something more clear might help people understand how this works. (Although this would be a "code documentation issue", Dries might not like to do it this late :).

Passing $teaser to hook_link seems to be logical here, but I honestly don't have a good overview on what else would also need to be fixed on these grounds, so we have better coverage of this issue fix.

Gabor,

I was wrong in my analysis.

This patch should certainly be commited.

I think that the $page flag has one, and only one, purpose:

It lets modules (node and nodeapi modules) know that they "own" the page and that therefore, if they wish, they are allowed to set the breadcrumbs and other "decorative" elements on the page.

That's exactly what the 'forum' module does when the $page flag is on. It sets the breadcrumbs.

In other words, this $page flag is not supposed to affect the links. So hook_link() too shouldn't see it.

(It's a pitty that the _purpose_ of this flag isn't documented. I suggest adding "so that modules can set the breadcrumbs" to the description of this parameter.)

Ah, the 'book' module is another example to setting the breadcrumbs when $page==TRUE. That's the only thing that happens there when $page==TRUE.

We have docs in Drupal itself which would need to be fixed, or do we have only contrib/docs/hooks to fix/expand?

>
> We have docs in Drupal itself which would need to be fixed, or do
> we have only contrib/docs/hooks to fix/expand?

Three things:

1.

The only documentation fix I know is here:

* @param $teaser
*   In case of node link: a 0/1 flag depending on whether the node is
*   displayed with its teaser or its full form (on a node/nid page)

"on a node/nid page" should be removed (because "full form" isn't used only on "node/nid" pages).

2.

Zarko provided a list of contributed modules that use node_view(), but I don't think there's a cause for worry here as it seems that, except for very few, they won't be affected by our fixing the bug.

But I take it upon myself to inspect this list further and to notify the owners of potentially problematic modules. An alternative would be to add a "A clarification for node_view()'s $page parameter" to our "Converting 5.x modules to 6.x" page and thus leave it to the module owners to check their code. Would you prefer this route? If so, you could leave this task to me.

3.

I would change this:

* @param $page
*   Whether the node is being displayed by itself as a page.

To:

* @param $page
*   Whether the node is being displayed by itself as a page. This signals
*   the module that it is allowed to modify peripheral elements on the page,
*   such as the breadcrumbs.

But I leave this to your judgement.

Earlier I wrote:
>
> I was wrong in my analysis.

I owe an apology to all involved, especially to Nancy.

Can we reroll the patch with the small documentation improvement that was suggested? Other than that, this patch should be ready to go. (I don't want to rename these variables so let's just document them better.)

>
> Can we reroll the patch with the small documentation
> improvement that [..,]

Here's a reroll.

(I've already 'fixed' hook_links()'s doc, http://drupal.org/cvs?commit=82300)

BTW, my original wording was:

"$page - Whether the node is being displayed by itself as a page. The module
responsible for this node may respond by setting the breadcrumbs or other
peripheral elements on the page."

But I decided to drop "or other peripheral elements on the page", because I don't of any except the breadcrumbs. I don't mind reverting to this original version if somebody thinks it's better.

I committed a slightly modified version of this patch to CVS HEAD. Thanks.