Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks

IMO, Druplicon should be fixed.
For the [#x] filter, people should get the nid from somewhere else than the url.

Will the old URLs work?

In the workflow I use for projects, if I have to patch a contrib module, I give the URL to the comment that has the patch in the git commit message for my project's repository.

So I have a lot of existing commits with the old URLs in various projects.

(Details are reasoning here: https://github.com/joachim-n/git-house-guidelines#patching-contrib-modules)

I propose putting the nid at the end of the h1 tag so they are easy to copy/paste.

Ah yes, the old URLs work. That's good.

The new format is not as nice, as you don't see the node ID in the URL.

(It's also a bit silly: why does the comment ID need to be repeated in the URL?)

This has caused me the most problems out of anything thus far.

1) Copy the URL, paste, hit TAB for dreditor... nothing happens. Because its not a node!
2) I'm on a node, but I want to link someone to the appropriate comment. Now that's a page load! Not just adding a #fragment. On mobile, this is a HUGE problem.
3) You can no longer tell which issue something is on. Combined with #2125287: Add issue metadata (besides related issues) above the comment textarea, this is really really annoying.

What does search engines think about this?
There are lots of links (one per comment) in an issue linking to "other url", which in fact is the same page with an anchor. Canonical link tag in HTML head looks OK, pointing to node/X
Any SEO expert?

At least, comment references link inside comments are kept with just the anchor, so no page load is required (which is quite good, because today drupal.org had lot of chances to get 500 error messages when requesting a new URL).

it would be helpful to see some reasoning behind changing the comment permalinks in that way.

on the downside it confuses users and causes unnecessary page reloads when clicking on comment permalinks.

edit: also note that after posting comments, you are still redirected to the old url structure which basically leads to a non-existant anchor tag ups, the last observation might just be wrong

Shouldn't this be rather easy issue to fix, I mean if you look at code structure:

<a id="comment-8136269"></a>
<div class="comment comment-new clearfix">
  
    <h3 class="comment-title"></h3>
  
  <a href="/comment/8136269#comment-8136269" class="permalink" rel="bookmark">#6</a>

...
There is already link that we need and anchor works anyway, just replace href from
<a href="/comment/8136269#comment-8136269" class="permalink" rel="bookmark">#6</a>
with value from (+ appending #)
<a id="comment-8136269"></a>
So it could look something like this:
<a href="/node/2125397#comment-8136269" class="permalink" rel="bookmark">#6</a>

The list of issue files has links to comments which do include the node ID, since they're intended to jump further down on the same page.

So at least my git workflow is okay, since I only ever need to link to comments where a file was uploaded.

I really liked the old D6 way.

> https://drupal.org/comment/8133955#comment-8133955 are a feature when an issue has so many comments

Slightly off-topic, but wouldn't a better fix for that problem have been for the comment rendering on nodes to have this sort of logic:

1. get given the node
2. how many comments does it have?
3. what's the comment pager set to?
4. does the requested URL have a jump link appended to it? If so, does this point to a comment that is past the first page?
5. render comments accordingly.

does the requested URL have a jump link appended to it?

Jump link information is simply not available server side.

I'll chime in as another person whose workflow has been pretty screwed up by this change. :( Though I can't in good consciousness raise it to "major" with all of the other outstanding things.

Nevertheless, here's what's happening:

1. Find a comment I want to reference from an e-mail or whatever.
2. Click on "#44"
Old step 3: Copy/paste the URL from my browser's address bar.
New step 3: Curse and wait 10+ seconds while Drupal.org reloads the entire page I was just on at a different URL.
4. Hit the back button.
5. Right-click "#44" and copy the URL.
6. Paste it at the end of the URL in my browser address bar.
7. Manually munge it until I get the old style URL.
8. Copy/paste the URL from my browser's address bar.

:(

I have to assume that at least some of these performance issues we're experiencing are caused by these superfluous page reloads by other people trying to do the same thing.

I've updated this summary with a fourth option (node/nid?page=pid#comment=cid) and added links to the core issues where these problems are being addressed. dww expressed on irc that he would rather these issues were fixed in core than worked around in project.

The core issues have yet to reach agreement, and even if there is agreement soon any fix is unlikely to be accepted for 7.x, so I don't think we can just wait for it to be fixed in core.

I've put some advantages/disadvantages of the old URIs in the summary, general feeling here (which I agree with) seems to be that the old URIs were better, even if they had a problem on issues with lots of comments.

Would a patch to implement #4 (or #1) be accepted?

Thanks for the pointer to project_issue_preprocess_comment, we're already messing with the permalink there so it's definitely the right place.

The patch is actually quite simple. Important notes:

• 'class' => 'permalink' changed to 'class' => array('permalink') (this fixes a bug exposed by the other changes).
• We build the fragment ourselves, but in the same way the comment modules does it.
• comment_get_display_page will trigger an SQL call each time it's used. If we're worried about the performance of this then removing it would take us back to the drupal 6 behaviour (i.e. assume page 0), or we could store it in a static variable on the assumption that you'll usually be rendering the set of comments for a single page at a time.

doh!

Changing the Comment Permalink URIs in D7 certainly won't be possible at this point in time anymore. However, we will try to fix the UX regression at least for D8...

In studying the situation, I noticed that there is a module for this:

https://drupal.org/project/comment_fragment

Note sure what the d.o policy is, but it might make sense to use that module instead of custom code. Modules have an overhead, too. Really depends on the project-specific policy.

FWIW, I adjusted Dreditor to at least provide a temporary stop-gap measure until this issue is fixed:
https://github.com/dreditor/dreditor/pull/105

This was the fix from the Drupal 7 upgrade I was most looking forward to, and I haven't been very bothered by the comment/% paths at all.

There are very good reasons why we ended up with this scheme, and I don't see any of that discussion reflected here or at #2010202: Deprecate comment_uri().

If the issue is with copy/paste, then I'd suggest some javascript that changes the URL of the link when it's copied to include the nid (but retains the behavior on click to avoid redirects/extra queries) and possibly document.location when you visit a page. That way nothing has to change under the hood at all.

A solution that reverts or significantly waters down #6162: #new links don't work on paged threads / #26966: Fix comment links when paging is used. please no.

Let's try https://drupal.org/project/comment_fragment. That will be better than maintaining this code in project_issue module.

It needs a general code review. Any potential performance or security issues? Is it something you would trust to run on your own site?

I do see some minor whitespace problems. Otherwise, it looks okay on code read. Since it is such a small module, it is possible to check that all code paths are free from PHP notices.

Looks okay to me. As you say, there's a whitespace issue which makes the big complex if/else stuff at line 20 had to read (comments would help there too).

I spotted this: #2227073: Remove calls to drupal_get_path_alias(). I'm not sure if that's a performance problem, as AFAICT path aliases get cached statically.

The situation became worse ~two days ago, created:

#2239171: Regression: "Most recent comment" jump links navigate to an entirely new page

#2227073: Remove calls to drupal_get_path_alias() is a problem, it actually shouldn't even be called at all. I'll follow up in that issue with a patch.

That triggers a core bug, #1200478: Changing the comment path to the node path triggers a PHP fatal error in l() due to wrongly structured options.

Comment fragment will also need #2239337: Add 7.x-1.x-dev release.

Once the comment fragment issues are resolved, we can deploy this. My testing shows the module otherwise works well. We can apply the core patch to Drupal.org ahead of core commit, since it looks good.

Additional Drupal core issue to fix the secondary root cause:

#2239369: "Comments per page" setting cannot be configured to be larger than 300

This is now ready to deploy. I fixed #2239337: Add 7.x-1.x-dev release & #2239369: "Comments per page" setting cannot be configured to be larger than 300. And #1200478: Changing the comment path to the node path triggers a PHP fatal error in l() due to wrongly structured options has been committed to our BZR repo.

Now deployed & enabled.

<3

+++ b/project_issue.module
@@ -1774,8 +1774,18 @@ function project_issue_preprocess_comment(&$variables) {
+      $display_page = comment_get_display_page($comment->cid, 'project_issue');
+      if ($display_page > 0) {

This adds a big query execution for each comment render, are you really sure??

That's in project_issue module, not here. #268374: "Jump to" links are broken on multi-page issues would be the issue to re-open. But, only with specifics, not being unsure. We've been running that patch in production for about a week already.