New comments links don't point to correct pages

OI. node_load(). Man. We can't do that if we can avoid it. Views goes to a lot of effort to try and reduce the number of queries run to the minimum necessary, and node_load is pretty much the opposite of that.

We may have to do what comment_new_count does and do a pre_render or something to try and deal with this.

subscribing. a-must-be-fixed-problem. And even more, not just the new comments link, but any link in recent comments views that goes to the second/third/etc page of a long thread doesn't work.

Here's a patch with the same fix, rolled from the Drupal root as per standard procedure.

This is a vital enough feature for my site that I really don't care if it causes extra node_load()s. This bug is critical enough that it really must be worked around soon, or just throw the node_load() in there along with a warning in the GUI that it may be intensive. Or just remove it altogether since it doesn't work as advertised under the very real possibility that a node is spreading its comments out across more than one page. Please fix!

I'm sorry, no, I won't accept a node_load.

At least, not until I'm sure that the method used can't be broken down into a join so that node_load isn't required. And if it is, then there needs to be a giant warning on the field that it's a performance drain.

If you just need the node type, in the init() set $this->additional_fields = array(array('type' => array('table' => node, 'field' => type')); After you've added it, in the render() you can refer to it as $values->{$this->aliases['type']} -- I recommend doing a quick search for additional_fields in the handlers, you should see various uses.

Then you don't need pre_render at all, nor extra queries.

Paul: I won't be able to test this until much later tonight, but I'll give that a try. Sorry I ignored it earlier - the inner workings of Views is still voodoo to me, so I'm going to glaze over unless you spell it out for me as you did in #7 (I have no idea what to do with moc's suggestions in #8).

Thanks.

Okay, I only did some minor testing, but Paul's node_load()less tweak seems to work just fine. Here's the new patch.

We're definitely closer, but we don't need to do an extra query at all. We can use the additional fields functionality to add the 'type' directly to the query. One example of doing this would be the node edit field, which also needs type information in order to do the security check.

It's acceptable as a quick fix in that you can apply it to your site to make it work. It's not something I am willing to commit; it still performs extra unnecessary queries.

BTW, I'm really sorry if I come off as a pain in the behind about all this. Performance is a real issue with Views, and unnecessary performance degradation is a major issue to me.

I read that as "no, Garret's patch is not acceptable as a quick fix."

My patch; your code. =P

Later last month, I managed to have some breakthroughs with regards to wrapping my brain around how coding for Views works. If Paul doesn't beat me to it, I'll take a look at it tomorrow afternoon and see about getting the type added to the initial query. And don't worry about being a pain, merlin; insistence on a higher level of code for major projects like this is not a bad thing.

Paul Kishimoto, this patch don't work for me

Paul is doing something naughty - either he's following the correct procedure for creating patches but he's put the Views module folder in the root modules directory, or he has the Views folder in the correct directory (usually sites/all/modules) but he's rolling the patch incorrectly. Either way, the patch is small and easy to apply manually for now.

…Unless you're saying the code isn't working after the patch is applied. I haven't tested it yet myself, so that might be the case…?

Garrett, he's rolling the patch correctly. Views has its own 'modules' folder, which I think is probably what's making it appear incorrect.

Initial glance at the patch is that the code looks like what I want, so if I can get some feedback from other testers that it functions properly, I think we're very close to getting this in.

i tryed all patches in this issue, but new comments links don't point to correct pages

Okay, I'm used to all patches being made from the Drupal root directory. So maybe the only thing wrong with the patch is it's rolled from a place I'm not used to it being rolled from.

I did not have a chance to test yet, but I'll do so today once I'm off the clock.

@Paul Kishimoto, i use views 2.1 and drupal 6.6

i provide in my view - field: New comments

i have 1 new comment on 2 page, but link in view: /node/1570#new

tracker display this correct: /node/1570?page=1#new

I trying clear system cache and views cache to, but nothing has changed..

What can I do wrong?

i trying new patch (#21), but no effect

Okay. I can confirm that Paul's last two patches don't work. In the l() function's parameters, one of the keys of the array in the third parameter had switched from "query" to "argument." So the bit which tells l() to add the ?page=X part to the link was not getting passed to the function correctly. Simply swapping "query" back in fixed the problem.

Here's my fixed patch - rolled from the Drupal root 'cuz that's what I'm used to, by crockey. Get off my lawn!

Aside: Is it considered good form to create an object from an array in that way? Yes, it's a one-liner, but I've never been a big fan of PHP's parenthesized type-casting thingamajigs - they're syntactically weird and usually unnecessary when we have intval() and strval() and such. Maybe that's just my taste, though.

Sorry, patches against modules should be rolled against the module root. I don't work in the drupal root, and there's multiple places you can have the module anyway. You could put it in sites/all or in sites/mysite or in profiles and of course there's the regular modules directory too. So from module root is the correct place to submit patches to modules.

Patch won't apply.

Whoops, wrong window. Sorry!

@Paul Kishimoto, this patch (#29) works on my site!

This is a duplicate of #173955: use comment_new_page_count() for #new links field but there's more action here so I've closed out the old one. I independently rolled a similar patch fro that issue.

Looks like the patch here was a bit cleaner than my draft. Re-roll to use new stdClass() instead of the cast to object.

And minus the extra space :(

i hope this patches will be in the next version of views)

OFF, have you tested it? Did it work?

I tested patch (#29), but if it needs I can test (#33) to

That would be great, and then if you were to RTBC it after testing, it'll have a better chance of getting into the next release.

what is RTBC?

'Reviewed and tested by the community' - it's the status you set something when you think the patch is good enough and has been looked at by enough people to be committed to the project.

Patch (#33) working

No longer applied, re-rolled.

Committed! Thanks for seeing this one through, folks!

i testing this patch on others projects.. working good..

would like to see this patch in the recommended versions of the module

patch #42 working with views 6.x-2.2

OFF, Views 2.2 is the current recommended version, which can never change. Views 2.3 will be the next recommended version, and will have this patch in it - but running 2.2 with the patch applied until that comes out won't do you any harm at all.

catch, ok! Thanks!

Should it already work in 2.3 stable version?

Still not working? I've check 2.3 & 2.6.

It is unacceptable to re-open a closed issue with only one sentence. See the issue submission guidelines.

On 6.x-2.10, the issue still persists. Node:Has New Comments filter is generating #new links, and these #new links aren't redirecting to the correct page of the comment. It functions correctly for single page threads but fails on multi-page ones.

From what I could gather in other issue reports and such, the #new issue was fixed in Drupal core (http://drupal.org/node/6162). We have the latest 6.x version, and also the latest recommended release of Views (which is 2.10). Our #new links are still not working. I thought the patch at #42 was already in 2.10. Would appreciate it if someone could direct me to the right patch to apply, and to which module (be that Views itself or the core Comments module).

Thanks

Just checking in. Haven't seen any responses in 2 weeks. We've been trying every patch on this topic we could get our hands on, and sifting through code to figure out the problem but we can't seem to find it.

Some help would be greatly appreciated.

I'm running into the same problems as described 2 years ago, and recently here with adener.

I'm not at a level of proficiency to adapt the patch from post #42 to work with the current version of Views, but it seems like the problem's fix is right there. If someone could simply update the file in question to reflect the code in the patch, the problem should be solved for good, right?

For reference, the file the patch wants to update is
modules/comment/views_handler_field_node_new_comments.inc
(in the Views module directory)

But, since the versions don't match... it's yelling about being patched, and I don't know how to proceed.

Patch from 42 was committed ten months ago, and the issue was closed.

If there are further problems in the most current release, open a new issue and cross reference the old one. There's no way the patch from 42 can be applied, the changes are already there.

Views is using the core function.