Problem/Motivation
On CommentInterface we have two different methods CommentInterface::permalink and EntityInterface::uri. We also have comment_uri so If we want to remove comment_uri which method will replace it and why?
Proposed resolution
Please clarify the difference and document it.
Remaining tasks
Discussion
User interface changes
None
API changes
None
Related Issues
#2010202: Deprecate comment_uri()
#2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()
#1978904: Convert comment_admin() to a Controller
#1970360-78: Entities should define URI templates and standard links
Beta phase evaluation
Issue category | Task because it's a deprecation of an existing interface method, renaming it to prevent ambiguity. |
---|---|
Issue priority | Normal, non-critical. |
Disruption | Impact > Disruption, as there's virtually no disruption (only two changed theme variables and two controller method names), and the impact is to reduce the possibility of confusion of what the difference between Comment::permalink and Comment::uri is. |
Comment | File | Size | Author |
---|---|---|---|
#41 | deprecate-2113323-41.patch | 14.75 KB | SchnWalter |
#41 | 39-41-interdiff.txt | 3.83 KB | SchnWalter |
Comments
Comment #1
Crell CreditAttribution: Crell commentedI introduced the permalink() method while adding unified Entity URI links support. I have no idea why it's there, but the existing code had separate operations for comment_permalink() and the entity URI. The comment URI is the path that the comment lives at as far as REST is concerned; eg, /comment/123. The permalink is the anchored URI on the node page, eg, /node/123#comment-456.
No, this doesn't make sense to me at all. :-) Honestly the word "permalink" has always struck me as stupid and born of some popular blogging platform being utterly ignorant of how the web works in the first place, and then the term stuck. I left that split in place in the original patch because sorting that out was off topic for that issue.
The canonical entity URI needs to be anchor-less, for REST purposes, and we likely do need to retain a way to link to a comment "in context". Beyond that, and whether "permalink" is even the right name for that method (I'd be happy to purge the word from our lexicon entirely as a relic of ignorant developers 10 years ago who didn't know what they were doing) I don't know and largely don't care. :-)
Comment #2
andypostAccording #1970360-78: Entities should define URI templates and standard links entity URI can't have #fargment part so we need another method (
CommentInterface::permalink()
) to point the exact comment that's why we have route comment.permalink withCommentController::commentPermalink()
handlerAccording #2010202-18: Deprecate comment_uri() I think that also we need
CommentInterface::getCommentedEntityUri
with fragment to redirect user after new comment posted which requires us:1) calculate the page for the new comment
2) apply path alias for entity URI
3) add fragment
#comment-123
to linkComment #3
andypostAlso
permalink()
method is needed for better UX to allow users to easily copy/paste/share link for the commentBoth
uri()
andpermalink()
used in template_preprocess_comment()uri()
mostly used to generate comment related links (edit/delete/translate)pemalink()
used only to navigate from comment administration screen to the comment and for UX pointed aboveComment #4
catchWe definitely need to retain the ability to link to comments in context - that's what allows thinks like 'first unread comment' and 'most recent attachment' to work on issue queues for issues with more than 300 comments (once Drupal.org is running on 7.x).
Doesn't need to be called 'permalink' though, that was taken from http://drupal.org/project/permalink when the original bug fix went in iirc.
Comment #5
jibranFor the context of #1 http://en.wikipedia.org/wiki/Permalink
Comment #6
linclark CreditAttribution: linclark commentedYeah, agreed with catch. We need a way to show comments in context, but that shouldn't be called permalink because the word permalink does not require that the comments be shown in context. For example, look at the way that permalinks work in Reddit... http://www.reddit.com/r/drupal/comments/1oi244/iama_chx_ama/ccs4wlw.
Comment #7
jibranSo permalink should be
comment/$cid
which is same as uri IMO. Then what should we call$commented_entity_type/$commented_entity_id#commnet-$cid?page=whatever
. Related #2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()Comment #8
andypostSo I postponed #2010202: Deprecate comment_uri() on the issue
There's more problems that affects D.o migration too
1) currently links to comments in issues does not have
?page=x
when page > 0Suppose we need to make them
comment/x#commment-x
format2) SEO content duplication https://groups.drupal.org/node/26674
related issue #180379-45: Fix path matching in robots.txt
So better to have 2 methods to generate:
1)
comment/123#comment-123
- to display the 123 comment - this link generation looks like permalinks, we could easily override the router to display "reddit-like" comments without context or redirect to "contexted urls"2)
some_entity_url?page=YYY#comment-XXX
- should be used only in case of form redirectComment #9
linclark CreditAttribution: linclark commentedJust brainstorming a couple of ideas...
Comment #10
linclark CreditAttribution: linclark commentedIf we are already using the URI for the comment (
comment/123
), then why do we need a fragment there (#comment-123
)? Fragment identifiers are meant to refer to subordinate resources. Since we are already pointing to the comment, what does the fragment identify?Comment #11
andypostWe need this to navigate comment on page oncw core by default does not use "reddit-style"
Comment #12
linclark CreditAttribution: linclark commentedWhat do you want to see rendered at comment/123? Would the full thread get rendered?
I would assume only the comment gets rendered when I go to
comment/123
, and that the thread gets rendered when I go tosome_entity_url?page=YYY#comment-XXX
Comment #13
andypostCurrently on the page we make sub-request to get proper page of the comment and renders a whole entity and comment thread so we need
#comment-123
on it to navigate to needed comment.Suppose it would be better if after visiting
comment/123
we got redirected to propersome_entity_url?page=YYY#comment-XXX
so this become actual permalink for comment because in this redirect we can calculated the actual page for comment and this could makecomment::permalink()
to actually return the needed pageEDIT remember that this calculation is not cheap so docs should point that this function should not be used for render
Comment #14
catchFor reference the original issue that added this is #26966: Fix comment links when paging is used.. The original bug was was what got me into Drupal core development back in 2007.
@jibran/@andypost/@linclark
$commented_entity_type/$commented_entity_id#commnet-$cid?page=whatever
. isn't a good link to print anywhere.Especially with threaded comments when a comment can move between pages, but also if the number of comments per page changes, or if some comments are deleted/unpublished, then the page a comment appears on can change and those links go stale. comment/123#comment-123 guarantees that regardless of any of this it's always shown in the right place. Sticking with comment/123#comment-123 then redirecting also has issues:
1. HTTP redirects take time. Linking to a page that redirects is less than ideal too.
2. Lots of people share links by cutting and pasting from browser address bars, and again the ?page is unreliable with threaded comments.
On the original issue that introduced this, one idea was node/123/comment/321#comment-321 for the comment in context. I think we went for comment/123 in case comments applied to any entity at some point -obviously they do now, so it'd be a question of generating the link knowing the entity type that the comment applies to, which we can find out easily enough but would mean some dependencies in the comment class.
It'd be nice not to have the fragment since it looks redundant, but the only way to nuke that would be using js to scroll to the right place which is a lot less efficient than just using #fragment.
Comment #15
jibranI think first we have to identify the problem here.
$uri
so edit link will be$uri/edit
and delete will be$uri/delete
.Now we have two options generate
$url
for each comment.$comment->id()
as_GET
param to commented entity display and scroll to the comment using JS. In this case page number should be part of$url
.@catch is right
$commented_entity_type/$commented_entity_id#commnet-$cid?page=whatever
is not a good link. But IMOcomment/$comment->id()
is fine.My Idea, if we have rest request to
comment/$comment->id()
it should return$comment
entity and if we have HTTP request it should redirect to commented entity display with JS scroll to remove fragment.Yes redirect is slow but we have permanent link to
$comment
entity we can use link to share/SEO and we can access the commented entity display where comment entity is displayed.In this way we can cover all the cases.
Comment #16
catchI don't think these are the only two options - we can also keep the existing link structure which solves both the HTTP redirect and front-end performance issue for the comment in context links.
Comment #17
ianthomas_ukI agree with #12, in other REST APIs that I have used comment/123 would display the contents of comment 123 and maybe some identifiers that allowed me to retrieve details of its associated entities. Similarly, if I go to comment/123/edit I'd expect to see the edit form for that comment, I wouldn't expect to be able to edit the node and all other comments.
I propose that
To avoid duplicate content SEO issues there should be an option to HTTP redirect from the entity URI to the contextual URI, but that would not routinely be used because the contextual URI would be the standard one output.
I'd be happy to make a patch for this if other people think it's a good idea.
Comment #18
catchRather than that, we should use rel=canonical - that's already used for the comment/$cid#comment-$cid version (to point to node/n) iirc.
Comment #19
andypostTalked with @klausi in IRC and he said that REST currently does not use render system so it does not matter what is rendered at 'comment/{id}' so proposed solution is:
1) use
/comment/{id}
for entity uri2) use some other method
$comment->contentualURI()
for threaded list@jibran, @larowlan are you agree?
Comment #20
sunAdjusting issue title for recent comments/conclusions.
I would think the
uri()
method can stay as-is — it shouldn't hurt anyone if it contains a fragment?permalink()
is indeed ambiguous withuri()
. That said, all themes in core are outputting a (literal) "permalink" link/anchor for each comment, so the namepermalink()
is at least consistent with that.Contrary to
uri()
,permalink()
generates a link to the comment entity within the context of a parent entity.The current implementation hard-codes a bold assumption that the comment will actually appear on the entity view URI of the context entity — which is not necessarily the case, especially with Views in core. → The method should get the context URI injected.
The recent improvements of #2167641: EntityInterface::uri() should use route name and not path and #2153891: Add a Url value object might help us to make some more sense of this:
Comment #21
andypostActual trouble with this approach that comment needs own route/controller for that contextual URI to be able to calculate page.
This calculation depends on threading/per-page settings that going to change in #1920044: Move comment field settings that relate to rendering to formatter options
Suppose we need:
1) remove
comment_uri()
see #2010202-14: Deprecate comment_uri()2) add new
CommentInterface::uriByContext()
3) use 1-2 properly
Comment #22
larowlanComment #23
andypostProbably "novice" needs better IS
Comment #24
jcnventura CreditAttribution: jcnventura commentedI'm working on this one in DDD2015
Comment #25
jcnventura CreditAttribution: jcnventura commentedI might have gone overboard on renaming every instance of the term 'Permalink' in Drupal to 'Contextual URI'. If the objective is to change only the method I can also provide a patch with that subset.
Comment #26
jcnventura CreditAttribution: jcnventura commentedMouse wheel malfunction.
Comment #27
jibranWe need a beta eval here.
Comment #28
jcnventura CreditAttribution: jcnventura commentedIn preparation to the beta eval, I'm changing the patch to deprecate permalink instead of renaming it. We can then remove it later in 9.x. I've also realised that the isPermalink attribute in RSS is part of the guid attributes and should keep using the 'permalink' terminology.
Currently the patch does three things:
The last two can easily be removed from the patch, if these changes are not beta-compliant. Unfortunately, I don't think that marking them deprecated would work in this case.
Comment #29
jcnventura CreditAttribution: jcnventura commentedComment #30
andypost@jcnventura thanx for taking that!
Markup for comments is still flux, see #2031883: Markup for: comment.html.twig and related
The main question for me is what to do with
I think it should be removed and that should be done is one issue that brings sanity to comment routing!
Maybe better to override some method in entity link templates land to add the fragment to context?
Comment #33
heddnRemoving novice tag. Anything with discussion as the next step is not really a good novice.
Comment #34
nlisgo CreditAttribution: nlisgo commentedThis is a re-roll of #28.
Comment #39
SchnWalter CreditAttribution: SchnWalter as a volunteer and commentedI've re-rolled the patch against the latest 8.2.x, with a couple of fixes:
The last line should have been
$comment_contextual_uri->setOption('attributes', $attributes);
. Fixed it in the re-roll.The \Drupal\comment\Controller\CommentController::commentApprove() didn't have the "Redirects to the permalink URL for this comment", so the re-roll just adds that comment. I'm wondering if it has been removed on purpose since the last re-roll of this patch?
Comment #41
SchnWalter CreditAttribution: SchnWalter as a volunteer and commentedFixed failing test by reverted the text from "Parent contextual URI" to "Parent permalink", and from "Contextual URI" to "Permalink" and printing the proper variable.
And made a couple more changes:
NOTE: The parent_contextual_uri is not rendered in the comment template and it doesn't have tests.
Comment #42
SchnWalter CreditAttribution: SchnWalter as a volunteer and commentedComment #43
catchThis will break any custom/contrib templates relying on permalink. Should Stable add back the variable?
Comment #44
SchnWalter CreditAttribution: SchnWalter as a volunteer and commentedYes, and I also realized that there are no tests are running against the stable.theme for the permalink and probably other things. Which I believe is very bad.
I'll provide an updated patch soon.
Comment #49
andypostComment #50
xjmNew deprecations are minor-only changes. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!
Comment #57
catch