Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
1. Installed Drupal 8.0.1
2. Created an article.
3. Posted some comments to the article.
Expected - the comment title should link to the comment fragment. /comment/123#comment-123
Actual - the comment title just points to /comment/123
Proposed resolution
Include the fragment to the comment in the link.
At #29 was decided to use the Permalink function instead of just add the fragment.
Remaining tasks
None
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#36 | missing_url_fragment-2642260-36.patch | 1.54 KB | dinarcon |
Comments
Comment #2
gargsuchi CreditAttribution: gargsuchi as a volunteer and at Acquia commentedThe attached patch fixes this problem.
Comment #3
gargsuchi CreditAttribution: gargsuchi as a volunteer and at Acquia commentedComment #4
gnugetHi!
This looks good! but with the txt extension this patch won't be tested by the testbot.
Can you please re-upload your patch using the guidelines? https://www.drupal.org/node/707484
Thanks!
Comment #5
gargsuchi CreditAttribution: gargsuchi as a volunteer and at Acquia commentedHere you go!
Comment #6
gnugetComment #7
gnugetTestbot didn't complain so this is RTBC to me.
Thanks!
Comment #8
Wim LeersThis needs test coverage to prevent future regressions.
Comment #9
gnugetOK, In this new patch I added the test.
Comment #12
gnugetNot sure why the testbot fails here, I tried with a slightly different approach this time
Comment #13
Wim LeersCan you provide an interdiff? It's difficult to figure out what's changed.
Comment #14
gnugetSure, this is the interdiff between #9 and #12
Thanks for your time.
Comment #15
lokapujyaI guess this was the fix to the test?
It's no longer testing that the title has the correct link. Now it's testing that the comment includes a fragment.
Comment #16
gnugetOk,I just added the comment id just to make sure to we are testing the correct link.
Thanks for your review!
Comment #17
lokapujyaI'll leave it up to you, but maybe we don't need to use the preg_match anymore if that works?
Comment #18
gnugetyup, you are right.
Thanks.
Comment #19
lokapujyaComment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedJust tested this manually and it is indeed a regression from D7.
The patch looks pretty good, I only found two small things that could be fixed:
Missing space after "(string)".
We should use assertEqual() here :)
Comment #21
lokapujya#20.2 now that you mention it, assertLinkByHref() may be an option too.
Comment #22
gnugetThanks for your reviews.
Here the patch with the changes.
I considered using assertLinkByHref but in this particular page we might have the same link twice (see #2641682: permalink breaks in comment) so I prefer to look for this specific link in the title of the comment, thanks for your suggestion, I didn't know about this function.
Comment #25
gnugetThis is weird, #18 passed before and now it failed, and #22 passed in my local version :-/
And looking at https://www.drupal.org/pift-ci-job/155572 what is that "checkout" think in the URL?
is this might be a malfunctioning of the bot? am I doing something wrong?
Comment #26
gnugetComment #27
gnugetLets see if this time I can make happy the testbot
Comment #28
lokapujyaComment #29
Wim Leers#25: #18 was likely a false positive due to #2657482: Test bot doesn't apply the patch.
The logic in
template_preprocess_comment()
is now actually making it a permalink… which means it can use$comment->permalink()
, just like$variables['permalink']
already does.This is nearly equal to
The only difference is the
$uri->setOption('attributes', $attributes);
call.So, this patch can hence AFAICT be simplified to:
Comment #30
gnugetWhile I'm agree with you about to we should use the permalink here instead of just add the fragment to the url the thing is the permalink function is broken.
The
$comment->permalink();
returns/node/1#comment-1
which is not a the permalink because this function doesn't consider the pagination of the nodes, indeed this was already reported in #2641682: permalink breaks in commentSo in my opinion what we need to do is pospone this issue fix #2641682: permalink breaks in comment and later come back to this one and use
$comment->permalink();
this time.What do you think?
Comment #31
Wim LeersWorks for me!
Comment #32
gnugetAlright, let's do that.
Comment #33
gnugetAlright #2641682: permalink breaks in comment was fixed now we can continue working on this one.
I will assign it to me.
Comment #34
gnugetOk in this new patch I used the permalink function.
Comment #35
gnugetComment #36
dinarcon CreditAttribution: dinarcon at Agaric commentedThe patch at number at #34 needed reroll. I have done that and tested it. It looks good. Thanks @gnuget!
Comment #37
larowlanminor: can we just use double quotes here instead of \'?
Should we also check that the href includes the fragment?
Other than that, looks RTBC to me
Comment #38
gnugetComment #39
naveenvalechaAccomodate #37 nit Still N/R for question in #37
Comment #40
naveenvalechaHiding #39 patches
Comment #41
larowlanThanks - #38 is RTBC
Comment #43
gnugetI return this to RTBC because the patch which failed was #39 and the patch RTBC is 38. (and I just make sure and it still applies correctly)
Comment #46
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!
Comment #47
cilefen CreditAttribution: cilefen commentedCan somebody please close #2793373: permalink for comments not working if it is a duplicate?
Comment #48
Wim Leers@cilefen: done.