Currently node_view.inc does not execute the normal Drupal hooks, e.g. hook_node_view and hook_entity_view. In order for several other modules to work correctly, e.g. Metatag, these hooks must be executed.


Original request:

Currently the node_content.inc plugin uses node_invoke($node, 'view') to trigger node_view(), which means that several key node-related and entity-related hooks do not get triggered unless that pane is added to the page. Several modules, include Metatag, depend on entity hooks triggering on entity view pages, which won't happen. I suggest moving the entity/node view logic to node_view.inc, and trim the node_content.inc logic to just building the final output.

Files: 
CommentFileSizeAuthor
#13 ctools-n1760384-13-d7.patch2.12 KBmeba
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]
#12 ctools-n1760384-12-d7.patch2.12 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]
#11 ctools-n1760384-11-d7.patch2.12 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]
#10 ctools-n1760384-10-d7.patch2.01 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]
#9 ctools-n1760384-9-d7.patch2.08 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]
#7 ctools-n1760384-7-d7.patch1.4 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]

Comments

merlinofchaos’s picture

That doesn't work; the node_invoke call can do a lot of modifications to the node that are necessary for that rendering, so simply moving it is not an option.

DamienMcKenna’s picture

Title:Move hook_node_view implementation to node_view.inc» Update node_view.inc to execute the normal Drupal hooks
DamienMcKenna’s picture

I've updated the title to clarify the task.

DamienMcKenna’s picture

Component:Code» Page Manager

This is a Page Manager thing.

DamienMcKenna’s picture

Category:feature» bug
DamienMcKenna’s picture

Status:Active» Needs review
StatusFileSize
new1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]

This patch clones most of the code from node_view_multiple() in order to trigger the appropriate hooks.

DamienMcKenna’s picture

DamienMcKenna’s picture

StatusFileSize
new2.08 KB
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]

A slight variation of #7 that moves the generic core snippets to the top of the function and the CTools custom code underneath.

DamienMcKenna’s picture

StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]

A huge simplification of the code by just running node_page_view() at the beginning, and storing its output for later in case it's needed.

DamienMcKenna’s picture

StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]

Some updated comments, functionally it is identical to #10.

DamienMcKenna’s picture

StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 67 pass(es).
[ View ]

A small tweak to the last patch that removes the $function variable as it is no longer needed.

meba’s picture

StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

The patch did not apply anymore. I rerolled it and also confirmed that once applied Meta Tags start showing on Panel pages that are not using full rendered output

DamienMcKenna’s picture

Status:Needs review» Needs work

This needs some additional work, it should also trigger the page title handling.

DamienMcKenna’s picture

Status:Needs work» Needs review

Never mind, I think I found the solution for the page titles in #1732538: Page title pattern ignored. Pushing this back for review.

Peter Törnstrand’s picture

This patch (from #13) did not have any effect on my setup. The meta tags (from metatag.module) did not show up on my node.

Meta tags 7.x-1.0-alpha8
Ctools 7.x-1.2
Panels 7.x-3.0
Panels everywhere 7.x-1.0-rc1

DamienMcKenna’s picture

@Peter: Please try the -dev version of Metatag.

DamienMcKenna’s picture

@Peter: also note that currently the page title is not overridden correctly, I'm working separately to resolve that (#1732538: Page title pattern ignored), so please check the description and other tags.

philsward’s picture

Patch from #13 gave me the following error:

    Warning: array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 178 of /home/username/public_html/h/mydomain.com/includes/entity.inc).
    Warning: array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->cacheGet() (line 354 of /home/username/public_html/h/mydomain.com/includes/entity.inc).

Redownloading 1.x-dev from 2012-Oct-11 makes the error go away, so definitely something with the patch...

Hope to see something working between this and metatags soon : )

philsward’s picture

Oh, I forgot to mention that I'm using the metatag-1.x-dev-[2012-Oct-16] and I'm still not having any luck with the metatag description after applying patch #13 : (

thelmer’s picture

Patch #13 don't work with :
ctools (7.x-1.2) + metatag 7.x-1.0-beta1
or latest dev versions of ctools+metatag

Also the patch breaks conditionals in webform 7.x-4.0-alpha6
The div id's are changed from : id="xxx-1" to: id="x--2-1"
Seems like node_page_view($node) are invoked twice

DamienMcKenna’s picture

Status:Needs review» Needs work

@Thelmer: when you say it "didn't work", what meta tags did you check and entity page were you viewing (taxonomy term, node, etc)?

thelmer’s picture

Status:Needs work» Needs review

Hi Damien

I'm trying with node pages displayed with panels.
I have only been looking at the description meta data.

After some more testing I have seen that it actually works with 1.2+beta1 on regular nodes but not on the one I use for the front page. For now I can set the description here with the global metadata settings for the frontpage.

I still see the prblem with webform conditionals.

DamienMcKenna’s picture

@thelmer: sorry, I got mixed up between this and another issue.

I'll have to look into Webform.

liquidcms’s picture

patch in #13 worked for me to add metatag keywords on to a node overridden by panels.. thanks a lot guys.

Lukas von Blarer’s picture

Works for me as well.

othermachines’s picture

Patch in #13 works for me in that it adds meta tags to a node overridden with a panel. Thanks!

Meta tags 7.x-1.0-alpha8
Ctools 7.x-1.2
Panels 7.x-3.3

othermachines’s picture

@DamienMcKenna:

After updating I no longer seem to need this patch anymore. Everything works fine.

Drupal 7.22 (from 7.16)
ctools 7.x-1.3 (from 7.x-1.2)
metatag 7.x-1.0-beta5 (from 7.x-1.0-alpha8)
panels 7.x-3.3 (not updated)

DamienMcKenna’s picture

The patch is no longer strictly needed as I built some workarounds in Metatag.

That said, I still think the change to CTools is worthwhile as it should be triggering the correct hooks.

caschbre’s picture

I have a project with logic in hook_node_view that is needed and not being executed for panel pages. I'm going to give this patch a shot to see if it solves the issue.

caschbre’s picture

Issue summary:View changes

Rewrote the description.

justafish’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Patch in #12 applied and worked great. Thanks @DamienMcKenna!

japerry’s picture

Status:Reviewed & tested by the community» Needs review

While #13 works as well I'd like to have a few more eyes on this before I commit it since it makes a few changes and the patch is relatively old. Marking needs review.

joelpittet’s picture

I've been using this in production for a number of months and it's been working well.

dillix’s picture

Status:Needs review» Reviewed & tested by the community

#13 works great on few sites with latest Drupal 7.34, ctools and Metatag 7.x-1.4, please commit it. hook_node_view now executes for panel pages with this patch.

Here is log:

# patch -p1 < ctools-n1760384-13-d7.patch
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/page_manager/plugins/tasks/node_view.inc b/page_manager/plugins/tasks/node_view.inc
|index b8a7e0c..8f53751 100644
|--- a/page_manager/plugins/tasks/node_view.inc
|+++ b/page_manager/plugins/tasks/node_view.inc
--------------------------
Patching file page_manager/plugins/tasks/node_view.inc using Plan A...
Hunk #1 succeeded at 78.
Hunk #2 succeeded at 89.
done
mrjmd’s picture

japerry’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+SprintWeekend2015

Coolness, committed.

  • japerry committed 05d7202 on 7.x-1.x authored by meba
    Issue #1760384 by DamienMcKenna, meba: Update node_view.inc to execute...

Status:Fixed» Closed (fixed)

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

rjacobs’s picture

I'm not quite sure if this specific issue should be re-opened or a new one started, but I suspect there may be some problems with the solution that was committed. It seems that this fix introduces situations where node-related hooks, such as hook_node_view(), get fired twice for the same $node entity variable. It seems that this could be a problem if related hook implementations make certain kinds of alterations.

The case that I've been debugging, and that lead me to this issue, involves the addition of a book node within a panels pane (added as a "rendered node"). With ctools 1.5 all was fine, but now with ctools 1.6 we are getting a barrage of php notices triggered from node_page_title(). As best I can tell, this is due to the following:

  1. page_manager_node_view_page() builds the book node via the $default_output = node_page_view($node); line that was added from this patch. This triggers all node-related hooks, such as hook_node_view() as intended even though the returned output is never used.
  2. Later in page_manager_node_view_page() ctools_context_handler_render() is called, which leads to the appropriate panes display rendering plugin to be called, which ultimately calls node_page_view() a second time with what appears to be the same $node variable as before (as per the static entity cache).
  3. Node hooks are fired again, including of course hook_node_view().
  4. book_node_view() is invoked, which redundantly adds some 'book_navigation' markup to the $node->content (it was previously added in the first pass of the node hooks).
  5. While the 'book_navigation' markup is being themed some preprocessors fire (in my case these happen to be related to the bootstrap theme), which try to rebuild the book navigation tree. This triggers a number of menu logic calls that end up throwing a number of php notices because the menu had previously been built and translated (the specific notice in our case were related to the book navigation title values not being accessible).

Commenting out the $default_output = node_page_view($node); line that was added to page_manager_node_view_page() in this issue's commit makes the problem go away.

Is it ok to assume that some node hooks, like hook_node_view(), can be safely triggered more than once on the same $node object? I'm sure the specific problem we encountered is somewhat case-specific but it would seem that there is a decent potential for other issues here (with this example being only demonstrative). It also seems like a performance concern to be redundantly building and rendering the same node twice, but only displaying it once.

In light of all this, please let me know if this should be re-opened or a new issue started.

DamienMcKenna’s picture

@rjacobs: Please open a new issue. Also, oops :-\

rjacobs’s picture

kristofferwiklund’s picture

This commit has introduced some bugs related to rendering display suite nodes in a panel for node/%node

kristofferwiklund’s picture

Seems to be the problem mention in #40.

rjacobs’s picture

Note that there is some discussion in #2422123: should entity view hooks be triggered unconditionally in node_view, term_view and user_view pages? about reverting the changes from this issue, which could apply to ctools 7.x-1.8. The justification is that:

  • Nodes rendered via the the node_content.inc plugin do have the ability to trigger node/entity view hooks, as long as the "No Extras" option is disabled. In fact the only purpose of that "No Extras" option is to either enable or disable the firing of hook_node_view() and hook_entity_view().
  • It may not be correct for modules to depend absolutely on hook_node_view() and hook_entity_view() being invoked via page manager for nodes.
  • Unconditionally forcing hook_node_view() and hook_entity_view() to be invoked when accessing a node path via page manager has the potential to cause many serious, and difficult to debug, issues as illustrated in #2422123 and it's related/linked issues.

At this point comments on all this are probably best directed to #2422123: should entity view hooks be triggered unconditionally in node_view, term_view and user_view pages?.