Notices/warnings when comment module is disabled

Confirming for the later warming, haven't seen the earlier ones... yet.

       || ($last_viewed <= $node->changed || (isset($node->last_comment_timestamp) && $last_viewed <= $node->last_comment_timestamp))

Adding an isset fixes it though, but seems slightly hack. The comment timestamp isn't mentioned anywhere else in last_comment_timestamp.. so it feels like it should not check a variable it does not know about? Shrug.

The earlier ones (entity_get_info(),_field_info_prepare_instance()) were fixed by #668386: Handle unavailable entity types or bundles..

Should be just node_tag_new() now.

Ack, that one's my fault, see #667152: Optimize node_tag_new(). A module_exists() would be more correct than an isset(), or we could roll that entire patch back. {node_comment_statistics} is a crappy table.

NCS may be crappy, but for large sites, keeping separate counts like these, updated only on changes, is basically unavoidable instead of having to recompute them constantly for each display.

IMHO the problem is rather with the fact that node still has knowledge of comment, which should be the one able to tell node if it should update its "new" status. Which BTW means only node and comment can do this, whereas other modules could also want to notify node that their changes should also result in a "new"/"updated" status for the node, and this is not really doable as long as node directly access data from comment instead of relying on (one more) hook.

If I recall my own conversation correctly, I mentioned to catch on IRC how I thought a more generlized term could be stored in the node object for the function to check, one unconnected to node's update date, and so modules could mark/change it as they desire, e.g. $node->something_to_do_with_update_timestamp = $node->last_comment_date in some comment hook. Thus could open up whatever other module wants to update it, without having to deal with any new hooks.

Therey're an interesting discussion that catch linked me to here #148849: Refactor {comment_entity_statistics} into performant Field

Yep, on either that issue or another one, we discussed $node->last_activity as a general timestamp column, then you define what counts as activity, but we can't do that for D7.

Here's a patch. But we could equally roll back #667152: Optimize node_tag_new(), I wasn't 100% sure of that patch in the first place.

O.O! new function node_tag_new => comment_tag_new

Also this is on every node view, bumping to critical

Tested the patch in #8. It makes the notices/warnings go away. Does it do it the right way?

It's either that or a rollback to how it was in D6. I'm starting to lean towards the rollback (despite writing the original patch), because the issue that tried to solve was pretty minor anyway. However either fixes the bug, just depends how much we want to preserve the original attempt at optimizing this, vs. verbosity of the code. Uploaded a rollback patch for comparison.

Coming from duplicate #721848: Notice in node.module caused by optional comment field
Dries proposed idea to merge comment_node_load() onto node_load() but this ways seems more clear, my +1 to separate comment code from node.module

Patch from #8 applies with 2 succeeded hunks so this is a re-roll against current HEAD

Talking on IRC with catch I confirm that patch from #11 is much better than #8(#12) because this check was added as optimization for not most common case (when user often refreshes a page by waiting for new comments).

So confirm RTBC for rollback patch!

Alright, committed to CVS HEAD! Thanks.