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

The updated logic inside page_manager_node_view_page() calls node_page_view() at the start, which builds/renders the node and invokes all related hooks no matter what output is produced. This makes sense as a way to ensure those hooks are fired, but does not accommodate for cases where the page's configured output (via panels or some other means) also includes the content of the node who's ID is called in the node/[id] request. This use case of using page_manager to "take over" a node page, but still render the node from the path to that page, is probably quite common as many people simple want to "wrap" a normal node display with other layout logic. So it seems like this could be a common error case.

Perhaps we need a way to detect if the configurable output of the page already triggered hook_node_view() and/or hook_entity_view() and then only call node_page_view() (transparently) if that detection fails? I suppose ctools could implement it's own version of these hooks and set a static variables or something as a flag? Maybe there are more elegant ways?

It also occurs to me that this pattern of invoking entity hooks via [entity-type]_view() independent of the page output produced by page_manager may repeat as part of #1760578: Update user_view.inc to execute all normal Drupal hooks and #1760584: Update term_view.inc to execute all normal Drupal hooks. So I think those are also cases where this same problem could play out for term and user content (in addition to nodes). I'm marking those are related just in case so it can be confirmed whether-or-not they pose the same problem.

Maybe each entity_view() call should be controllable via a variable?

Do you mean a configuration option within page_manager directly (like a toggle within the system-provided node_view, term_view and user_view pages)? I guess that would be a pretty low-impact way to do things and might make more sense that trying to add logic (that would surely end up pretty convoluted) to auto-detect when to trigger these extra *_view() calls.

Before getting too far down that road though I do wonder if you agree that there is a fundamental issue here? I'm not too sure if it really is considered a bug to fire entity hooks redundantly on the same entity object. I'm just sort-of assuming that's the case given our local case-specific experience with this. My best judgement tells me it's a dangerous thing to, but I can think of soooo many different ways that could happen in the contrib space (given the static entity cache) that I wonder how preventable it really is.

Bug or no bug, I suppose there is still a performance concern here that is ctools specific.

The way this issue manifests itself is rather tricky and case-specific. It seems like we need a patch that can at least evade the multi-invoking hook behavior so people can verify if this issue is related to their own observed problems... even if the correct way to fix things is not 100% clear yet.

I'm attaching a patch that adds a ctools_entity_view() implementation to set a static variable that tracks which entities have been "viewed" (hook_entity_view() invoked and hook_[type]_view() by inference). This variable can later be checked in page_manager_node_view_page() (or page_manager_[type]_view_page(), etc.) to see if the related entity "view" hooks were invoked. The patch only handles things for nodes, but the idea could easily be extended to terms and users as per those other related issues.

I really have no idea how close this is to a "real" fix. It may be an over-engineered fix (I'm not sure that using drupal_static() across scopes like this is a good idea), but it does fix the issue locally while retaining the requirements established in #1760384: Update node_view.inc to execute the normal Drupal hooks.

You may want to use this patch instead of #7.

All the moving parts are not fresh in my head right now, so forgive me if I'm missing something. Though the idea from #10 would remove this bug I think it would also introduce a regression. Namely it would re-open #1760384: Update node_view.inc to execute the normal Drupal hooks because the node/entity view hooks would only get called in the "fallback" case where no plugin is selected to render the output (which is rare). However, the patch does highlight another way to test if observed bugs are linked to this issue.

Anyway, at this point I think it's pretty safe to say that we've isolated the root cause of a fairly significant bug here. It would be interesting to learn more about the best-practice ways to address it. I think #7 is a candidate, I'm just not sure if there is a more elegant way.

Actually, it looks like this needs a bit more work as the current patch will not generate a "default" page title. So if you are using, say, Panels to set your layout, and have the panel configured to fall-back on the generic page title from the node referenced in the URL, no title will be generated.

Anyway, it looks like prior to #1760384: Update node_view.inc to execute the normal Drupal hooks there was an explicit call to drupal_set_title() in page_manager_node_view_page() that took care of this. After #1760384 that line was removed, most likely because the change from that issue forced node_page_view() to be called no matter what, which it seems took care of setting the default title. The whole point of this issue's patch is to not call node_page_view() unless we can confirm that it's hooks were not invoked via other means (to ensure they don't fire twice), so if we do that I guess we have to also set a fallback title manually.

Anyway, I'll try to get my patch updated to address this, though it would be useful to get some technical feedback on all this from a maintainer.

Ok, here's an new version that addresses the page title issue by essentially reverting a bit more of #1760384: Update node_view.inc to execute the normal Drupal hooks. This updated patch ensures that the few lines that must always be triggered from node_page_view() are run (e.g., drupal_set_title(), etc.) while still ensuring that the entity_view hooks are only invoked once.

The fundamental strategy for checking that the entity_view hooks only fire once is the same as it was in #7.

Yep, the re-roll applies fine and looks consistent with my patch from #15. Thanks.

Unfortunately the new ideas proposed in #27-onward would appear to also introduce a regression. Namely they would re-open #1760384: Update node_view.inc to execute the normal Drupal hooks because the node/entity view hooks would only get called in the "fallback" case where no plugin is selected to render the output.

I think this is largely going back to where we were before #1760384. In fact there are other issues that seem to be, in one way or another, trying to undo the changes from #1760384 (most recently #2437773: Attached CSS and JS files are not loaded, which btw contains a commit that sorta complicates the review process here).

So the pending problem, and complication, seems to be that we also need to address the problem that #1760384 originally set out to solve, which is to ensure entity view hooks are always fired when page manager is displaying a node page. The reason the original patches in this issue (e.g. #15) may seem slightly "heavy" (with the new static, etc.) is really because of the problem opened in #1760384.

Hummmm. I'm now digesting your comment here:

then ctools/plugins/content_types/node_context/node_content.inc should be able execute hooks defending on "no_extras" setting

That's very interesting. I didn't realize that setting toggled those exact 2 hooks that were missing (node_view and entity_view), and were presumably the whole basis of the issues in #1760384: Update node_view.inc to execute the normal Drupal hooks. That might be the key to creating harmony with #1760384.

I'm wondering however if all the limitations that came up in #1760384 could be addressed with this one toggle? Are there cases where the "no extras" toggle would need to be on, but those hooks would still need to be invoked for some tracking purposes? If so, I'm guessing that would be an issue with the implementing modules, and not ctools/page-manager.

Putting back to "needs review" in light of that insight.

metatag and others that "could" have issues with logic flow (suppose doing too much assumptions about "view" hook) must be fixed by their own (great to link them here)

True. It sounds like this issue could be "decoupled" from the points discussed in #1760384: Update node_view.inc to execute the normal Drupal hooks and moved forward more efficiently. However, it's worth pointing out that as it stands right now (based on this issue and the existing commit from #2437773: Attached CSS and JS files are not loaded) anyone depending on the changes previously implemented in #1760384 (ctools 7.x-1.6) is going to perceive a regression when they upgrade to ctools 7.x-1.8. They may be able to fix it with the "no extras" toggle, or via changes to the modules that depend on those *_view hooks (I think metatag already did this), but some disruption will likely result. I'm not sure what the best process is to mitigate that, but figured it was worth noting.

I posted a comment in #1760384 about this to point people here, though it may go relatively unseen without re-opening that issue.

Ok, finally had a chance to look at the term_view.inc and user_view.inc changes that were added to the patch in #29. From what I can see everything is consistent such that all "fallback" cases trigger default core behavior (*_view hooks are invoked, etc.) and otherwise everything is left up to whatever plugins are active with most all other assumptions eliminated. It also looks like quite a bit of baggage was stripped away from term_view.inc, which I can only assume was some legacy bloat (redundancy with taxonomy_term_page() removed).

I'm all-for moving forward with this approach, but I think others will need to comment on this (new) premise that *_view hooks are the sole responsibility of plugins (and may be conditional... see #33).

I think we are potentially playing regression tag with this issue. In order to clarify things I've created an issue summary. I hope it accurately reflects the components of this issue that remain along with the rather complex history.