Posted by podarok on October 14, 2009 at 9:43am
| Project: | Notifications |
| Version: | 6.x-2.1 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
| Issue tags: | comments, l10n-server-uk, pagination |
Issue Summary
From mail notifications
Greetings, podarok, A new comment has been added by badsit to this thread in group Ukrainian team: Назва модуля, як все ж писати!
У мене таке питання : модуль FileField як все ж перекласти -
Залишити як є - FileField;
Перекласти - Файлове поле або ФайлПоле або ПолеФайл
Чи транслітерувати - ФайлФилд
На мою думку якщо вже перекладати назви модулів то перекласти як ПолеФайл (або Файлове поле)!
Read more at http://localize.drupal.org/node/186#comment-648 or reply via http://localize.drupal.org/comment/reply/186/648. -- This is an automatic message from localize.drupal.org To manage your subscriptions, browse to http://localize.drupal.org/user/2222/notificationsbut
http://localize.drupal.org/node/186#comment-648has to be
http://localize.drupal.org/node/186?page=1#comment-648the same troubles are in page
http://localize.drupal.org/node/186?page=1
<h3 class="title"><a href="/node/186#comment-650" class="active">Є два варіанти, або залишити</a></h3>but has to be
<h3 class="title"><a href="/node/186?page=1#comment-650" class="active">Є два варіанти, або залишити</a></h3>
Comments
#1
This looks like a bug of messaging and notifications modules. Moving there.
#2
ok so here is a patch against a hacked up 6.1. I really need to upgrade and will by end of next week.
But here is the solution.
http://api.drupal.org/api/function/comment_new_page_count/6 handles this for you in D6. The problem is, it requires a fully loaded node. That isn't good for notifications as the node isn't loaded yet so it would hit the database hard.
So i created a similar function for notifications that just uses the nid to find the node type. Which is then used to find the variables. There is some copy and pasting from that D6 function into this new one so please review carefully, as I didn't test every possible comment mode / sort / comments per page
#3
#4
Thanks for the patch, looks good though it's a bit complex, I don't think we have the time to properly review and test this unless the D6 core issue is fixed.
See #223741: Notifications, token and comments url problems
#5
What would be the core issue? I believe your talking about the #new problem. Which is entirely different, and this isn't a work around for core. Here it is for 2.x branch as my previous was 1.x.
Please quantify this with evidence. Cause I believe *strongly* that you are mistaken.
#6
I just realized that we can use http://api.drupal.org/api/function/comment_new_page_count/6 directly.
The reason I was not using comment_new_page_count was to avoid a node_load(). But notifications_content_notifications('event load'), does a node_load() for comment events, therefore, doing a node load for the comment token is 'free' as it will come right out of the static cache.
So we can trim this patch down to a one line change.
#7
The Drupal core issue is that there's no way in Drupal core to get the page a comment is in. Actually, comment links in Drupal core fail too. Give it a try. Just click a link on the second page of comments in your forum.
So this is an important Drupal core issue, that still needs resolution for D6, the issue has been fixed for D7 but is still 'to be ported' for D6 see #26966: Fix comment links when paging is used.
Also, this is a really hard one (what happens with threaded comments?), using 'comment_new_page_count' is not that straight forward either. Yeah, it's easier for new comments but not for old ones. Moreover if your comments are ordered 'newest first' you cannot even provide a reliable link
Thus this is not actually Notifications issue. I'd say Drupal core comments paging is badly broken. And this notifications token is just a best effort to provide a link that works for some cases. We are not adding some more complex code for it to work just in a few more cases...
Anyway, if you think it's worth the effort, give me a workin patch with some unit testing (with threaded comments) and I'll commit it. But I think you'd better work on the core patch, or just add the token in some other module.
Actually any module that provided some comment tokens, either as a patch for token or a stand alone would be cool (And we could drop these tokens from Notifications).
#8
Ahh I get it. If you were to sort comments by newest first, the link that was sent out in the message could be bad as new comments could have gotten in. Make sense.
In D7, the link would hit comment/CID which would at that point figure out the page and sent the menu system properly.
I will take a swing at the d6 backport.