should entity view hooks be triggered unconditionally in node_view, term_view and user_view pages?

Problem/Motivation

This issue was initially opened due to a regression introduced in #1760384: Update node_view.inc to execute the normal Drupal hooks → (in 7.x-1.6). The commit from #2437773: Attached CSS and JS files are not loaded → (now in 7.x-1.8/9) largely reverses the changes from #1760384, but at the same time essentially re-opens #1760384. This also means that the initial problem from the OP of this issue is no longer present.

As a result, we need to determine if any components from #1760384 still need attention and also ensure that the behavior of node_view.inc, term_view.inc and user_view.inc (in terms of *_view hook invoking) is consistent.

Proposed resolution

It's currently proposed (#28) that the changes in #1760384 (which force entity view hooks to always be invoked for node_view pages) was essentially a mistake, and that it's the sole responsibility of plugins to invoke, or not invoke, entity view hooks. The most recent patch (#29) therefore ensures this rule is consistent for node_view.inc, term_view.inc and user_view.inc, along with some other cleanup.

Remaining tasks

We need to definitely answer the base question "should entity view hooks be triggered unconditionally in node_view, term_view and user_view pages or not"? In 7.x-1.6 the answer was "yes".

User interface changes

n/a

API changes

Other modules would no longer be able to depend on *_view hooks (e.g., hook_node_view() and hook_entity_view()) being invoked unconditionally for node_view, term_view and user_view pages.

Data model changes

n/a

Original report by rjacobs (2015-02-07)

This is a follow-up to #1760384: Update node_view.inc to execute the normal Drupal hooks → . That (now closed and committed) issue made some changes to ensure that node-related hooks are triggered whenever page manager is taking over the display of a node path (node/%) even if the node itself is not rendered in the final output. Unfortunately the changes made to accommodate this introduce cases where these node-related hooks may get triggered twice for the same node, during a single request. I don't think it's safe to assume that all node hook implementations can be run redundantly on the same node, especially those that might make alterations. In our case this has led to numerous php errors popping up immediatly after updating to ctools 1.6.

To replicate:

1. Install ctools and panels.
2. Enable the system provided node_view page and configure it with some arbitrary layout and no selection criteria (active for all nodes).
3. Add a variant to this node_view page that displays some panels content. Within one of the pane regions add a "rendered node" pane. This pane could show any view mode, but for testing "Full Content" is probable easiest.
4. Set a breakpoint or dpm() call within node_build_content(), or implement either hook_node_view() or hook_entity_view() and set a breakpoint or dpm() in there.

With the above in place, you'll see that hook_node_view() and hook_entity_view() are fired twice for the same $node object variable when viewing any site node. If reverting back to ctools 1.5, or simply commenting out the first $default_output = node_page_view($node) line in page_manager_node_view_page() these invocations are only triggered once as expected.

I think the ramifications of having these hooks fired twice on the same node could range from none to dangerous. Either way my impression is that it's probably not safe to do this. We discovered this issue because we have some bootstrap (theme) related preprocessors that are triggered on book nodes within theme calls triggered from book_node_view(). These theme calls did some menu proecessing that is apparently not robust enough to happen redundantly on the same node (triggering a barrage of php notices). I don't think the specifics of our case are all that important though, as the main point is that any weird things could happen if the same hook implementation logic runs redundantly on the same variables.

There is also a performance concern here, as it seems quite wasteful to be building and rendering the same entity twice (even if the static cache ensures they it's only loaded once) when a node is only displayed once during a page request.